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 02:47:23PM +0200, David Hildenbrand (Arm) wrote:
> On 4/23/26 14:31, Greg Kroah-Hartman wrote:
> > 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.
> > 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: David Hildenbrand <david@kernel.org>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Reported-by: Anthropic
> > Assisted-by: gkh_clanker_t1000
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > My first foray into -mm, eeek!
> 
> Oh, nommu ... what a great use of our time.

Yeah, tell me about it.  I have been cursing a specific company's name a
lot these past days...

> I was briefly wondering if we want to add a Fixes: ... but then, this was likely
> broken for years and nobody cared so far in practice.

Agreed.

> > 
> > Anyway, this was a crazy report sent to me, and I knocked up this
> > change, and I have a reproducer if people need/want to see that as well
> > (it's for nommu systems, so be wary of it.)
> 
> [...]
> 
> > -				get_page(pages[i]);
> > +			if (pages[i]) {
> > +				/*
> > +				 * pin_user_pages*() arrives here with FOLL_PIN
> > +				 * set; unpin_user_page() (which is not
> > +				 * !CONFIG_MMU-specific) calls
> > +				 * gup_put_folio(..., FOLL_PIN) which subtracts
> > +				 * GUP_PIN_COUNTING_BIAS (1024).  A bare
> > +				 * get_page() here adds only 1, so 1023 pins on
> > +				 * a fresh page bring refcount to 1024 and a
> > +				 * single unpin then frees it out from under the
> > +				 * remaining 1022 pins and any live VMA
> > +				 * mappings. Use the same grab path as the MMU
> > +				 * implementation so pin and unpin are
> > +				 * symmetric.
> > +				 */
> 
> Yeah, drop all that. Especially the hardcoded 1024/1022 is just screaming for
> trouble longterm.

Ok, will drop!

> 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;
+				}
+			}
 		}
 
 		start = (start + PAGE_SIZE) & PAGE_MASK;