Re: [PATCH] mm: Fix invalid page pointer returned with FOLL_PIN gups

[Peter Xu]

Hi, Jason, John,

On Wed, Jan 26, 2022 at 08:42:06PM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 26, 2022 at 04:15:02PM -0800, John Hubbard wrote:
> 
> > > We had that -EEXIST logic since commit 1027e4436b6a ("mm: make GUP handle pfn
> > > mapping unless FOLL_GET is requested") which seems very reasonable.  It could
> > > be that when we reworked GUP with FOLL_PIN we could have overlooked that
> > > special path in commit 3faa52c03f44 ("mm/gup: track FOLL_PIN pages"), even if
> > > that commit rightfully touched up follow_devmap_pud() on checking FOLL_PIN when
> > > it needs to return an -EEXIST.
> 
> It sounds like this commit was all about changing the behavior of
> follow_page()
> 
> It feels like that is another ill-fated holdover from the effort to
> make pageless DAX that doesn't exist anymore.
> 
> Can we safely drop it now?

Not quite familiar with the dax effort, but just to note again that this patch
fixes an immediate breakage, hence IMHO we should still merge it first (not
mention it's a one-liner..).

> 
> Regardless..
> 
> > > diff --git a/mm/gup.c b/mm/gup.c
> > > index f0af462ac1e2..8ebc04058e97 100644
> > > +++ b/mm/gup.c
> > > @@ -440,7 +440,7 @@ static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address,
> > >   		pte_t *pte, unsigned int flags)
> > >   {
> > >   	/* No page to get reference */
> > > -	if (flags & FOLL_GET)
> > > +	if (flags & (FOLL_GET | FOLL_PIN))
> > >   		return -EFAULT;
> > 
> > Yes. This clearly fixes the problem that the patch describes, and also
> > clearly matches up with the Fixes tag. So that's correct.
> 
> It is a really confusing though, why not just always return -EEXIST
> here?

Because in current code GUP handles -EEXIST and -EFAULT differently?

We do early bail out on -EFAULT.  -EEXIST was first introduced in 2015 from
Kirill for not failing some mlock() or mmap(MAP_POPULATE) on dax (1027e4436b6).
Then in 2017 it got used again with pud-sized thp (a00cc7d9dd93d) on dax too.
They seem to service the same goal and it seems to be designed that -EEXIST
shouldn't fail GUP immediately.

> 
> The caller will always see the error code and refrain from trying to
> pin it and unwind upwards, just the same as -EFAULT. 
> 
> We shouldn't need to test the flags at this point at all.
> 
> > >   	if (flags & FOLL_TOUCH) {
> > > @@ -1181,7 +1181,13 @@ static long __get_user_pages(struct mm_struct *mm,
> > >   			/*
> > >   			 * Proper page table entry exists, but no corresponding
> > >   			 * struct page.
> > > +			 *
> > > +			 * Warn if we jumped over even with a valid **pages.
> > > +			 * It shouldn't trigger in practise, but when there's
> > > +			 * buggy returns on -EEXIST we'll warn before returning
> > > +			 * an invalid page pointer in the array.
> > >   			 */
> > > +			WARN_ON_ONCE(pages);
> > 
> > Here, however, I think we need to consider this a little more carefully,
> > and attempt to actually fix up this case. It is never going to be OK
> > here, to return a **pages array that has these little landmines of
> > potentially uninitialized pointers. And so continuing on *at all* seems
> > very wrong.
> 
> Indeed, it should just be like this:
> 
> @@ -1182,6 +1182,10 @@ static long __get_user_pages(struct mm_struct *mm,
>                          * Proper page table entry exists, but no corresponding
>                          * struct page.
>                          */
> +                       if (pages) {
> +                               page = ERR_PTR(-EFAULT);
> +                               goto out;
> +                       }
>                         goto next_page;
>                 } else if (IS_ERR(page)) {
>                         ret = PTR_ERR(page);

IIUC not failing -EEXIST immediately seems to be what we want.  We've discussed
the only (unless I overlooked something...) two sites that can return -EEXIST
in follow_page_mask() context; if it is triggered somewhere else unexpectedly
it is a programming error and needs fixing, imho.  Hence the gup code shouldn't
rely on the -EEXIST -> -EFAULT transition to work at all in any existing case.

>From that POV, WARN_ON_ONCE() helps better on exposing an illegal return of
-EEXIST (as mentioned in the commit message) than the -EFAULT convertion, IMHO.

Thanks,

-- 
Peter Xu