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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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 */

Yup, this passes my tests, let me go send a v2 with this version,
thanks!

greg k-h