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

[Ackerley Tng]

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

>
> [...snip...]
>
> @@ -3072,13 +3072,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);


Sashiko:

Does this violate the subpool rollback protocol in alloc_hugetlb_folio()?

The error path appears to unconditionally pass the full requested amount (1)
to hugepage_subpool_put_pages() instead of the absorbed amount. According
to the subsystem guidelines, error paths should return the difference
between the change and the global reserve to hugepage_subpool_put_pages(),
not the full requested amount.

Ackerley:

Nope, the subpool rollback protocol has changed since the first patch in
this series, where now the subpool always tracks used_hpages. After that
change, pages taken from the subpool should always be returned to the
subpool. The return value from hugepage_subpool_put_pages() can then be
used in hugetlb_acct_memory() to fix up global accounting, like in the
line below.

> +
> +		hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put);

Sashiko:

Can this result in silent state corruption and accounting desynchronization?

If alloc_hugetlb_folio() fails (e.g., due to memory pressure) after getting
1 page from the global pool (gbl_resv_get = 1), and concurrently another
thread frees a page to the same subpool dropping used_hpages below
min_hpages, hugepage_subpool_put_pages(spool, 1) will absorb the page into
rsv_hpages and return 0 (gbl_resv_put = 0).

This causes hugetlb_acct_memory(h, 1) to be called, which attempts to
gather a surplus page from the global pool. Since the system is under
memory pressure, this can fail and return -ENOMEM.

Because the return value of hugetlb_acct_memory() is ignored,
spool->rsv_hpages remains incremented without a corresponding credit to
the global pool, which can lead to an integer underflow in resv_huge_pages
and permanent memory management desynchronization.

Ackerley:

Sashiko made the same comment on patch 2 as well. This is a common
pattern on cleanup paths, where the return value is ignored.

I think the fix here would be as described in [1], to flip the
reservation tracking to track an available page count directly, but
that's for another patch series :)

[1] https://lore.kernel.org/all/CAEvNRgGN0HSJ2iLSDD2haSKOxifa-uhkO9Hwossh0+Q_d9fzOw@xxxxxxxxxxxxxx/

>  	}
>
>  out_end_reservation:
>
> --
> 2.55.0.970.g62bdec98f9-goog