Re: [PATCH 2/3] mm: hugetlb: Fix out_put_pages subpool reserve calculation

[Ackerley Tng]

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

Copying Sashiko's comments here for discussion :)

> From: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>
> When reserving pages for a mapping fails during global accounting, the
> error path rolls back the adjustments made to the subpool.
>
> Currently, this rollback was performed in two separate steps:
>
> 1. Returning only the portion of reservations originally satisfied from the
>    subpool
> 2. Separately adjusting the subpool used pages counter for the portion that
>    was requested from the global pool.
>
> In (1.), because the used pages counter had not yet been decremented for
> the global portion, the subpool observed an inflated used pages count. If
> the mount was configured with both a minimum size and a maximum size, this
> inflated count prevented the subpool from recognizing that usage fell below
> the minimum size guarantee.
>
> As a result, the subpool failed to restore its reserved pages counter and
> instead returned that a global reservation should be dropped. The
> mount-time reservation is permanently destroyed, leaving global reservation
> counts depleted and causing an underflow when the filesystem is eventually
> unmounted.
>
> Additionally, if concurrent threads modified subpool usage during the
> reservation attempt, calculating the rollback amount using stale local
> variables could cause global reservation counts to diverge.
>
> Now that used pages are always tracked within the subpool, return the
> entire requested page count to the subpool in a single call. Global
> reservations are then adjusted using the difference between the
> reservations originally requested and those returned, fixing the issues
> described above.
>
> Fixes: 1d3f9bb4c8af ("mm/hugetlb: restore failed global reservations to subpool")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> ---
>  mm/hugetlb.c | 49 +++++++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 66b6601abb069..d84ef044d1346 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6674,12 +6674,14 @@ long hugetlb_reserve_pages(struct inode *inode,
>  		struct vm_area_struct *vma,
>  		vma_flags_t vma_flags)
>  {
> -	long chg = -1, add = -1, spool_resv, gbl_resv;
> +	long chg = -1, add = -1;
>  	struct hstate *h = hstate_inode(inode);
>  	struct hugepage_subpool *spool = subpool_inode(inode);
>  	struct resv_map *resv_map;
>  	struct hugetlb_cgroup *h_cg = NULL;
> -	long gbl_reserve, regions_needed = 0;
> +	long regions_needed = 0;
> +	long gbl_resv_get;
> +	long gbl_resv_put;
>  	int err;
>
>  	/* This should never happen */
> @@ -6754,9 +6756,9 @@ long hugetlb_reserve_pages(struct inode *inode,
>  	 * the subpool has a minimum size, there may be some global
>  	 * reservations already in place (gbl_reserve).
>  	 */
> -	gbl_reserve = hugepage_subpool_get_pages(spool, chg);
> -	if (gbl_reserve < 0) {
> -		err = gbl_reserve;
> +	gbl_resv_get = hugepage_subpool_get_pages(spool, chg);
> +	if (gbl_resv_get < 0) {
> +		err = gbl_resv_get;
>  		goto out_uncharge_cgroup;
>  	}
>
> @@ -6764,7 +6766,7 @@ long hugetlb_reserve_pages(struct inode *inode,
>  	 * Check enough hugepages are available for the reservation.
>  	 * Hand the pages back to the subpool if there are not
>  	 */
> -	err = hugetlb_acct_memory(h, gbl_reserve);
> +	err = hugetlb_acct_memory(h, gbl_resv_get);

[GLOBAL ACCOUNTING]

>  	if (err < 0)
>  		goto out_put_pages;
>
> @@ -6783,7 +6785,7 @@ 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_reserve);
> +			hugetlb_acct_memory(h, -gbl_resv_get);

Sashiko:

Does this error path complicate the rollback logic? By explicitly freeing the
previously held global reservations here, it guarantees that out_put_pages
will need to re-allocate them from scratch if the subpool absorbs them. Does
this increase the likelihood of the subsequent hugetlb_acct_memory() allocation
failing?

Ackerley:

Yes, it does. Restoring gbl_resv_get here is the correct thing to do, to
undo the [GLOBAL ACCOUNTING] (see above). It does look weird that
hugetlb_acct_memory() is called twice, but the hugetlb_acct_memory() in
out_put_pages is actually purely meant to follow up after
hugetlb_subpool_put_pages().

Everything is mathematically correct. Sashiko is also right that this
could result in allocation within out_put_pages.

I'll add another patch to clean this up, see next revision!

>  			err = add;
>  			goto out_put_pages;
>  		} else if (unlikely(chg > add)) {
> @@ -6819,26 +6821,21 @@ long hugetlb_reserve_pages(struct inode *inode,
>  	}
>  	return chg;
>
> -out_put_pages:
> -	spool_resv = chg - gbl_reserve;
> -	if (spool_resv) {
> -		/* put sub pool's reservation back, chg - gbl_reserve */
> -		gbl_resv = hugepage_subpool_put_pages(spool, spool_resv);
> -		/*
> -		 * subpool's reserved pages can not be put back due to race,
> -		 * return to hstate.
> -		 */
> -		hugetlb_acct_memory(h, -gbl_resv);
> -	}
> -	/* Restore used_hpages for pages that failed global reservation */
> -	if (gbl_reserve && spool) {
> -		unsigned long flags;
> + out_put_pages:
> +	/*
> +	 * Return all that was requested from the subpool, let subpool
> +	 * tell us the new number of reservations that need to be
> +	 * returned to the global pool.
> +	 */
> +	gbl_resv_put = hugepage_subpool_put_pages(spool, chg);
> +	/*
> +	 * 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.
> +	 */
> +	hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put);

Sashiko:

Does ignoring the return value of hugetlb_acct_memory() here lead to accounting
divergence? If gbl_resv_get > gbl_resv_put, this attempts to allocate global
pages. If this allocation fails (e.g. out of memory), the failure is silently
ignored.

Could this lead to the global resv_huge_pages counter diverging from the
subpool's rsv_hpages counter? If a concurrent operation drops the subpool's
used_hpages below min_hpages, hugepage_subpool_put_pages() would absorb the
returned pages into rsv_hpages to satisfy the minimum size constraint. If the
re-allocation then fails and the subpool is later unmounted, could this
divergence cause resv_huge_pages to underflow, inflating available_huge_pages
and potentially enabling a system-wide OOM denial of service?

Ackerley:

Yes I agree that ignoring the return value of hugepage_acct_memory()
could result in issues, but this isn't unique to this code path. The
return value of hugepage_acct_memory() is ignored in similar cleanup
paths.

>
> -		spin_lock_irqsave(&spool->lock, flags);
> -		if (spool->max_hpages != -1)
> -			spool->used_hpages -= gbl_reserve;
> -		unlock_or_release_subpool(spool, flags);
> -	}
>  out_uncharge_cgroup:
>  	hugetlb_cgroup_uncharge_cgroup_rsvd(hstate_index(h),
>  					    chg * pages_per_huge_page(h), h_cg);
>
> --
> 2.55.0.970.g62bdec98f9-goog