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

[Lance Yang <lance.yang@linux.dev>]

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

Good catch, thanks!

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

Shouldn't the tag point to the following ?

Fixes: 012f18004da3 ("mm: always lock the root (oldest) anon_vma")

5c341ee1dfc8 introduced anon_vma->root, but 012f18004da3 made the lock
helpers dereference it. Before that, anon_vma_lock() used anon_vma->lock
directly, so a stale root could not cause this lock/unlock mismatch.

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

Hmm ... no luck reproducing this locally ... Still, AFAICT the race is
real.

For the read side, no extra barrier needed because the anon_vma->root
dereference is address-dependent on the READ_ONCE() load, IIUC :)

> 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.
>+		 */

Maybe be more specific:

/*
 * Publish anon_vma only after ->root is visible, otherwise a
 * concurrent fault may dereference a stale ->root when taking
 * the rwsem.
 */

Otherwise, LGTM.

Reviewed-by: Lance Yang <lance.yang@linux.dev>

Cheers, Lance