Re: [PATCH] mm/rmap: fix missing barrier between anon_vma init and vma->anon_vma publish

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

Sorry, I looked at this on the weekend, then thought 'I should really not read
kernel mail on the weekend' and abandoned my reply. :)

On Sat, Sep 05, 2026 at 02:18:19PM +0800, Jinjiang Tu wrote:
> On arm64 server, we found  __anon_vma_prepare() reuses anon_vma and
> anon_vma->root is stale due to missing memory barrier, leading to
> lock and unlock two different anon_vma->root, thus leading to a anon_vma
> will never be unlocked, and another anon_vma couldn't be locked anymore.
>
> The race is as follows:
>
>     THREAD A                             THREAD B
> __anon_vma_prepare                __anon_vma_prepare
>  anon_vma = anon_vma_alloc();
>  // writes may out of order here

-> may be.

>  vma->anon_vma = anon_vma;
>                                    anon_vma = find_mergeable_anon_vma(vma);
>                                    anon_vma_lock_write(anon_vma);
>                                      // may still see the old root
>                                      down_write(&anon_vma->root->rwsem);
>                                    anon_vma_unlock_write(anon_vma);
>                                      // see the new root, never unlock old
>                                      up_write(&anon_vma->root->rwsem);

The left column is missing a lot of stuff I feel.

I think you need to make it clearer that what's happening is that you have:

	|-----------||-----------|
	|   VMA A   ||   VMA B   |
	|-----------||-----------|

Where VMA A and VMA B are merge compatible _except_ for a property that can
be modified by mprotect() (i.e. fulfills criteria of
anon_vma_compatible()).

There are racing faults on each.

Now VMA A has already tried calling find_mergeable_anon_vma(), which
checked VMA B for merge compatbility and found it was not, yet, faulted in.

Since:

static struct anon_vma *reusable_anon_vma(struct vm_area_struct *old,
					  struct vm_area_struct *a,
					  struct vm_area_struct *b)
{
	if (anon_vma_compatible(a, b)) {
		struct anon_vma *anon_vma = READ_ONCE(old->anon_vma);

		if (anon_vma && list_is_singular(&old->anon_vma_chain))
			return anon_vma; <---- Other VMA MUST have anon_vma.
	}
	return NULL;
}

So, __anon_vma_prepare() for VMA A goes ahead and allocates the anon VMA:

int __anon_vma_prepare(struct vm_area_struct *vma)
{
	...

	anon_vma = find_mergeable_anon_vma(vma);
	allocated = NULL;
	if (!anon_vma) {
		anon_vma = anon_vma_alloc();
		...
	}
	...
}

And sets its fields in anon_vma_alloc:

static inline struct anon_vma *anon_vma_alloc(void)
{
	struct anon_vma *anon_vma;

	anon_vma = kmem_cache_alloc(anon_vma_cachep, GFP_KERNEL);
	if (anon_vma) {
		...
		anon_vma->root = anon_vma;
	}

	return anon_vma;
}

Then it assigns vma->anon_vma and observe the classic gotcha with
acquire/release semantics:

int __anon_vma_prepare(struct vm_area_struct *vma)
{
	...

	<inlined>
	anon_vma = kmem_cache_alloc(anon_vma_cachep, GFP_KERNEL);
	if (anon_vma) {
		...
		anon_vma->root = anon_vma; ----------| Nothing stops this
	}                                            | being reordered
                                                     | to crit sect
                                                     |
	anon_vma_lock_write(anon_vma); --------------|------------
	spin_lock(&mm->page_table_lock);  -----------|------------ Acquire x2
                                                     |           |
	if (likely(!vma->anon_vma)) {                |           v
		vma->anon_vma = anon_vma;            |           ^
		...                                  v           |
	}                                                        |
	spin_unlock(&mm->page_table_lock); -----------------------
	anon_vma_unlock_write(anon_vma); ------------------------- Release x2

	...
}

And so you can end up in the verse situation of a weakly ordered arch
doing:

	vma->anon_vma = anon_vma;
	anon_vma->root = anon_vma;

And this the problem observed is that Thread/VMA B finds this VMA (it now
has vma->anon_vma assigned! So reusable_anon_vma() returns fine) and then:

