Re: [PATCH] mm/gup: honour FOLL_PIN in NOMMU __get_user_pages_locked()

Greg Kroah-Hartman
2026-04-23T13:11:30Z

On Thu, Apr 23, 2026 at 03:04:56PM +0200, David Hildenbrand (Arm) wrote:
>
> >> It just follows what we do everywhere else (e.g., follow_page_pte()).
> >>
> >>
> >>> + if (try_grab_folio(page_folio(pages[i]), 1,
> >>> + foll_flags)) {
> >>> + pages[i] = NULL;
> >>> + break;
> >>> + }
> >>> + }
> >>
> >> If it fails on the first iteration, we return -EFAULT instead of -ENOMEM.
> >>
> >> I know, I know, nobody cares. But if we touch it, we might just want to return
> >> the error we get from try_grab_folio().
> >
> > So just abort here and return it? No, that will not work, there's a
> > lock we would jump around. How about something like this horrid thing,
> > adding back in the relevant unlikely() to match the other calls like
> > this:
> >
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index ad9ded39609c..8fa5b37be8b7 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1983,6 +1983,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> > struct vm_area_struct *vma;
> > bool must_unlock = false;
> > vm_flags_t vm_flags;
> > + int ret;
> > long i;
> >
> > if (!nr_pages)
> > @@ -2019,8 +2020,15 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> >
> > if (pages) {
> > pages[i] = virt_to_page((void *)start);
> > - if (pages[i])
> > - get_page(pages[i]);
> > + if (pages[i]) {
> > + ret = try_grab_folio(page_folio(pages[i]), 1,
> > + foll_flags);
> > + if (unlikely(ret)) {
> > + pages[i] = NULL;
> > + i = ret;
> > + break;
>
> So, we have to report the old i in case of an error.
>
> Assuming we want to also return -EFAULT on !pages[i] -- which i think we want,
> maybe the following (uncompiled and untested)?
>
> diff --git a/mm/gup.c b/mm/gup.c
> index ad9ded39609c..d00ff8d988fa 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1983,6 +1983,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> struct vm_area_struct *vma;
> bool must_unlock = false;
> vm_flags_t vm_flags;
> + int ret, err = -EFAULT;
> long i;
>
> if (!nr_pages)
> @@ -2019,8 +2020,14 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>
> if (pages) {
> pages[i] = virt_to_page((void *)start);
> - if (pages[i])
> - get_page(pages[i]);
> + if (!pages[i])
> + break;
> + ret = try_grab_folio(page_folio(pages[i]), 1, foll_flags);
> + if (ret) {
> + pages[i] = NULL;
> + err = ret;
> + break
> + }
> }
>
> start = (start + PAGE_SIZE) & PAGE_MASK;
> @@ -2031,7 +2038,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> *locked = 0;
> }
>
> - return i ? : -EFAULT;
> + return i ? : err;
> }
> #endif /* !CONFIG_MMU */

Cool, let me through this at my reproducer and see how it goes!

Ugh, nommu...