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

[Joshua Hahn]

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.

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

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/