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

[John Hubbard <jhubbard@nvidia.com>]

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