Re: [PATCH v2 2/5] mm: hugetlb: Fix subpool usage leak on allocation failure

[Ackerley Tng]

Ackerley Tng <ackerleytng@xxxxxxxxxx> writes:

> Joshua Hahn <joshua.hahnjy@xxxxxxxxx> writes:
>
>> Hi Ackerley,
>>
>> Thank you for this series. I really wanted to work on hugeTLB accounting
>> fixes but never got the time to get to it. I'm very grateful that you
>> are taking a look!!
>>
>>> From: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>>>
>>> When alloc_hugetlb_folio() fails early (e.g. buddy allocation failure or
>>> hugetlb cgroup charging failure) and gbl_chg == 1 (meaning a reservation
>>> was not used, but a global page was allocated instead), the subpool page
>>> acquired via hugepage_subpool_get_pages() must still be returned.
>>>
>>> Currently, the error path out_subpool_put: only calls
>>> hugepage_subpool_put_pages() if !gbl_chg is true. If gbl_chg is 1, it
>>> skips it, permanently leaking the subpool's used_hpages counter.
>>>
>>> With the earlier patch to always track used_hpages in the subpool, always
>>> call hugepage_subpool_put_pages() if map_chg is true to consistently
>>> restore the page to the subpool. Only call hugetlb_acct_memory() to adjust
>>> global reservations if gbl_chg == 0 since gbl_chg == 0 indicates a
>>> subpool (and global) reservation was used.
>>
>> So I think that I've seen that this part of the accounting specifically
>> is a bit suspicious. There have been two attempts in the past to fix
>> this area [1] [2]. I think functionally they are quite similar to this
>> fix, they just open-code the contents of the put_pages function inside
>> the condition. I've Cc-ed the authors of those two patches in case
>> they wanted to chime in.
>>
>
> Thanks for connecting us! I didn't realize this was already being worked
> on. Also adding Lance, who commented at [3].
>
>> I reference these fixes because I think they handle the minimum
>> subpage case a bit differently. To be honest, I recall reading those
>> fixes a while back and getting a bit confused on what exactly happens
>> when the page is absorbed to fulfill the minimum size...
>>
>> It does seem like Sashiko also notes this as a possible concern.
>> WDYT? Does your reproducer for this issue also work when a minimum
>> size is set (let's say, to 1?)
>>
>
> Let me look into this more!
>

I was looking at Sashiko's comment [4] and Sashiko is right that there
could be a race. Specifically, at the time of
hugepage_subpool_get_page(), I might be using a reservation, hence
returning 1 (thread A), but at the time of hugepage_subpool_put_page(),
some other thread (B) might already have restored a reservation to the
subpool.

With used_hpages tracking within the subpool, we can rely on the return value of
hugepage_subpool_put_page():

+ Return 1: Reservation should be returned elsewhere (pool has enough pages to
  satisfy minimum, or doesn't track minimums)
+ Return 0: Reservation does not need to be returned elsewhere

But calling hugepage_subpool_put_pages(1) doesn't say anything about whether the
1 (in this case, 1 doesn't always represent a page, since specifically on the
error cases, the page wasn't allocated) consumed a reservation or not.

+ gbl_chg == 0 means a reservation was used, and
+ gbl_chg == 1 means a reservation *should be used*, but
+ gbl_chg == 1 does not necessarily mean a reservation should be used, since
  h->resv_huge_pages may not have been decremented yet. (See goto
  out_subpool_put and goto out_uncharge_cgroup_reservation in
  alloc_hugetlb_folio())

So, we need to track if h->resv_huge_pages-- happened (with some local variable
like reservation_consumed)

| reservation_consumed | hugepage_subpool_put_pages() return value |
h->resv_huge_pages++ |
|----------------------|-------------------------------------------|----------------------|
| 1                    | 1                                         |
Yes                  |
| 1                    | 0                                         |
No                   |
| 0                    | 1                                         |
No                   |
| 0                    | 0                                         |
No                   |

TLDR: replace !gbl_chg with reservation_consumed?

    <<== reservation_consumed = true

	if (!gbl_chg) {
		folio_set_hugetlb_restore_reserve(folio);
		h->resv_huge_pages--;
        <<== reservation_consumed = true
	}

	if (map_chg) {
		long gbl_reserve = hugepage_subpool_put_pages(spool, 1);

		if (!gbl_chg)  <<== this should be reservation_consumed
			hugetlb_acct_memory(h, -gbl_reserve);
	}

I believe this depends on always tracking used_hpages in subpools, in order to
know for sure, just based on a number (with no page information), if the page is
supposed to use a reservation.


Perhaps related: does it make sense to flipping the reservation tracking
to instead track an "available" page count? This is basically the
mathematical opposite of rsvd_huge_pages, as in,

  available_pages = free_huge_pages - resv_huge_pages

This way, there's no need to synchronize the number of reserved pages in
both the subpool and global hstate.

It's quite confusing now, that reserved_pages-- can be either "used a
reservation" or "unreserve". If we track the opposite, available_pages--
will definitely mean the page's availability is removed from the global
hstate and transferred to the subpool, and allocating page doesn't
involve updating both the subpool and hstate.

One downside I can think of is that creating hugepages (surplus, or
promote/demote, or hotplug, or runtime echo 1 > nr_hugepages) would need
to update the global hstate's available_pages count, but I imagine that
would be less frequent than allocating huge pages?

>>
>> [...snip...]
>>
>>
>> [1] https://lore.kernel.org/linux-mm/20260428113037.88766-2-enderaoelyther@xxxxxxxxx/
>> [2] https://lore.kernel.org/linux-mm/20260515202902.461539-1-devnexen@xxxxxxxxx/
> [3] https://lore.kernel.org/linux-mm/20260428113059.79001-1-lance.yang@xxxxxxxxx/
[4] https://sashiko.dev/#/patchset/20260708-hugetlb-alloc-failure-fixes-v2-0-c7f27cbb462b%40google.com?part=2,