Re: [PATCH v3] mm/memfd: fix hugetlb reservation accounting in error paths

[David Hildenbrand (Arm)]

On 10/8/26 00:35, Andrew Morton wrote:
> On Sun, 27 Sep 2026 17:47:57 +0800 Hongfu Li <hongfu.li@xxxxxxxxx> wrote:
> 
>> From: Hongfu Li <lihongfu@xxxxxxxxxx>
>>
>> If hugetlb_add_to_page_cache() in memfd_alloc_folio() fails with -EEXIST,
>> a concurrent fault has already instantiated the folio in the page cache,
>> and the reservation now belongs to that folio. Calling
>> hugetlb_unreserve_pages() in that case incorrectly removes the region
>> backing the cached folio and releases one reservation more than it
>> should, leaving that folio in the page cache with no region recording it.
>>
>> That leaves resv_huge_pages one page short until that folio is removed
>> from the page cache, so available_huge_pages() reports a page that is
>> not actually free and the pool can grant one reservation more than it
>> can back. Applications using hugetlb memfds can then fail to allocate
>> or fault in a page they already reserved. They see -ENOMEM or -ENOSPC
>> from the allocation or fault path even though the reservation and
>> HugePages_Free still look healthy.
>>
>> So hold the hugetlb fault mutex from hugetlb_reserve_pages() until the
>> error-path unreserve completes to make the reserve, allocate and
>> instantiate steps atomic against concurrent faults. With the mutex held
>> from the start, a concurrent fault can no longer consume the reservation
>> between reserve and allocate/instantiate. If a fault completed before the
>> mutex was taken, it has already added the region for that index, so
>> hugetlb_reserve_pages() returns 0 and the error path leaves the region in
>> place.
>>
>> Fixes: 717cf9357325 ("mm/memfd: reserve hugetlb folios before allocation")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Hongfu Li <lihongfu@xxxxxxxxxx>
> 
> Can we please have review of this cc:stable regression fix?

Hugetlb AND reservation? what could possibly be nicer to review ;)

> 
>> v3:
>> - Update commit message to describe the accounting damage and its
>>   user-visible effect; no code changes.
>> - Add Cc: stable@xxxxxxxxxxxxxxx.
>> v2:
>> - Take the hugetlb fault mutex before hugetlb_reserve_pages() and hold
>>   it until the error-path unreserve completes.
>> - Update commit message
>> - Link to v1: https://lore.kernel.org/all/20260831090631.29227-1-hongfu.li@xxxxxxxxx/
>> ---
>>  mm/memfd.c | 31 ++++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/mm/memfd.c b/mm/memfd.c
>> index c708d92533f4..0f6fff004f5e 100644
>> --- a/mm/memfd.c
>> +++ b/mm/memfd.c
>> @@ -82,22 +82,31 @@ struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx)
>>  		struct hstate *h = hstate_file(memfd);
>>  		int err = -ENOMEM;
>>  		long nr_resv;
>> +		u32 hash;
>>  
>>  		gfp_mask = htlb_alloc_mask(h);
>>  		gfp_mask &= ~(__GFP_HIGHMEM | __GFP_MOVABLE);
>>  		idx >>= huge_page_order(h);
>>  
>> +		/*
>> +		 * Serialize hugepage allocation and instantiation to prevent
>> +		 * races with concurrent allocations, as required by all other
>> +		 * callers of hugetlb_add_to_page_cache().
>> +		 */
>> +		hash = hugetlb_fault_mutex_hash(memfd->f_mapping, idx);


I'd just do

	const u32 hash = hugetlb_fault_mutex_hash(memfd->f_mapping, idx);

above, but doesn't really matter for the fix.



>> +		mutex_lock(&hugetlb_fault_mutex_table[hash]);
>> +
>>  		nr_resv = hugetlb_reserve_pages(inode, idx, idx + 1, NULL, EMPTY_VMA_FLAGS);
>> -		if (nr_resv < 0)
>> -			return ERR_PTR(nr_resv);
>> +		if (nr_resv < 0) {
>> +			err = nr_resv;
>> +			goto out_unlock;
>> +		}
>>  
>>  		folio = alloc_hugetlb_folio_reserve(h,
>>  						    numa_node_id(),
>>  						    NULL,
>>  						    gfp_mask);
>>  		if (folio) {
>> -			u32 hash;
>> -
>>  			/*
>>  			 * Zero the folio to prevent information leaks to userspace.
>>  			 * Use folio_zero_user() which is optimized for huge/gigantic
>> @@ -112,20 +121,9 @@ struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx)
>>  			 */
>>  			__folio_mark_uptodate(folio);
>>  
>> -			/*
>> -			 * Serialize hugepage allocation and instantiation to prevent
>> -			 * races with concurrent allocations, as required by all other
>> -			 * callers of hugetlb_add_to_page_cache().
>> -			 */
>> -			hash = hugetlb_fault_mutex_hash(memfd->f_mapping, idx);
>> -			mutex_lock(&hugetlb_fault_mutex_table[hash]);
>> -
>>  			err = hugetlb_add_to_page_cache(folio,
>>  							memfd->f_mapping,
>>  							idx);
>> -
>> -			mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>> -
>>  			if (err) {
>>  				folio_put(folio);
>>  				goto err_unresv;
>> @@ -133,11 +131,14 @@ struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx)
>>  
>>  			hugetlb_set_folio_subpool(folio, subpool_inode(inode));
>>  			folio_unlock(folio);
>> +			mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>>  			return folio;
>>  		}
>>  err_unresv:
>>  		if (nr_resv > 0)
>>  			hugetlb_unreserve_pages(inode, idx, idx + 1, 0);
>> +out_unlock:
>> +		mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>>  		return ERR_PTR(err);
>>  	}
>>  #endif

We now run allcoation+zeroing under the mutex. Which is the same as done in
hugetlb_no_page(). (I just looked at that function, now I have to wash my eyes)

So holding the mutex for longer seems to be the right thing to do.

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David