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

[Ackerley Tng]

Joshua Hahn <joshua.hahnjy@xxxxxxxxx> writes:

> */
> On Wed, 9 Sep 2026 15:54:59 -0700 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>> On Wed, 09 Sep 2026 14:49:28 -0700 Ackerley Tng via B4 Relay <devnull+ackerleytng.google.com@xxxxxxxxxx> wrote:
>>
>> > From: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>> >
>> > When folio allocation fails early (e.g. buddy allocation failure or
>> > cgroup charging failure) and a reservation was not used (meaning an
>> > unreserved global page was needed), the subpool page acquired during the
>> > allocation attempt must still be returned.
>> >
>> > Currently, the subpool cleanup error path only returns the page to the
>> > subpool if a reservation was used. If no reservation was used, it skips
>> > releasing the page back to the subpool, permanently leaking the subpool's
>> > used pages counter.
>> >
>> > With subpools now always tracking used pages, always release the page
>> > back to the subpool whenever a subpool page was acquired.
>> >
>> > Opportunistically rename the local variables tracking global reservations
>> > needed and global reservations returned. This clarifies the accounting:
>> > a value of zero for needed global reservations indicates an existing
>> > reservation satisfies the allocation, while a non-zero value indicates
>> > new global pages are required.
>> >
>> > Adjust global reservations using the difference between reservations
>> > needed and reservations returned to properly handle races where concurrent
>> > threads interact with the same subpool.
>>
>> This one conflicts with "mm/hugetlb: fix max-only subpool accounting on
>> alloc_hugetlb_folio failure".
>> https://lore.kernel.org/20260428113037.88766-2-enderaoelyther@xxxxxxxxx.
>>
>>
>> > @@ -3074,13 +3074,10 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
>> >  	return folio;
>> >
>> >  out_subpool_put:
>> > -	/*
>> > -	 * put page to subpool iff the quota of subpool's rsv_hpages is used
>> > -	 * during hugepage_subpool_get_pages.
>> > -	 */
>> > -	if (map_chg && !gbl_chg) {
>> > -		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
>> > -		hugetlb_acct_memory(h, -gbl_reserve);
>> > +	if (map_chg) {
>> > +		long gbl_resv_put = hugepage_subpool_put_pages(spool, 1);
>> > +
>> > +		hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put);
>> >  	}
>> >
>> >  out_end_reservation:
>>
>> Here.
>>
>> I presently have Zhao Li's patch staged in mm.git's
>> mm-hotfixes-unstable branch, awaiting review.
>>
>> What to do?

My vote is to drop [2] from Zhao and switch over to this series.

>
> Hi Andrew, I hope you are having a good day!
>
> Yeah, I also noticed this too [1]. There are actually 4 proposed
> solutions for this in the mailing list which is a good indication that
> something is wrong here : -) But unfortunately it seems like there was
> no resolution the last time I pointed this out : -(
>
> I CC-ed all the authors previously on that chain but wasn't able to
> get a response, maybe we'll give it another shot here?
>
> Ackerley, Zhao, David, Song, hopefully we can align here.
>

[2] from Zhao could be one of the components to get -stable out of
trouble as Andrew requested at [3], but I think small changes on -stable
leave lots of gaps.

[2] does not address these issues with alloc_hugetlb_folio():

Firstly, there's a condition in the fix checking that min_hpages is not
requested and max_hpages is requested. That looks like if min_hpages
were requested, used_hpages wouldn't be restored even if it was being
tracked.

I could be wrong about the above, and we could also iterate on [2] to
cover the edge cases, but I think [2] would then just become bigger and
harder to understand.

Beyond that, [2] relies on local variables in alloc_hugetlb_folio() to
determine whether to update subpool tracking (a global state), so if
there's any kind of race between threads doing subpool get/put, any
local variable state would be updating the global subpool state wrongly.

> Personally I'm partial to Ackerley's approach here since I think the
> get/put semantics that he proposes is the easiest to understand for me.
>

I think it's hard to use smaller changes to fix the global state
changes, I'd prefer not digging to find all the edge cases in favor of a
cleaner fix.

I'm not super familiar with upstream backports to stable, but I'd rather
backport an understandable fix than more open-coding.

(Understandable is subjective, of course, I'm biased towards the fix
here.)

> It would be great if we could sort this out since I think it's been
> a while since we've had all of these proposals in flight.
>
> Thank you everyone!
> Joshua
>
> [1] https://lore.kernel.org/all/20260713144557.3845941-1-joshua.hahnjy@xxxxxxxxx/
[2] https://lore.kernel.org/20260428113037.88766-2-enderaoelyther@xxxxxxxxx
[3] https://lore.kernel.org/all/20260909155003.e6c2f4fe3c0bbee3b34578fd@xxxxxxxxxxxxxxxxxxxx/