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

[Ackerley Tng]

Ackerley Tng via B4 Relay <devnull+ackerleytng.google.com@xxxxxxxxxx>
writes:

> 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.
>
> Fix this by calling hugepage_subpool_put_pages() unconditionally if
> map_chg is true. Only call hugetlb_acct_memory() to adjust global
> reservations if !gbl_chg is true, to prevent underflowing the global
> resv_huge_pages counter.
>
> Fixes: a833a693a490e ("mm: hugetlb: fix incorrect fallback for subpool")

My bad. This isn't the right fix, I'm kind of re-introducing the bug
that a833a693a490e tried to fix. Let me think about a better fix.


This is what Sashiko had to say:

Can this unconditional call to hugepage_subpool_put_pages() lead to
resv_huge_pages underflowing?

If a hugetlbfs mount is configured with a minimum size but no maximum size
(so max_hpages == -1), spool->used_hpages remains 0. When the subpool's
reserve is exhausted, the next allocation will use a global page
(gbl_chg == 1).

If that allocation fails later in alloc_hugetlb_folio() (for example, hitting
a cgroup limit), we now reach this error path and call
hugepage_subpool_put_pages(spool, 1).

Because spool->used_hpages < spool->min_hpages, hugepage_subpool_put_pages()
will increment spool->rsv_hpages. However, because gbl_chg == 1, we skip
calling hugetlb_acct_memory().

This appears to leave the new subpool reservation without backing from the
global counter. When the successfully allocated pages are eventually freed,
won't resv_huge_pages be decremented out of balance, eventually underflowing
to ULONG_MAX and causing gather_surplus_pages() to exhaust system memory?

> 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 571212b80835e..e607dcbb2fc3e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2858,7 +2858,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;
> @@ -3009,13 +3009,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);
>  	}
>
>
>
> --
> 2.55.0.795.g602f6c329a-goog