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

David Hildenbrand (Arm)
2026-04-23T13:05:04Z

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

--
Cheers,

David