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

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

[Guilherme Giacomo Simoes]

The race occurs because the reader (__vmf_anon_prepare()) checks
`vma->anon->vma` without holding the mmap_lock and withou the
READ_ONCE() macro. Since the writer (__anon_vma_prepare()) is holding the
mmap_lock and updating the pointer, it creates a data race as the two
access are not properly synchronized.

Use READ_ONCE() on the reader side and WRITE_ONCE() on the writer side
to tell to compiler treat these memory access carefully and not to
optimize them leading to inconsistent read.

Reported-by: syzbot+395b7abe9696862fc188@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=395b7abe9696862fc188
Fixes: 164b06f238b9 ("mm: call wp_page_copy() under the VMA lock")
Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
 mm/memory.c | 2 +-
 mm/rmap.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 6b8280cfc1db..33c1fbd30cdd 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3820,7 +3820,7 @@ vm_fault_t __vmf_anon_prepare(struct vm_fault *vmf)
 	struct vm_area_struct *vma = vmf->vma;
 	vm_fault_t ret = 0;
 
-	if (likely(vma->anon_vma))
+	if (likely(READ_ONCE(vma->anon_vma)))
 		return 0;
 	if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
 		if (!mmap_read_trylock(vma->vm_mm))
diff --git a/mm/rmap.c b/mm/rmap.c
index 1c77d5dc06e9..9d64d776b8c5 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -209,7 +209,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
 	/* page_table_lock to protect against threads */
 	spin_lock(&mm->page_table_lock);
 	if (likely(!vma->anon_vma)) {
-		vma->anon_vma = anon_vma;
+		WRITE_ONCE(vma->anon_vma, anon_vma);
 		anon_vma_chain_assign(vma, avc, anon_vma);
 		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
 		anon_vma->num_active_vmas++;
-- 
2.52.0

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

[Matthew Wilcox]

On Sat, Aug 29, 2026 at 07:00:34AM -0300, Guilherme Giacomo Simoes wrote:
> The race occurs because the reader (__vmf_anon_prepare()) checks
> `vma->anon->vma` without holding the mmap_lock and withou the
> READ_ONCE() macro. Since the writer (__anon_vma_prepare()) is holding the
> mmap_lock and updating the pointer, it creates a data race as the two
> access are not properly synchronized.
> 
> Use READ_ONCE() on the reader side and WRITE_ONCE() on the writer side
> to tell to compiler treat these memory access carefully and not to
> optimize them leading to inconsistent read.
> 
> Reported-by: syzbot+395b7abe9696862fc188@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=395b7abe9696862fc188
> Fixes: 164b06f238b9 ("mm: call wp_page_copy() under the VMA lock")

what makes you think this is the right commit for fixes?

> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> ---
>  mm/memory.c | 2 +-
>  mm/rmap.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 6b8280cfc1db..33c1fbd30cdd 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3820,7 +3820,7 @@ vm_fault_t __vmf_anon_prepare(struct vm_fault *vmf)
>  	struct vm_area_struct *vma = vmf->vma;
>  	vm_fault_t ret = 0;
>  
> -	if (likely(vma->anon_vma))
> +	if (likely(READ_ONCE(vma->anon_vma)))
>  		return 0;
>  	if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
>  		if (!mmap_read_trylock(vma->vm_mm))
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 1c77d5dc06e9..9d64d776b8c5 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -209,7 +209,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
>  	/* page_table_lock to protect against threads */
>  	spin_lock(&mm->page_table_lock);
>  	if (likely(!vma->anon_vma)) {
> -		vma->anon_vma = anon_vma;
> +		WRITE_ONCE(vma->anon_vma, anon_vma);
>  		anon_vma_chain_assign(vma, avc, anon_vma);
>  		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
>  		anon_vma->num_active_vmas++;
> -- 
> 2.52.0
> 
> 

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

[Matthew Wilcox]

On Sat, Aug 29, 2026 at 07:00:34AM -0300, Guilherme Giacomo Simoes wrote:
> The race occurs because the reader (__vmf_anon_prepare()) checks
> `vma->anon->vma` without holding the mmap_lock and withou the
> READ_ONCE() macro. Since the writer (__anon_vma_prepare()) is holding the
> mmap_lock and updating the pointer, it creates a data race as the two
> access are not properly synchronized.

also this explanation is bogus.  i don't have time to fix it right now.

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

[Guilherme Giacomo Simoes]

Matthew Wilcox <willy@infradead.org> 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)

>> The race occurs because the reader (__vmf_anon_prepare()) checks
>> `vma->anon->vma` without holding the mmap_lock and withou the
>> READ_ONCE() macro. Since the writer (__anon_vma_prepare()) is holding the
>> mmap_lock and updating the pointer, it creates a data race as the two
>> access are not properly synchronized.
>
> also this explanation is bogus.  i don't have time to fix it right now.
Hmm... I would like to say that the reader (__vmf_anon_prepare) access the same
data that the writer (__anon_vma_prepare()), lead to a race condition problem.

When the huge page alloc failed, the asm_exc_page_fault interrupt is fired but
on the same time the procces that was trying to alloc the huge page, try handle
to this failed too..

How READ_ONCE() and WRITE_ONCE() is atomic, the race problem can be resolved.

Thanks,
Guilherme

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

[Lance Yang]

On Sat, Aug 29, 2026 at 03:02:34PM -0300, Guilherme Giacomo Simoes wrote:
>Matthew Wilcox <willy@infradead.org> 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)
>
>>> The race occurs because the reader (__vmf_anon_prepare()) checks
>>> `vma->anon->vma` without holding the mmap_lock and withou the
>>> READ_ONCE() macro. Since the writer (__anon_vma_prepare()) is holding the
>>> mmap_lock and updating the pointer, it creates a data race as the two
>>> access are not properly synchronized.
>>
>> also this explanation is bogus.  i don't have time to fix it right now.
>Hmm... I would like to say that the reader (__vmf_anon_prepare) access the same
>data that the writer (__anon_vma_prepare()), lead to a race condition problem.
>
>When the huge page alloc failed, the asm_exc_page_fault interrupt is fired but
>on the same time the procces that was trying to alloc the huge page, try handle
>to this failed too..
>
>How READ_ONCE() and WRITE_ONCE() is atomic, the race problem can be resolved.

