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

[David Hildenbrand (Arm)]

On 9/21/26 11:57, David Hildenbrand (Arm) wrote:
> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> 
> The !CONFIG_MMU implementation of __get_user_pages_locked() takes a bare
> get_page() reference for each page regardless of foll_flags:
> 	if (pages[i])
> 		get_page(pages[i]);
> 
> This is reached from pin_user_pages*() with FOLL_PIN set.
> unpin_user_page() is shared between MMU and NOMMU configurations and
> unconditionally calls gup_put_folio(..., FOLL_PIN), which subtracts
> GUP_PIN_COUNTING_BIAS (1024) from the folio refcount.
> 
> This means that pin adds 1, and then unpin will subtract 1024.
> 
> If a user maps a page (refcount 1), registers it 1023 times as an
> io_uring fixed buffer (1023 pin_user_pages calls -> refcount 1024), then
> unregisters: the first unpin_user_page subtracts 1024, refcount hits 0,
> the page is freed and returned to the buddy allocator.  The remaining
> 1022 unpins write into whatever was reallocated, and the user's VMA
> still maps the freed page (NOMMU has no MMU to invalidate it).
> Reallocating the page for an io_uring pbuf_ring then lets userspace
> corrupt the new owner's data through the stale mapping.
> 
> Use try_grab_folio() which adds GUP_PIN_COUNTING_BIAS for FOLL_PIN and 1
> for FOLL_GET, mirroring the CONFIG_MMU path so pin and unpin are
> symmetric. Keep supporting the traditional behavior where users specify
> a pages array but don't set FOLL_GET.
> 
> While at it, don't return NULL pointers in the page array,
> as this is really not expected for GUP users; instead, just fail and return
> -EFAULT.
> 
> [ david: support traditional behavior with no FOLL_GET, extend
>   description ]
> 
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>
> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> Cc: Peter Xu <peterx@xxxxxxxxxx>
> Cc: Lance Yang <lance.yang@xxxxxxxxx>
> Reported-by: Anthropic
> Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
> Assisted-by: gkh_clanker_t1000
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> ---
> v3 -> v4:
> * Put FOLL_GET fixup into the NOMMU __get_user_pages_locked()
> 
> I'll queue the updated version after some more review.
> ---
>  mm/gup.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index eb898ea1ee22e..4e9b5ac2cb62e 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)
> @@ -1999,6 +2000,10 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>  		*locked = 1;
>  	}
>  
> +	/* See the MMU variant: support the traditional behavior. */
> +	if (pages && !(foll_flags & FOLL_PIN))
> +		foll_flags |= FOLL_GET;
> +
>  	/* calculate required read or write permissions.
>  	 * If FOLL_FORCE is set, we only require the "MAY" flags.
>  	 */
> @@ -2019,8 +2024,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);

BTW Sashiko reports:

"Does this code leak the folio if it is a slab page?" ... "When the operation
completes, the caller releases the page via put_page(). However, put_page()
explicitly checks folio_test_slab() and returns without
decrementing the refcount."

get_page() contains

	if (WARN_ON_ONCE(folio_test_slab(folio)))

for a good reason. It's not supposed to happen. So if we would get slab pages
here we'd be in "preexisting problem" territory.

As slab pages have a frozen refcount, we'd similarly trigger

if (WARN_ON_ONCE(folio_ref_count(folio) <= 0))

Now in try_grab_folio and refuse the operation.

So I'll ignore this one.

-- 
Cheers,

David