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

[Joshua Hahn]

On Thu, 9 Jul 2026 15:15:50 -0700 Ackerley Tng <ackerleytng@xxxxxxxxxx> wrote:

> 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!
> >

Hi Ackerley, sorry that the reply took a while : -(

> 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())

Yeah... this kind of tripped me up.

> 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?

This seems like quite a reasonable approach. It's new information that
we can't derive from the existing flags we have.

>     <<== 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.

I think I like the semantic change, since it does make the code a lot
more obvious on why and where we are making the accounting changes.
One fear that I have is that this churn could introduce some new
subtle accounting errors, since we have been fixing this area for
quite some time now. Let's audit very carefully to make sure that
there are no new accounting errors that will be introduced : -)

> 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?

Yup I think so. You might be more familiar with the new usecase you
are adding as part of the guest_memfd, but I have not really seen a
usecase for hugeTLB that would require userspace to do many hugeTLB
allocations (that THP wouldn't be able to serve). Having this accuracy
seems to be much more important than the rare contention we will see.

Thanks again for the fixes and ideas Ackerley! Looking forward to your
new approach : -)
Joshua

> >>
> >> [...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,