Re: [PATCH 5/5] mm/gup: limit pagecache folio refcount when grabbing references

[David Hildenbrand (Arm)]

On 11/6/26 11:13, Jose A. Perez de Azpillaga wrote:
> On 11/5/26 2:10 PM, David Hildenbrand (Arm) wrote:
>> Let's replace our existing safety check by the new
>> folio_ref_count_over_limit() check in fast and slow GUP before acquiring
>> additional references or pins.
>>
>> Keep it simple and use the check for all folio types.
>>
>> Drop the WARN_ON_ONCE(), because apparently these things can currently
>> happen with large pagecache folios.
>>
>> Keep the zero-refcount rejection if the refcount is 0.
>>
>> Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
>> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
>> Fixes: 793917d997df ("mm/readahead: Add large folio readahead")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> ---
>>  mm/gup.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 2ab42fc6834e5..db9db6b2a4662 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -77,7 +77,7 @@ static inline struct folio *try_get_folio(struct page *page, int refs)
>>  
>>  retry:
>>  	folio = page_folio(page);
>> -	if (WARN_ON_ONCE(folio_ref_count(folio) < 0))
>> +	if (unlikely(folio_ref_count_over_limit(folio)))
>>  		return NULL;
>>  	if (unlikely(!folio_ref_try_add(folio, refs)))
>>  		return NULL;
>> @@ -140,7 +140,11 @@ static void gup_put_folio(struct folio *folio, int refs, unsigned int flags)
>>  int __must_check try_grab_folio(struct folio *folio, int refs,
>>  				unsigned int flags)
>>  {
>> -	if (WARN_ON_ONCE(folio_ref_count(folio) <= 0))
>> +	if ((flags & (FOLL_GET | FOLL_PIN)) &&
>> +	    unlikely(folio_ref_count_over_limit(folio)))
>> +		return -ENOMEM;
>> +
>> +	if (WARN_ON_ONCE(!folio_ref_count(folio)))
>>  		return -ENOMEM;
>>  
>>  	if (unlikely(!(flags & FOLL_PCI_P2PDMA) && folio_is_pci_p2pdma(folio)))
>> @@ -1482,8 +1486,8 @@ static long __get_user_pages(struct mm_struct *mm,
>>  				struct folio *folio = page_folio(page);
>>  
>>  				/*
>> -				 * Since we already hold refcount on the
>> -				 * large folio, this should never fail.
>> +				 * Grabbing more references can fail once we
>> +				 * hit folio refcount limits.
>>  				 */
>>  				if (try_grab_folio(folio, page_increm - 1,
>>  						   gup_flags)) {
> 
> this can fail now, so shouldn't the error path below return -ENOMEM like
> the first page does, instead of -EFAULT?

Hm, indeed, that's inconsistent. The issue is that we have this -EREMOTEIO check
mangled in there. But the ordinary path would already have checked that and
failed with he same error.

So this hunk would become:

                                /*
-                                * Since we already hold refcount on the
-                                * large folio, this should never fail.
+                                * Grabbing more references can fail once we
+                                * hit folio refcount limits.
                                 */
-                               if (try_grab_folio(folio, page_increm - 1,
-                                                  gup_flags)) {
+                               ret = try_grab_folio(folio, page_increm - 1,
+                                                    gup_flags);
+                               if (ret) {
                                        /*
-                                        * Release the 1st page ref if the
-                                        * folio is problematic, fail hard.
+                                        * Release the folio reference we
+                                        * already took.
                                         */
                                        gup_put_folio(folio, 1, gup_flags);
-                                       ret = -EFAULT;
                                        goto out;
                                }
                        }

-- 
Cheers,

David