Re: [PATCH v3 0/4] Fix HugeTLB subpool used_hpages tracking

Andrew Morton <[email protected]> writes:

> On Wed, 16 Sep 2026 16:39:00 -0700 Ackerley Tng via B4 Relay <[email protected]> wrote:
>
>> HugeTLB subpools currently only track used pages (used_hpages) when a
>> maximum size limit (max_hpages) is configured on the mount.
>>
>> ...
>>
>> This series addresses three problem areas resulting from untracked usage
>> and asymmetric error handling:
>
> These are rather serious-sounding problems.
>
> I'm seeing Fixes: for 10-year-old patches, for one last year and one
> this year.  The fourth patch doesn't identify what to fix.
>
> So if downstream people (-stable maintainers, others) follow our
> recommendations, some kernels will get two of these patches, other
> kernel versions will get three and some lucky kernels might get all
> four.  Are you confident that the patches can be split apart in this
> fashion and still produce a good result?  After all, we're sending
> downstream some patch combinations which we didn't test.
>
> Also, can you speculate about why these things took so long to
> discover?
>

I wouldn't have noticed these if Sashiko didn't point out these
pre-existing issues while I was trying to refactor alloc_hugetlb_folio()
in the earlier patch series.

Here are my guesses:

> 1. False restoration and premature subpool freeing (Patch 1):
>     + Global reservations (resv_huge_pages) are not decremented when
>       reservations are released, depleting available huge pages host-wide.

This requires min_size to be requested and NOT max_size, and it needs
an awkward partial truncation of the file.

>     + On unmount, subpool_is_free() checks (rsv_hpages == min_hpages).
>       Because of the false reservation restoration, this evaluates to true,
>       freeing the subpool while folios are still active in the page cache
>       and causing a use-after-free when they are later released.
>       (See Patch 1 for a detailed step-by-step trace.)

Unmounting masks the issue, because when the subpool is freed,
spool->rsv_hpages gets returned to the global hstate, restoring the
global state. The use-after-free doesn't actually happen unless a folio
outlives the subpool (not common), since free_huge_folio() is the one
that uses after the subpool is freed.

> 2. Reservation rollback races and underflow (Patches 2 & 4):
>     + On hugetlb_acct_memory() failure in hugetlb_reserve_pages(), the
>       error path manually calculates rollbacks using local variables.
>       This is race-prone when concurrent threads interact with the
>       subpool, risking reservation leaks or counter underflows.

I didn't reproduce the race here that could mess up global state, and
races probably hid this. I guess the messing up could go both ways so
perhaps on a machine with heavy load, future races offset the old error?

>     + On region_add() failure, prematurely releasing global reservations
>       forces error cleanup to attempt fresh allocations that can fail
>       under memory pressure.

I didn't write a reproducer for this one, but it's an edge case (memory
pressure causing huge page allocations to fail) of an edge case (OOM on
trying to allocate an entry for the resv_map). It might occur together
if the host is completely out of memory, but I think in that case people
worry more about the system OOM than fixing a HugeTLB bug.

> 3. Subpool usage leak on allocation failure (Patch 3):
>     + In alloc_hugetlb_folio(), when folio allocation fails and
>       gbl_chg == 1, the cleanup path skips hugepage_subpool_put_pages(),
>       permanently leaking used_hpages.

This is a failure path, which probably isn't executed that often. The
reproducer created a very small mount and was specially written to
trigger this edge case.

Overall, I was also second guessing myself: perhaps it was never the
intent to manage reservations by tracking used_hpages throughout, but in
09a95e29cb30 ("mm/hugetlb: optimize minimum size (min_size) accounting")
Mike noted that min size accounting "does not take into account used
pages within the filesystem", and the patch added a check for
used_hpages. Based on that, and the _original_ code documentation below:

	long used_hpages;	/* Used count against maximum, includes */
				/* both allocated and reserved pages. */

I think the original intent was for used_hpages to track both allocated
and reserved pages, and patch 1 restores the original intent, and also
simplifies the logic in many places.

> Ho hum, complicated.  Let's at least slap a Fixes: on [4/4].  My
> trillion-dollar sidekick suggests a9e443086489 ("hugetlb: disable
> region_add file_region coalescing").
>

Your trillion-dollar sidekick got the right patch but the wrong hash :)

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

I'll also reply to the last patch with the above. If there are no
further revisions required, could you please add the above tag in for
me?

> Our other little friend is talking to us:
> 	https://sashiko.dev/#/patchset/20260916-hugetlb-subpool-always-track-used-v3-0-38aae9b5ccdd@google.com
>
> I'd like to queue this up promptly because it fixes things.  But let's
> seek additional review input and argue with Sashiko before taking that
> step.

I'll copy Sashiko's comments onto this patch series in replies. :)