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

[David Hildenbrand (Arm)]

On 4/24/26 13:31, Greg Kroah-Hartman wrote:
> 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@xxxxxxxxxxxxxxxxxxxx>
>>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>>> Cc: Jason Gunthorpe <jgg@xxxxxxxx>
>>> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
>>> Cc: Peter Xu <peterx@xxxxxxxxxx>
>>> 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@xxxxxxxxxx>

I think we usually use Suggested-by:, but really I was just joking :)

> 
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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!

Andrew, can you add "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."? Thanks!

-- 
Cheers,

David