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?
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?
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);
Jason
