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

[sashiko-bot@kernel.org]

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-fixes_hugetlb-v1-0-a0a033db180f@kernel.org?part=14