This patch should make KCSAN happy, IIUC, BUT ...

Still, the subject and changelog are rather confusing. The race is on
vma->anon_vma, not on a failed huge page allocation. READ_ONCE() and
WRITE_ONCE() do not stop the two faults from racing. They just tell
KCSAN that these concurrent accesses are intentional ... no?

Cheers, Lance

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

[Matthew Wilcox]

First, I hope you're a human being and not just doing what an LLM tells
you, because I'm putting effort into this.  Second, for the same reason,
I hope you stick around and make further contributions.

On Sat, Aug 29, 2026 at 03:02:34PM -0300, Guilherme Giacomo Simoes wrote:
> Matthew Wilcox <willy@infradead.org> 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?)

> >> The race occurs because the reader (__vmf_anon_prepare()) checks
> >> `vma->anon->vma` without holding the mmap_lock and withou the
> >> READ_ONCE() macro. Since the writer (__anon_vma_prepare()) is holding the
> >> mmap_lock and updating the pointer, it creates a data race as the two
> >> access are not properly synchronized.
> >
> > also this explanation is bogus.  i don't have time to fix it right now.
> Hmm... I would like to say that the reader (__vmf_anon_prepare) access the same
> data that the writer (__anon_vma_prepare()), lead to a race condition problem.
> 
> When the huge page alloc failed, the asm_exc_page_fault interrupt is fired but
> on the same time the procces that was trying to alloc the huge page, try handle
> to this failed too..
> 
> How READ_ONCE() and WRITE_ONCE() is atomic, the race problem can be resolved.

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

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.

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.

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

[Guilherme Giacomo Simoes]

Matthew Wilcox <willy@infradead.org> 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
> ... 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.) 

> 
> 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.
Yeah, I mentioned this on commit message because how is said by KCSAN the
__anon_vma_prepare() and __vmf_anon_prepare() is called after huge fault... My
interpretation might be wrong and if so, i will fix the commit message without
problem. 

Thanks Matthew and Lance for reviewing my patch and sharing your thoughts,

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@infradead.org> 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