Re: [PATCH 14/15] RFC: mm/hugetlb: account restored reservations before freeing folios

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/29/26 21:16, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mm/hugetlb: gather_surplus_pages() over-allocates and retains surplus folios
> --
> 
> commit d5256483029a06972dd519de19de57831de33af4
> Author: David Hildenbrand (Arm) <david@kernel.org>
> 
> RFC: mm/hugetlb: account restored reservations before freeing folios
> 
> This commit updates anonymous hugetlb folio unmapping to increment
> resv_huge_pages immediately when restoring the VMA reservation rather than
> waiting for the folio to be freed. Because resv_huge_pages can now
> temporarily exceed free_huge_pages while unmapped folios still have
> elevated reference counts, available_huge_pages() is updated to clamp at
> zero.
> 
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 69a5d2c73ee1d..06ef8ba41df7c 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
> [ ... ]
>> @@ -5313,6 +5309,45 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
>>  	return len + old_addr - old_end;
>>  }
>>  
>> +/* Must be called after rmap removal. */
>> +static void hugetlb_folio_restore_rsv_on_unmap(struct vm_area_struct *vma,
>> +		struct folio *folio)
>> +{
>> +	struct hstate *h = hstate_vma(vma);
>> +	unsigned long address;
>> +	long rc;
>> +
>> +	/*
>> +	 * Restore only on the last unmap, otherwise the owner could reuse
>> +	 * the reservation while the folio is still mapped by a child.
>> +	 *
>> +	 * TODO: Handle migration temporarily dropping the mapcount to zero.
>> +	 * TODO: Prevent reservation reuse before the folio is freed.
>> +	 */
>> +	if (!folio_test_anon(folio) || !__vma_private_lock(vma) ||
>> +	    folio_mapped(folio))
>> +		return;
>> +
>> +	address = vma_anon_address(vma, folio->index, 1);
>> +	rc = vma_needs_reservation(h, vma, address);
>> +	if (!rc)
>> +		vma_end_reservation(h, vma, address);
>> +	if (rc <= 0)
>> +		return;
>> +
>> +	rc = vma_add_reservation(h, vma, address);
>> +	if (rc < 0)
>> +		return;
>> +
>> +	/* The restored reservation owns the subpool charge. */
>> +	hugetlb_set_folio_subpool(folio, NULL);
>> +
>> +	/* Account before the VMA can close or fault again. */
>> +	spin_lock_irq(&hugetlb_lock);
>> +	h->resv_huge_pages++;
>> +	spin_unlock_irq(&hugetlb_lock);
>> +}
> 
> [Severity: High]
> With h->resv_huge_pages now able to exceed h->free_huge_pages while an
> unmapped folio is still referenced (for example, via vmsplice() or GUP),
> can a concurrent hugetlb_acct_memory() -> gather_surplus_pages() over-allocate
> surplus folios and retain them in the pool?
> 
> In surplus_pages_needed():
> 
> 	static long surplus_pages_needed(struct hstate *h, long delta, long allocated)
> 	{
> 		long global_free = (long)h->free_huge_pages + allocated;
> 		long allowed_free = (long)allowed_mems_nr(h) + allocated;
> 
> 		return max((long)h->resv_huge_pages + delta - global_free,
> 			   delta - allowed_free);
> 	}
> 
> When h->resv_huge_pages > h->free_huge_pages, (long)h->resv_huge_pages +
> delta - global_free includes the deficit from the in-flight unmapped
> folios on top of delta.
> 
> If gather_surplus_pages() succeeds, needed += allocated equals delta +
> (h->resv_huge_pages - h->free_huge_pages), so it allocates and enqueues
> extra surplus folios beyond delta (or fails with -ENOMEM if
> nr_overcommit_huge_pages only had room for delta).
> 
> Furthermore, when gather_surplus_pages() cleans up unused or aborted
> folios at the free label:
> 
> 	free:
> 		spin_unlock_irq(&hugetlb_lock);
> 
> 		/*
> 		 * Free unnecessary surplus pages to the buddy allocator.
> 		 * Pages have no ref count, call free_huge_folio directly.
> 		 */
> 		list_for_each_entry_safe(folio, tmp, &surplus_list, lru)
> 			free_huge_folio(folio);
> 
> free_huge_folio() checks:
> 
> 	} else if (h->surplus_huge_pages_node[nid] &&
> 		   h->free_huge_pages >= h->resv_huge_pages) {
> 		/* Free this surplus folio that is not required globally. */
> 		remove_hugetlb_folio(h, folio, true);
> 		spin_unlock_irqrestore(&hugetlb_lock, flags);
> 		update_and_free_hugetlb_folio(h, folio, true);
> 	} else {
> 		arch_clear_hugetlb_flags(folio);
> 		enqueue_hugetlb_folio(h, folio);
> 		spin_unlock_irqrestore(&hugetlb_lock, flags);
> 	}
> 
> While h->free_huge_pages < h->resv_huge_pages, free_huge_folio() will
> enqueue the aborted surplus folios into the hugetlb pool instead of
> returning them to the buddy allocator. If the in-flight unmapped folio is
> a persistent folio on node A (where surplus_huge_pages_node[nid_A] == 0)
> and the surplus folio was allocated on node B, dropping the last reference
> to the persistent folio on node A will not free the surplus folio on
> node B either, leaving the surplus folio stranded in the pool.
> 

Damn that's confusing. What an overengineered mess :)

Surplus pages ... another thing that should have never been merged.

I guess we'd have to clamp also in surplus_pages_needed:

@@ -2317,11 +2312,13 @@ static nodemask_t *policy_mbind_nodemask(gfp_t gfp)
  */
 static long surplus_pages_needed(struct hstate *h, long delta, long allocated)
 {
-	long global_free = (long)h->free_huge_pages + allocated;
-	long allowed_free = (long)allowed_mems_nr(h) + allocated;
+	long available = min(available_huge_pages(h), allowed_mems_nr(h));

-	return max((long)h->resv_huge_pages + delta - global_free,
-		   delta - allowed_free);
+	return delta - available - allocated;
 }


And in gather_surplus_pages() we'd then have to be more careful about which
surplus folios we're actually freeing.

-- 
Cheers,

David