Re: Bug: Performance regression in 1013af4f585f: mm/hugetlb: fix huge_pmd_unshare() vs GUP-fast race

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Oct 09, 2025 at 09:40:34AM +0200, David Hildenbrand wrote:
> On 01.09.25 12:58, Jann Horn wrote:
> > Hi!
> >
> > On Fri, Aug 29, 2025 at 4:30 PM Uschakow, Stanislav <suschako@amazon.de> wrote:
> > > We have observed a huge latency increase using `fork()` after ingesting the CVE-2025-38085 fix which leads to the commit `1013af4f585f: mm/hugetlb: fix huge_pmd_unshare() vs GUP-fast race`. On large machines with 1.5TB of memory with 196 cores, we identified mmapping of 1.2TB of shared memory and forking itself dozens or hundreds of times we see a increase of execution times of a factor of 4. The reproducer is at the end of the email.
> >
> > Yeah, every 1G virtual address range you unshare on unmap will do an
> > extra synchronous IPI broadcast to all CPU cores, so it's not very
> > surprising that doing this would be a bit slow on a machine with 196
> > cores.
> >
> > > My observation/assumption is:
> > >
> > > each child touches 100 random pages and despawns
> > > on each despawn `huge_pmd_unshare()` is called
> > > each call to `huge_pmd_unshare()` syncrhonizes all threads using `tlb_remove_table_sync_one()` leading to the regression
> >
> > Yeah, makes sense that that'd be slow.
> >
> > There are probably several ways this could be optimized - like maybe
> > changing tlb_remove_table_sync_one() to rely on the MM's cpumask
> > (though that would require thinking about whether this interacts with
> > remote MM access somehow), or batching the refcount drops for hugetlb
> > shared page tables through something like struct mmu_gather, or doing
> > something special for the unmap path, or changing the semantics of
> > hugetlb page tables such that they can never turn into normal page
> > tables again. However, I'm not planning to work on optimizing this.
>
> I'm currently looking at the fix and what sticks out is "Fix it with an
> explicit broadcast IPI through tlb_remove_table_sync_one()".
>
> (I don't understand how the page table can be used for "normal,
> non-hugetlb". I could only see how it is used for the remaining user for
> hugetlb stuff, but that's different question)

Right, this surely is related only to hugetlb PTS, otherwise the refcount
shouldn't be a factor no?

>
> How does the fix work when an architecture does not issue IPIs for TLB
> shootdown? To handle gup-fast on these architectures, we use RCU.
>
> So I'm wondering whether we use RCU somehow.

Presumably you mean whether we _can_ use RCU somehow?

>
> But note that in gup_fast_pte_range(), we are validating whether the PMD
> changed:
>
> if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) ||
>     unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) {
> 	gup_put_folio(folio, 1, flags);
> 	goto pte_unmap;
> }

Right and as per the comment there:

/*
 ...
 * For THP collapse, it's a bit more complicated because GUP-fast may be
 * walking a pgtable page that is being freed (pte is still valid but pmd
 * can be cleared already).  To avoid race in such condition, we need to
 * also check pmd here to make sure pmd doesn't change (corresponds to
 * pmdp_collapse_flush() in the THP collapse code path).
 ...
 */

So if this can correctly handle a cleared PMD entry in the teardown case, surely
it can handle it in this case also?

>
>
> So in case the page table got reused in the meantime, we should just back
> off and be fine, right?

Yeah seems to be the case to me.

>
> --
> Cheers
>
> David / dhildenb
>

So it seems like you have a proposal here - could you send a patch so we can
assess it please? :)

I'm guessing we need only consider the 'remaining user' case for hugetlb PTS
right? And perhaps stabilise via RCU somehow?

Cheers, Lorenzo