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

[John Hubbard]

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