int __anon_vma_prepare(struct vm_area_struct *vma)
{
	...

	anon_vma = find_mergeable_anon_vma(vma); <--- finds A!

	...
	anon_vma_lock_write(anon_vma); <-- goes to lock vma->anon_vma->root->rwsem
					   but... it's not assigned yet.
}

And guess what:

void __init anon_vma_init(void)
{
	anon_vma_cachep = kmem_cache_create("anon_vma", sizeof(struct anon_vma),
			0, SLAB_TYPESAFE_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT,
			anon_vma_ctor); ^----------------  It's
	anon_vma_chain_cachep = KMEM_CACHE(anon_vma_chain,    our friend...
			SLAB_PANIC|SLAB_ACCOUNT);          SLAB_TYPESAFE_BY_RCU!
}							   so _very_ easy to UAF.
							   Not so much 'stale' just
							   a UAF that might still work.
							   Or the root might still exist...

How hideous all round.

>
> thread A triggers page fault and calls __anon_vma_prepare() to prepare
> anon_vma for the faulting vma. __anon_vma_prepare() allocates and
> initializes a new anon_vma, and then publishes it to the vma with a plain
> store. anon_vma_prepare() only requires the mmap_lock to be held for
> reading, so two threads can fault on adjacent VMAs at the same time. While
> thread A publishes a new anon_vma, thread B could finds the anon_vma via
> find_mergeable_anon_vma() and then locks anon_vma->root->rwsem.
>
> However, due to missing barrier, thread B can observe the published pointer
> but a stale anon_vma->root because the stores from anon_vma_alloc() aren't

Less so stale, more UAF but you might get away with it...

> yet visible. What's the value of the stale anon_vma->root? __put_anon_vma()
> doesn't clear anon_vma->root, so the root of the new allocated anon_vma
> may point to a valid anon_vma.
>
> As a result, thread B can call anon_vma_lock_write() with the old root,
> and call anon_vma_unlock_write() with the new root, leading to a anon_vma
> will never be unlocked, and another anon_vma couldn't be locked anymore
> (it's count is dropped from 0 to -1 due to wrong unlock).
>
> To fix it, change the plain store `vma->anon_vma = anon_vma` to store
> release, so that the fields of anon_vma are visible before anon_vma is
> published to vma->anon_vma.
>
> We don't need a read barrier at read side for thread B. The load of

Well you _have_ to have a barrier of some kind for this to work...

> anon_vma and anon_vma->root have address-dependency. According to
> Documentation/memory-barriers.txt and some investigations, only Alpha
> needs address-dependency barriers and it has been handled by READ_ONCE().

...yup, because you have an address dependency :)

That works because, revising the memory barrier docs, the address
dependency works on the LOAD side only, because it has to deref to knoww
hat it's looking at whereas on the store side things can just live in the
store buffer.

>
> This issue needs two adjacent VMAs aren't merged but are compatible for
> anon_vma. We reproduced this issue in v5.10 with KSM enabled. The kernel
> doesn't merge commit cf7e7a3503df ("mm: prevent KSM from breaking VMA
> merging for new VMAs"), so there are many adjacent VMAs that aren't merged
> but are compatible for anon_vma.

How I hate the 'mergeable but not merged' terminology around this
particular part of anon_vma.

Well in any case I am replacing it all (eventually :) but in the meantime
it's irksome :)

>
> Without this fix, our production environment could reproduce this issue
> about 2-5 times each month. After adding a smp_mb() before
> anon_vma_lock_write(anon_vma) in __anon_vma_prepare(), which is different
> to this patch, this issue hasn't be reproduced for one month.

I don't see how the smp_mb() would make any difference there, I wonder if
you just reduced the race window?

>
> Cc: stable@vger.kernel.org
> Fixes: 5c341ee1dfc8 ("mm: track the root (oldest) anon_vma")

I do wonder if something more recent made this at least more possible.

A decade and a half without it being caught before seems... unlikely :)

I wonder if the VMA locks made this more possible by (significantly)
increasing the ability for racing faults to occur (no mmap read lock
required).

Either that or something increased the race window or this was somehow
accidentally mitigated somehow.

> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>

Very good spot thanks!

