Re: [PATCH v3 4/4] mm: hugetlb: Avoid re-allocating global reservations on region add failure

Ackerley Tng via B4 Relay <[email protected]>
writes:

> From: Ackerley Tng <[email protected]>
>
> When reserving huge pages for a shared mapping, reservations are first
> requested from the subpool, and any remainder is accounted in global
> reservations. When adding the file region entries fails later in the
> process, the reservation attempt must be rolled back.
>
> Previously, this error path explicitly dropped the global reservations
> that were just acquired before jumping to the cleanup label. The cleanup
> label then returned the pages to the subpool. If concurrent activity in
> the subpool allowed the subpool to absorb more reservations upon return
> than it supplied initially, the cleanup label calculated a positive
> difference and attempted to allocate new global reservations from scratch.
>
> This premature release was completely unnecessary because all requested
> pages were already backed globally: partly by the mount guarantee and
> partly by the global reservations just acquired. Prematurely dissolving
> those reservations forced the cleanup path to attempt fresh buddy
> allocations that could fail under memory pressure.
>
> Instead, track the number of global reservations actually accounted so
> far. In the cleanup label, subtract the already-accounted amount from the
> difference between requested and returned reservations. This ensures
> that when global reservations were already acquired, the adjustment is
> purely non-positive, dropping excess reservations without ever attempting
> fresh allocations.
>

If there are no other issues, Andrew could you please add this for me?

Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")

> Signed-off-by: Ackerley Tng <[email protected]>
> Cc: [email protected]
> ---
>  mm/hugetlb.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 6589b188cf657..ee1ba9ded0ec7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6678,6 +6678,7 @@ long hugetlb_reserve_pages(struct inode *inode,
>  	struct hugepage_subpool *spool = subpool_inode(inode);
>  	struct resv_map *resv_map;
>  	struct hugetlb_cgroup *h_cg = NULL;
> +	long gbl_resv_accounted = 0;
>  	long regions_needed = 0;
>  	long gbl_resv_get;
>  	long gbl_resv_put;
> @@ -6768,6 +6769,7 @@ long hugetlb_reserve_pages(struct inode *inode,
>  	err = hugetlb_acct_memory(h, gbl_resv_get);
>  	if (err < 0)
>  		goto out_put_pages;
> +	gbl_resv_accounted = gbl_resv_get;
>
>  	/*
>  	 * Account for the reservations made. Shared mappings record regions
> @@ -6784,7 +6786,6 @@ long hugetlb_reserve_pages(struct inode *inode,
>  		add = region_add(resv_map, from, to, regions_needed, h, h_cg);
>
>  		if (unlikely(add < 0)) {
> -			hugetlb_acct_memory(h, -gbl_resv_get);
>  			err = add;
>  			goto out_put_pages;
>  		} else if (unlikely(chg > add)) {
> @@ -6826,9 +6827,10 @@ long hugetlb_reserve_pages(struct inode *inode,
>  	 * There may be a difference between the number of
>  	 * reservations to consume and the number to restore now if
>  	 * there are multiple threads interacting with the subpool -
> -	 * restore the difference.
> +	 * restore the difference, taking into account any global
> +	 * reservations already acquired.
>  	 */
> -	hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put);
> +	hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put - gbl_resv_accounted);
>

Sashiko:

This is a pre-existing issue, but does this error path ignore the return
value of a potentially positive allocation request?

If hugepage_subpool_get_pages() returns gbl_resv_get > 0, and the initial
hugetlb_acct_memory(h, gbl_resv_get) fails with -ENOMEM, the code jumps
to out_put_pages where gbl_resv_accounted remains 0.

When hugepage_subpool_put_pages(spool, chg) is called just before this
block, concurrent thread activity freeing subpool pages could replenish
rsv_hpages to meet min_hpages, returning gbl_resv_put < gbl_resv_get.

Then hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put - 0) is called
with a positive delta, attempting a fresh global allocation. If this
allocation fails with -ENOMEM, the return value is ignored.

Could this permanently over-credit the subpool's rsv_hpages with global
reservations that were never actually acquired, bypassing the global
pool check later and causing resv_huge_pages to underflow? Unprivileged
userspace repeatedly mmap-ing and unmapping hugetlb regions under memory
pressure might trigger this.

Ackerley:

Summary here is that if hugetlb_acct_memory() fails, since the return
value is ignored, there could be unfixed global issues.

Sashiko also pointed this same thing out on patch 2 and 3.

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@mail.gmail.com/

>  out_uncharge_cgroup:
>  	hugetlb_cgroup_uncharge_cgroup_rsvd(hstate_index(h),
>
> --
> 2.55.0.1082.g2b9226bbc0-goog