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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 9/8/26 14:29, Jinjiang Tu wrote:
> On arm64 server, we find that a task trying to grab the anon_vma lock
> triggers hungtask.
> 
> INFO: task main:2354726 blocked for more than 120 seconds.
>       Tainted: G            E     5.10.0-0021.aarch64 #1
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:main            state:D stack:    0 pid:2354726 ppid:2350673 flags:0x00000a01
> Call trace:
>  __switch_to+0x7c/0xbc
>  __schedule+0x3b4/0x8a0
>  schedule+0x50/0xe0
>  rwsem_down_write_slowpath+0x3cc/0x6cc
>  down_write+0x60/0x260
>  __anon_vma_prepare+0x6c/0x210
>  do_anonymous_page+0x258/0x660
>  handle_pte_fault+0x188/0x214
>  __handle_mm_fault+0x1b0/0x380
>  handle_mm_fault+0xf4/0x284
>  do_page_fault+0x19c/0x494
>  do_translation_fault+0xcc/0xf8
>  do_mem_abort+0x48/0xac
>  el0_da+0x44/0x80
>  el0_sync_handler+0x88/0xb4
>  el0_sync+0x160/0x180
> 
> After analyzing the vmcore, we found the anon_vma->root->rwsem.count is -1.
> There is another anon_vma whose anon_vma->root->rwsem.count is 1, the
> anon_vma->root->rwsem.owner shows the lock is held, but the stack of the
> task shows the task doesn't hold the anon_vma lock.
> 
> After adding more debugging info, we found __anon_vma_prepare() reuses
> anon_vma and triggers the UAF of anon_vma->root due to missing memory
> barrier, leading to locking and unlocking two different anon_vma->root,
> thus leading to an anon_vma will never be unlocked, and another anon_vma
> couldn't be locked anymore.
> 
> This race requires two adjacent VMAs that are not merged but are
> anon_vma-compatible (e.g., they differ in VMA_ACCESS_FLAGS that can be
> changed by mprotect()). Two threads fault on each VMA concurrently, both
> calling __anon_vma_prepare() with only mmap_lock held for reading.
> 
>     THREAD A                             THREAD B
> __anon_vma_prepare                __anon_vma_prepare
>  find_mergeable_anon_vma() -> NULL
>  anon_vma = anon_vma_alloc();
>    anon_vma->root = anon_vma;
>  // the two stores may be reordered
>  vma->anon_vma = anon_vma;
>                                    // finds A's 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 find the anon_vma via
> find_mergeable_anon_vma() and then locks anon_vma->root->rwsem.
> 
> The store to anon_vma->root in anon_vma_alloc() and the store to
> vma->anon_vma can be reordered. The anon_vma_lock_write() and spin_lock()
> only provide acquire semantics, which do not prevent prior stores from
> being reordered after them. The release semantics of the corresponding
> spin_unlock() and anon_vma_unlock_write() come too late, the store to
> vma->anon_vma is already published before they take effect. As a result,
> thread B can observe the following order:
> 
>     vma->anon_vma = anon_vma;
>     anon_vma->root = anon_vma;
> 
> The anon_vma slab is SLAB_TYPESAFE_BY_RCU, so a newly allocated anon_vma
> may reuse memory from a previously freed one. The constructor
> (anon_vma_ctor) does not reset anon_vma->root, and __put_anon_vma()
> doesn't clear it either, so the old root value persists until
> anon_vma_alloc() overwrites it. If that store isn't visible, thread B
> reads a root that points to the old anon_vma and locks it.
> 
> 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 an anon_vma
> will never be unlocked, and another anon_vma couldn't be locked anymore
> (its 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.
> 
> At read side, 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() in reusable_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 been reproduced for one month.
> 
> Cc: stable@vger.kernel.org
> Fixes: 5c341ee1dfc8 ("mm: track the root (oldest) anon_vma")
> Reviewed-by: Lance Yang <lance.yang@linux.dev>
> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---
> Change since v1:
>  * correct the fix tag (Lance Yang)
>  * rework commit message and update comment (Lorenzo Stoakes)
>  * collect Reviewed-by
> 
>  mm/rmap.c | 6 +++++-
>  mm/vma.c  | 8 ++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> 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);


Makes perfect sense to me, although I am not that familiar with all the nasty
details of anon_vma merging (in contrast to Lorenzo ;) )

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David