Re: [PATCH v2 13/13] mm/gup: move private gup FOLL_ flags to internal.h

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Thu, 26 Jan 2023 08:55:27 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Thu, Jan 26, 2023 at 01:48:46PM +0100, David Hildenbrand wrote:
> > On 24.01.23 21:34, Jason Gunthorpe wrote:  
> > > Move the flags that should not/are not used outside gup.c and related into
> > > mm/internal.h to discourage driver abuse.
> > > 
> > > To make this more maintainable going forward compact the two FOLL ranges
> > > with new bit numbers from 0 to 11 and 16 to 21, using shifts so it is
> > > explict.
> > > 
> > > Switch to an enum so the whole thing is easier to read.  
> > 
> > Using a __bitwise type would be even better, but that requires quite some
> > adjustments ...
> > 
> > The primary leftover for FOLL_GET seems to be follow_page(). IIRC, there is
> > only one caller that doesn't pass FOLL_GET (s390). We could either add a new
> > function to "probe" that anything is mapped (IIRC that's the use case), or
> > simply ref+unref.  
> 
> Is that code even safe as written? I don't really understand how it

yes (surprisingly) it is

> can safely call lock_page() on something it doesn't have a reference
> too ?

the code between lock_page and unlock_page will behave "properly" and
do nothing or at worst cause a tiny performance issue in the rare case
something changes between the follow_page and the page_lock, i.e. if
things are done on the wrong page.

make_secure_pte does some very hacky stuff involving page_ref_freeze,
with expected_page_refs counting how many references are expected.

the code is how it is because, when it was written, FOLL_PIN did
not exist, and FOLL_GET caused the page to be converted to unsecure.

I guess maybe we can make it work with FOLL_GET if expected_page_refs
takes into account the extra reference? (and then maybe we could a
put_page after unlock_page)

> 
> So adding the FOLL_GET and put_page seems like a good idea to me? At a
> minimum this should get a comment to explain why it is OK.
> 
> S390 people?
> 
> int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
> {
> [..]
>         rc = -ENXIO;
>         page = follow_page(vma, uaddr, FOLL_WRITE);
>         if (IS_ERR_OR_NULL(page))
>                 goto out;
> 
>         lock_page(page);
>         ptep = get_locked_pte(gmap->mm, uaddr, &ptelock);
> 
> Jason