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

[Ackerley Tng]

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!

> Thanks again. I hope you have a great day!!!
> Joshua
>
>> Fixes: a833a693a490e ("mm: hugetlb: fix incorrect fallback for subpool")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>> ---
>>  mm/hugetlb.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index ee5e99c1894b9..4093c1c0a4a1d 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -2852,7 +2852,7 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
>>  	struct hugepage_subpool *spool = subpool_vma(vma);
>>  	struct hstate *h = hstate_vma(vma);
>>  	struct folio *folio;
>> -	long retval, gbl_chg, gbl_reserve;
>> +	long retval, gbl_chg;
>>  	map_chg_state map_chg;
>>  	int ret, idx;
>>  	struct hugetlb_cgroup *h_cg = NULL;
>> @@ -3003,13 +3003,11 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
>>  		hugetlb_cgroup_uncharge_cgroup_rsvd(idx, pages_per_huge_page(h),
>>  						    h_cg_rsvd);
>>  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_reserve = hugepage_subpool_put_pages(spool, 1);
>> +
>> +		if (!gbl_chg)
>> +			hugetlb_acct_memory(h, -gbl_reserve);
>>  	}
>
> [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/