I feel the commit message should be reworked a bit to really clarify the
problem, feel free to steal my
explain-to-myself-because-memory-barriers-break-my-brain stuff above :)
also add the stack trace from the subthread.

I also want some changes in comments etc. but broadly I think your solution
is correct.

See attached patch for what I suggest for fixing that all up.

With everything addressed feel free to add:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

> ---
>  mm/rmap.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index d1819fd69938..a868e835eadb 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -209,7 +209,11 @@ 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;
> +		/*
> +		 * The fields of anon_vma must be visible before anon_vma
> +		 * is published to vma->anon_vma.
> +		 */

_Everything_ like this needs a 'paired with'.

In this case, paired with the READ_ONCE() in reusable_anon_vma() (and a
similar comment there please).

Also over there the comment:

 * NOTE! This runs with mmap_lock held for reading, so it is possible that
 * the anon_vma of 'old' is concurrently in the process of being set up
 * by another page fault trying to merge _that_. But that's ok: if it
 * is being set up, that automatically means that it will be a singleton
 * acceptable for merging, so we can do all of this optimistically. But
 * we do that READ_ONCE() to make sure that we never re-load the pointe

Will need updating.

Actually, see below, I provide a patch showing what edits I think make
sense here.

BTW I actually wondered about doing this with a lock instead.

Like:

  scoped_guard(spinlock, &mm->page_table_lock)
      anon_vma = find_mergeable_anon_vma(vma);

I think that should work across the board as you get the right
acquire/release semantics in both cases (you should then delete that
comment above, remove the READ_ONCE() in reusable_anon_vma() etc.)

BUT.

You are then in a weird situation where it's kinda lockless but we get away
with it because nobody else who touches anon_vma touches fields that might
exhibit this kind of behaviour.

So the barrier is probably still best.

Anyway, adjust the patch as below on respin and update the commit message
as requested above and think this is the right way to go.


> +		smp_store_release(&vma->anon_vma, anon_vma);
>  		anon_vma_chain_assign(vma, avc, anon_vma);
>  		anon_rmap_tree_insert(avc, anon_vma);
>  		anon_vma->num_active_vmas++;
> --
> 2.43.0
>

Cheers, Lorenzo

----8<----
diff --git a/mm/rmap.c b/mm/rmap.c
index d1819fd69938..f3b21aaa34ee 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -209,7 +209,11 @@ 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;
+		/*
+		 * Make anon_vma fields visible before anon_vma is published.
+		 * Paired with an address dependency in reusable_anon_vma().
+		 */
+		smp_store_release(&vma->anon_vma, anon_vma);
 		anon_vma_chain_assign(vma, avc, anon_vma);
 		anon_rmap_tree_insert(avc, anon_vma);
 		anon_vma->num_active_vmas++;
diff --git a/mm/vma.c b/mm/vma.c
index 35e7a64855fa..ec4101250d71 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -2094,6 +2094,13 @@ static int anon_vma_compatible(struct vm_area_struct *a, struct vm_area_struct *
  * acceptable for merging, so we can do all of this optimistically. But
  * we do that READ_ONCE() to make sure that we never re-load the pointer.
  *
+ * The READ_ONCE() establishes an address dependency between anon_vma and
+ * any access to its fields, which pairs with the assignment to
+ * vma->anon_vma performed with release semantics in __anon_vma_prepare().
+ *
+ * This is especially important as anon_vma's are SLAB_TYPESAFE_BY_RCU so
+ * accessing an uninitialised anon_vma's fields may result in a UAF.
+ *
  * IOW: that the "list_is_singular()" test on the anon_vma_chain only
  * matters for the 'stable anon_vma' case (ie the thing we want to avoid
  * is to return an anon_vma that is "complex" due to having gone through
@@ -2108,6 +2115,7 @@ static struct anon_vma *reusable_anon_vma(struct vm_area_struct *old,
 					  struct vm_area_struct *b)
 {
 	if (anon_vma_compatible(a, b)) {
+		/* Paired with a memory barrier in __anon_vma_prepare(). */
 		struct anon_vma *anon_vma = READ_ONCE(old->anon_vma);

 		if (anon_vma && list_is_singular(&old->anon_vma_chain))