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

[Ackerley Tng]

Karl Mehltretter <kmehltretter@xxxxxxxxx> writes:

> On Wed, 16 Sep 2026 16:39:03 -0700, Ackerley Tng wrote:
>>  out_subpool_put:
>> +	if (map_chg) {
>> +		long gbl_resv_put = hugepage_subpool_put_pages(spool, 1);
>> +
>> +		hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put);
>>  	}
>
> I reproduced the same fallible-positive-adjustment problem in this
> calculation.
>
> The complete subpool put is locally correct, but it can expose capacity
> which another operation consumes before this call tries to restore the
> corresponding global reservation. A positive hugetlb_acct_memory() can
> then fail with -ENOMEM, and its return value is ignored.
>
> I tested the exact patch prefixes on v7.3-rc3 in x86-64 QEMU with 2 MiB
> huge pages, at both one and four vCPUs. A default-off test hook enforced
> this ordering:
>
> 1. A shared MAP_NORESERVE fault acquires one page from a four-page
>    minimum subpool while the global pool has no spare capacity.
> 2. Folio allocation fails.
> 3. Before rollback, an existing reservation is released and a competing
>    reservation consumes that newly available capacity.
> 4. The subpool put restores the local minimum state, but its required +1
>    global correction fails with -ENOMEM.
>
> Both CPU counts produced the same result:
>
>   Source state  Cleanup result  After file removal / after unmount
>   ------------  --------------  ---------------------------------
>   v7.3-rc3      old cleanup     4 / 0
>   patches 1-2   old local leak  3 / 3
>   patches 1-3   +1, -ENOMEM     3 / ULONG_MAX
>   patches 1-4   +1, -ENOMEM     3 / ULONG_MAX
>
> The expected values are 4 after file removal and 0 after unmount. Patch 3
> removes the local usage leak, but the failed positive correction replaces
> it with globally unbacked subpool reservations and the unmount underflow.
> Patch 4 does not cover this earlier allocation-failure path.
>
> As in the reservation case, the base remains balanced in this controlled
> interleaving. Patch 1 makes the path observable on the minimum-only mount,
> and patch 3 changes the local leak into globally unbacked reservations.
>
> As for 2/4, I think the subpool get must remain provisional until the
> allocation commits or aborts.
>
> A LLM agent helped me with the tests.
>
> Thanks,
> Karl

This is confusing me. Perhaps I've been reading too much LLM output from
myself and from Karl :)


== We have a path forward on top of Zhao and Jinmeng's patches

I tried to integrate Zhao [1] and Jinmeng's [2] patches into a v4 of
this series, and I have path forward for my fixes on top of Zhao and
Jinmeng's patches.

== The path forward requires some small changes to [1/4] of this series

With Zhao and Jinmeng's patches, the necessary subpool tracking is
basically done outside of hugepage_subpool_put_pages() in 2 places
(alloc_hugetlb_folio() and hugetlb_reserve_pages()).

The logic is that patch [1/4] of this series always tracks used_hpages
in the subpool, so I can update those 2 places that Zhao and Jinmeng
updated in alloc_hugetlb_folio() and hugetlb_reserve_pages() to also
always update used_hpages.

I can update those 2 places if we end up going with always tracking
used_hpages in the subpool, which I still believe is a good cleanup.

== But I found more race-related issues

I thought [2/4] and [3/4] of this series would fix all the race-related
issues, but my LLM produced a reproducer that revealed even more
race-related bugs which I don't fully understand.

== Questions for Karl

(Independent of the "Path forward for 7.3/7.4")

1. Does squashing patch [4/4] of this series into [2/4] [3/4] resolve
   any of what you found?
2. Does patch [1/4] introduce new issues, or does it just not fully fix
   all the issues? At this point I think we have so many bugs, it's more
   about fixing bugs progressively (not regressing) than finding a
   complete fix.
3. Would it be ok if you integrate your findings across your two replies
   on [2/4] and [3/4]? They're similar yet slightly different so it's
   kind of confusing. If you have reproducers, it would help to share
   them! :)

== Path forward for 7.3/7.4

Andrew, I think we should merge Zhao and Jinmeng's patches. They
separately fix different issues, not completely, but is good enough of
an improvement given this bigger-than-expected mess.

I want to continue iterating on this series but I'm a little busy over
the next few weeks (LPC) and I don't want to hold things up.

I believe the relative order between those 2 doesn't matter, but I
tested this order: Zhao then Jinmeng. I'll reply to those patches with
Tested-by tags.


Thank you everyone!