netdev - Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings by default

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <92b1040b-dffc-9a4a-340e-99e9822821b0@nvidia.com>
Date:   Thu, 27 Apr 2023 15:44:07 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Lorenzo Stoakes <lstoakes@...il.com>
CC:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jason Gunthorpe <jgg@...pe.ca>, Jens Axboe <axboe@...nel.dk>,
        Matthew Wilcox <willy@...radead.org>,
        Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
        Leon Romanovsky <leon@...nel.org>,
        Christian Benvenuti <benve@...co.com>,
        Nelson Escobar <neescoba@...co.com>,
        Bernard Metzler <bmt@...ich.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        "Namhyung Kim" <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Bjorn Topel <bjorn@...nel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "Christian Brauner" <brauner@...nel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        <linux-fsdevel@...r.kernel.org>,
        <linux-perf-users@...r.kernel.org>, <netdev@...r.kernel.org>,
        <bpf@...r.kernel.org>, Oleg Nesterov <oleg@...hat.com>,
        Jason Gunthorpe <jgg@...dia.com>, Jan Kara <jack@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Pavel Begunkov <asml.silence@...il.com>
Subject: Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings
 by default

On 4/27/23 15:31, Lorenzo Stoakes wrote:
...
>>         bool file_backed = !vma_is_anonymous(vma);
>>
>> would lead to a slightly better reading experience below.
> 
> Well you see, I'm not so sure about that, because vma_is_anonymous() checks
> vm_ops == NULL not vm_file == NULL which can be the case for a special
> mapping like VDSO that is not in fact file-backed :) the horror, the
> horror.
> 

Yes, you are right. It looks like vma_anon is a better name here,
after all.

...
>> ...and now we call it again. I think once should be enough, though.
> 
> Right, this was intentional (I think I mentioned it in the revision
> notes?), because there is a conundrum here - the invocation from
> vma_wants_writenotify() needs to check this _first_ before performing the
> _other_ checks in vma_needs_dirty_tracking(), but external calls need all
> the checks. It'd be ugly to pass a boolean to see if we should check this
> or not, and it's hardly an egregious duplication for the _computer_
> (something likely in a cache line != NULL) which aids readability and
> reduces duplication for the _reader_ of the code for a path that is
> inherently slow (likely going to fault in pages etc.)
> 
> I think it'd be confusing to have yet another split into
> vma_can_track_dirty() or whatever because then suddenly for the check to be
> meaningful you have to _always_ check 2 things.
> 
> Other options like passing an output parameter or returning something other
> than boolean are equally distasteful.

Agreed. (And yes, I overlooked that discussion in the version notes.)

> 
>>
>> Also, with the exception of that double call to
>> vm_ops_needs_writenotify(), these changes to mmap.c are code cleanup
>> that has the same behavior as before. As such, it's better to separate
>> them out from this patch whose goal is very much to change behavior.
> 
> It's not really cleanup, it's separating out some of the logic explicitly
> to be used in this new context, without which the separation would not be
> useful, so I feel it's a bit over the top to turn a small single patch into
> two simply to avoid this.
> 

Sure, OK.

>>
> 
> Thanks for the review, I will respin with the suggestions (other than ones
> I don't quite agree with as explained above) and a clearer description in
> line with Mika's suggestions.
> 
> Hopefully we can move closer to this actually getting some
> reviewed/acked-by tags soon :)

thanks,
-- 
John Hubbard
NVIDIA