Re: [PATCH] mm: fix the race on huge alloc failed

[Pedro Falcato]

On Sun, Aug 30, 2026 at 09:47:56AM -0300, Guilherme Giacomo Simoes wrote:
> Matthew Wilcox <willy@xxxxxxxxxxxxx> wrotes:
> > > >> Fixes: 164b06f238b9 ("mm: call wp_page_copy() under the VMA lock")
> > > >
> > > > what makes you think this is the right commit for fixes?
> > > Maybe I would should analyzed this better. I only seed the commit that introduce
> > > this function (and consequently this reader)
> > 
> > That was what I thought, but it's not enough to determine if that's the
> > start of the problem.  Look, that commit does:
> > 
> > -       if (unlikely(anon_vma_prepare(vma)))
> > -               goto oom;
> > +       ret = vmf_anon_prepare(vmf);
> > +       if (unlikely(ret))
> > +               goto out;
> > 
> > ... and anon_vma_prepare() does:
> > 
> >         if (likely(vma->anon_vma))
> >                 return 0;
> > 
> > so either this race was already present in 164b06f238b9 (and you need to
> > go back further) or it was actually introduced later (maybe the write
> > side was introduced later?)
> 
> This commit introduce the vmf_anon_prepare():
> +static vm_fault_t vmf_anon_prepare(struct vm_fault *vmf)
> +{
> +       struct vm_area_struct *vma = vmf->vma;
> +
> +       if (likely(vma->anon_vma))
> +               return 0;
> +       if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
> +               vma_end_read(vma);
> +               return VM_FAULT_RETRY;
> +       }
> +       if (__anon_vma_prepare(vma))
> +               return VM_FAULT_OOM;
> +       return 0;
> +}
> 
> in commit 2a058ab3286d (mm: change vmf_anon_prepare() to __vmf_anon_prepare())
> the vmf_anon_prepare became __vmf_anon_prepare.
> Where the race problem occours `if (likely(vma->anon_vma))`...
> This commit 164b06f238b9 is introduced in 2023.
> 
> The write side is introduce in commit d5a187daf585 (mm, rmap: handle
> anon_vma_prepare() common case inline) in 2016.
> 
> > The important thing to know is that the mmap_lock is a read-write lock.
> > That means that two readers can be present at the same time.  So this race
> > can happen when both threads hold the mmap_lock.  I don't know whether
> > they do in the syzbot reproducer; probably not, but it doesn't matter.
> > 
> > The other important thing is that _we don't care_ what the value of
> > vma->anon_vma is.  We only care whether it's NULL or not (this is a
> > sufficiently common case that I wonder whether KCSAN shouldn't special-case
> > it and decline to monitor it ...)  VMAs are created with a NULL anon_vma,
> > and then if needed, anon_vma is set.  Once set, it is never changed (uhh

I believe it can be changed (IIRC on a mremap dontunmap edge case??), but
that needs the write lock anyway.

> > ... at least I don't think it is.  Lorenzo, could you check me on this?
> > I think all the places where we set vma->anon_vma to NULL are in
> > situations where the VMA is not yet exposed to the page fault handler,
> > like in the child side of fork()).
> But if I have a write in the same time, this can be a problem, even though if
> you only want to know if vma->anon_vma is NULL or not.
> 
> > 
> > So it's inappropriate to use READ_ONCE() / WRITE_ONCE() to "solve"
> > this problem, because we don't need those semantics.  It's sufficient
> > to wrap the read side in data_race() to indicate to KCSAN that we know
> > what we're doing.
> you sure? 
> 
> the __anon_vma_prepare(..) is write on vma->anon_vma and the
> __vmf_anon_prepare(..) is reade from the same vma->anon_vma at the same time,
> you sure that is not a problem? (I'm asking as a curious layperson.) 

99.9% sure. Here's the basic logic laid out:

1) Fault needs to fault in anonymous pages
2) Fault needs to possibly create an anon_vma
 2a) Thus it does the lockless check, where indeed we only
     care if it's non-null or not.
 2b) if the lockless check fails, we get into __anon_vma_prepare()
     logic, which crucially takes the page_table_lock to write the
     anon_vma to the vma. If it takes the lock and something is already
     there, it backs out.
3) Now, into the weeds of anon page faulting, we end up in __folio_set_anon(),
   which reads the anon_vma from vma. This function always (AFAIK?) runs with
   the PTE lock held. Thus we can be sure the anon_vma value is correct. In
   any case, we only need to have held the page table lock once in the fault
   for it to be valid; any change to its value from non-null to null needs
   the vma/mmap write lock. Because we take a bunch of locks and do a bunch of
   stuff between that initial check in __vmf_anon_prepare and this, the compiler
   cannot validly cache the load (which can, in theory, tear).

Now, for memory ordering and its wonderful transitive properties:
1) writing anon_vma takes the page_table_lock. therefore if you acquire
   page_table_lock, you obsreve the anon_vma store and all preceding stores
   (due to spin_unlock providing RELEASE semantics, and spin_lock providing
   ACQUIRE semantics)
2) say you install e.g a PUD entry, you take the page_table_lock. So you fully
   observe the anon_vma that was installed (by doing an ACQUIRE on the lock).
   you also issue a smp_wmb() which makes sure the ptdesc setup is visible.
3) others using that PUD entry will (should?) transitively observe everything
   you have observed, data-dependent loads will help you there. If we _ever_
   observe a page table without seeing an associated anon_vma, it's broken.

[Yes, I spent quite a bit of time thinking through this; it isn't trivial to prove
that 2->3 transition is correct, but it looks vaguely _handwavely_ correct]

> 
> > 
> > Also, as Lance said, I don't see how this is related to huge_page_alloc
> > failing.  All I see is two threads calling  __vmf_anon_prepare() at the
> > same time, which I presume is an attempt to COW a hugetlb page.
> > 
> > I don't think it's enough to just add a data_race() to this one read of
> > vma->anon_vma.  I think it's quite prevalent.  There's probably other
> > syzbot reports that mention it.

It sounds to me like the most cromulent solution is simply adding a

/* maybe vma_has_anon? */
static inline bool vma_has_anon_vma(const struct vm_area_struct *vma)
{
	return data_race(vma->anon_vma);
}

and churn everything to use it.

-- 
Pedro