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

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

On Thu, Apr 23, 2026 at 05:55:56PM +0200, David Hildenbrand (Arm) wrote:
> On 4/23/26 16:28, 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
> 
> Assisted-by: David :(
> 
> (no, I'm not a tool! :) )

True, sorry, I guess people can "assist", I should have added that.

If Andrew's tools automatically pick this up then:

Assisted-by: David Hildenbrand <david@kernel.org>

> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > v2: - drop huge comment
> >     - rework error return value based on David's suggestion (heck,
> >       pretty much the full patch was written by him now)
> >     Link to v1: https://lore.kernel.org/r/2026042334-acutely-unadorned-e05c@gregkh
> > 
> >  mm/gup.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index ad9ded39609c..2f6f95a167af 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;
> 
> Best to mention that change in the patch description. I really think this is the
> right thing to do (returning NULL in the page array is just very dubious).

Ick, I see Andrew already grabbed this so I'll just leave it for now,
thanks for the help and review!

greg k-h