Re: [PATCH] mm/vma: fix NULL anon_vma deref in validate_mm() under mmap read lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/vma: use of data_race() instead of READ_ONCE() in validate_mm()
--

commit 3d1c343bc8c8b0bc43d0e0e2048a1459c107a3cf
Author: Songsen Yang <yangsongsen@insta360.com>

mm/vma: fix NULL anon_vma deref in validate_mm() under mmap read lock

This commit changes validate_mm() to read vma->anon_vma into a local variable
with data_race() before checking it and taking anon_vma_lock_read(), avoiding
a race where a concurrent page fault installs vma->anon_vma under the mmap
read lock between a separate check and lock acquisition.

> diff --git a/mm/vma.c b/mm/vma.c
> index 4672d420521e3..b674c69f720c1 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -679,7 +679,7 @@ void validate_mm(struct mm_struct *mm)
>  	mt_validate(&mm->mm_mt);
>  	for_each_vma(vmi, vma) {
>  #ifdef CONFIG_DEBUG_VM_RB
> -		struct anon_vma *anon_vma = vma->anon_vma;
> +		struct anon_vma *anon_vma;
>  		struct anon_vma_chain *avc;
>  #endif
>  		unsigned long vmi_start, vmi_end;
> @@ -703,7 +703,13 @@ void validate_mm(struct mm_struct *mm)
>  		}
>  
>  #ifdef CONFIG_DEBUG_VM_RB
> -		if (vma_has_anon_rmap(vma)) {
> +		/*
> +		 * Callers may hold only the mmap read lock, in which case a
> +		 * concurrent fault can install vma->anon_vma at any time. Read
> +		 * it once, so that the check and the lock use the same value.
> +		 */
> +		anon_vma = data_race(vma->anon_vma);
> +		if (anon_vma) {
>  			anon_vma_lock_read(anon_vma);

[Severity: Medium]
Should validate_mm() use READ_ONCE(vma->anon_vma) here instead of
data_race(vma->anon_vma)?

In include/linux/compiler.h, data_race() is a KCSAN annotation that evaluates
its argument as a plain C expression without volatile semantics:

#define data_race(expr)							\
({									\
	__kcsan_disable_current();					\
	disable_context_analysis();					\
	auto __v = (expr);						\
	enable_context_analysis();					\
	__kcsan_enable_current();					\
	__v;								\
})

Because data_race() does not prevent compiler reloads, the compiler is still
permitted to reload vma->anon_vma between the NULL check and
anon_vma_lock_read(anon_vma).

In addition, when vms_complete_munmap_vmas() calls validate_mm() after
downgrading the mmap lock to a read lock, a concurrent page fault in
__anon_vma_prepare() publishes vma->anon_vma with release semantics after
initializing anon_vma->root on a SLAB_TYPESAFE_BY_RCU slab:

mm/rmap.c:__anon_vma_prepare() {
	...
		/*
		 * 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);
	...
}

Unlike vma_has_anon_rmap(), which only tests whether the pointer is non-NULL,
validate_mm() dereferences anon_vma->root->rwsem in anon_vma_lock_read() and
anon_vma_unlock_read(). Without READ_ONCE(), as used by reusable_anon_vma() in
mm/vma.c:

mm/vma.c:reusable_anon_vma() {
	...
		/* Paired with a memory barrier in __anon_vma_prepare(). */
		struct anon_vma *anon_vma = READ_ONCE(old->anon_vma);
	...
}

is the address dependency paired with smp_store_release() guaranteed when
dereferencing anon_vma->root?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102074414.10902-1-yangsongsen@insta360.com?part=1