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

[Lorenzo Stoakes (ARM)]

On Sun, Aug 30, 2026 at 03:07:25PM +0100, Pedro Falcato wrote:
> On Sun, Aug 30, 2026 at 09:47:56AM -0300, Guilherme Giacomo Simoes wrote:
> > Matthew Wilcox <willy@xxxxxxxxxxxxx> wrotes:
> > > 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.

Yep in dontunmap_complete() if a source VMA is left in place due to
MREMAP_DONTUNMAP being set (which has caused some fun lately), but that does
require a VMA write lock.
>
> > > ... at least I don't think it is.  Lorenzo, could you check me on this?

As above :>)

> > > 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.

The forking logic holds the write lock anyway.

> >
> > >
> > > 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.)

Yup, page_table_lock is taken explicitly to serialise this. See
https://docs.kernel.org/mm/process_addrs.html

>
> 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.

Yes.

> 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

Confirmed (manually, probably should have got an LLM to do it ;):

All hold PTL:

Huge TLB (yuck) cases with 'huge' PTL:

copy_hugetlb_page_range()
  -> hugetlb_install_folio()
   -> hugetlb_add_new_anon_rmap()
     -> __folio_set_anon()
hugetlb_wp()
  -> hugetlb_add_new_anon_rmap()
    -> __folio_set_anon()
hugetlb_no_page()
  -> hugetlb_add_new_anon_rmap()
    -> __folio_set_anon()
hugetlb_mfill_atomic_pte()
  -> hugetlb_add_new_anon_rmap()
    -> __folio_set_anon()

THP using the PMD PTL:

__do_huge_pmd_anonymous_page()
  -> map_anon_folio_pmd_pf()
    -> map_anon_folio_pmd_nopf()
      -> folio_add_new_anon_rmap()
        -> __folio_set_anon()
do_huge_zero_wp_pmd()
  -> map_anon_folio_pmd_pf()
    -> map_anon_folio_pmd_nopf()
      -> folio_add_new_anon_rmap()
        -> __folio_set_anon()
collapse_huge_page()
  -> map_anon_folio_pmd_nopf()
    -> folio_add_new_anon_rmap()
      -> __folio_set_anon()

The rest all hold the PTE PTL:

copy_pte_range()
  -> copy_present_ptes()
    -> copy_present_page()
      -> folio_add_new_anon_rmap()
        -> __folio_set_anon()

wp_page_copy()
  -> folio_add_new_anon_rmap()
    -> __folio_set_anon()

do_swap_page()
  -> folio_add_new_anon_rmap()
    -> __folio_set_anon()

do_anonymous_page()
  -> map_anon_folio_pte_pf()
    -> map_anon_folio_pte_nopf()
      -> folio_add_new_anon_rmap()
        -> __folio_set_anon()

collapse_huge_page() [fallback path]
  -> map_anon_folio_pte_nopf()
    -> folio_add_new_anon_rmap()
      -> __folio_set_anon()

filemap_map_pages()
  -> filemap_map_folio_range()
    -> set_pte_range()
      -> folio_add_new_anon_rmap()
        -> __folio_set_anon()

filemap_map_pages()
  -> filemap_map_order0_folio()
    -> set_pte_range()
      -> folio_add_new_anon_rmap()
        -> __folio_set_anon()

finish_fault()
  -> set_pte_range()
    -> folio_add_new_anon_rmap()
      -> __folio_set_anon()

unuse_pte()
  -> folio_add_new_anon_rmap()
    -> __folio_set_anon()

mfill_atomic_install_pte()
  -> folio_add_new_anon_rmap()
    -> __folio_set_anon()

>    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).

Yup.

>
> 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]

I don't think you'd ever need to know for PUD installation?

In any case you are always serialised through one lock or another with
acquire/release semantics AFAICT so I don't think there's an issue here,
and if there were one we'd have encountered it by now :)

>
> >
> > >
> > > 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.

Yeah nor odo I.

> > >
> > > 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.

I'd prefer vma_is_faulted(). That'll align better with my scalable CoW work
also.

Anyway I agree with Pedro that a data_race() resolution is appropriate here
rather than an unnecessary READ_ONCE()/WRITE_ONCE() pair.

>
> --
> Pedro

--
Cheers, Lorenzo