Re: [PATCH] mm/vma: read anon_vma carefully in validate_mm()

[Lorenzo Stoakes (ARM)]

On Mon, Nov 02, 2026 at 10:48:45AM +0000, Kiryl Shutsemau wrote:
> On Mon, Nov 02, 2026 at 10:25:20AM +0000, Lorenzo Stoakes (ARM) wrote:
> > Commit 38564710f443 ("mm: implement and use vma_has_anon_rmap(), silence
> > KCSAN") rather clumsily converted the read of vma->anon_vma into a
> > vma_has_anon_rmap() without accounting for the separation between
> > assignment of anon_vma and later use of it.
> >
> > vma_has_anon_rmap() is intended as an optimistic check, so doesn't need to
> > protect against a torn read in vma->anon_vma. The caller is expected to
> > take care of this themselves if they then in fact access the field.
> >
> > validate_mm() does indeed then reference the field, so resolve the issue
> > via READ_ONCE(), and simplify the code by separating this into a new
> > function validate_mm_anon_rmap().
> >
> > Finally, add a vma_assert_stabilised() to make it clear the required state
> > of the VMA.
> >
> > Fixes: 38564710f443 ("mm: implement and use vma_has_anon_rmap(), silence KCSAN")
> > Reported-by: syzbot+10e72ea5747737bcd2ad@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://lore.kernel.org/all/6ae78418.6552420f.2a0efc.005c.GAE@xxxxxxxxxx/
> > Reported-by: Songsen Yang <yangsongsen@xxxxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/20261102074414.10902-1-yangsongsen@xxxxxxxxxxxx/
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> > ---
> >  mm/vma.c | 36 ++++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index d416ba58eb4c..9f02f19a7845 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -669,6 +669,28 @@ static int dup_anon_vma(struct vm_area_struct *dst,
> >  }
> >
> >  #ifdef CONFIG_DEBUG_VM_MAPLE_TREE
> > +#ifdef CONFIG_DEBUG_VM_RB
> > +static void validate_mm_anon_rmap(struct vm_area_struct *vma)
> > +{
> > +	struct anon_vma *anon_vma = READ_ONCE(vma->anon_vma);
> > +	struct anon_vma_chain *avc;
> > +
> > +	vma_assert_stabilised(vma);
> > +
> > +	if (!anon_vma)
> > +		return;
> > +
> > +	anon_vma_lock_read(anon_vma);
> > +	list_for_each_entry(avc, &vma->anon_vma_chain, same_vma)
> > +		anon_rmap_tree_verify(avc);
> > +	anon_vma_unlock_read(anon_vma);
> > +}
> > +#else
> > +static void validate_mm_anon_rmap(struct vm_area_struct *vma)
> > +{
> > +}
> > +#endif
> > +
> >  void validate_mm(struct mm_struct *mm)
> >  {
> >  	int bug = 0;
>
> This ifdeffery seems unnecessary. What about this:

Yup, even better.

Andrew - could you update it in-place or do you need a v2?

>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 9f167950926a..2e43077dcd51 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4363,9 +4363,7 @@ anon_rmap_tree_iter_first(struct anon_vma *anon_vma,
>  struct anon_vma_chain *
>  anon_rmap_tree_iter_next(struct anon_vma_chain *avc,
>  			 pgoff_t pgoff_start, pgoff_t pgoff_last);
> -#ifdef CONFIG_DEBUG_VM_RB
>  void anon_rmap_tree_verify(struct anon_vma_chain *avc);
> -#endif
>
>  #define anon_rmap_tree_foreach(avc, anon_vma, pgoff_start, pgoff_last)		 \
>  	for (avc = anon_rmap_tree_iter_first(anon_vma, pgoff_start, pgoff_last); \
> diff --git a/mm/vma.c b/mm/vma.c
> index 1be613044b4a..30f31d3b3e26 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -669,6 +669,26 @@ static int dup_anon_vma(struct vm_area_struct *dst,
>  }
>
>  #ifdef CONFIG_DEBUG_VM_MAPLE_TREE
> +static void validate_mm_anon_rmap(struct vm_area_struct *vma)
> +{
> +	struct anon_vma *anon_vma;
> +	struct anon_vma_chain *avc;
> +
> +	if (!IS_ENABLED(CONFIG_DEBUG_VM_RB))
> +		return;
> +
> +	vma_assert_stabilised(vma);
> +
> +	anon_vma = READ_ONCE(vma->anon_vma);
> +	if (!anon_vma)
> +		return;
> +
> +	anon_vma_lock_read(anon_vma);
> +	list_for_each_entry(avc, &vma->anon_vma_chain, same_vma)
> +		anon_rmap_tree_verify(avc);
> +	anon_vma_unlock_read(anon_vma);
> +}
> +
>  void validate_mm(struct mm_struct *mm)
>  {
>  	int bug = 0;
> @@ -678,10 +698,6 @@ 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_chain *avc;
> -#endif
>  		unsigned long vmi_start, vmi_end;
>  		bool warn = 0;
>
> @@ -702,14 +718,8 @@ void validate_mm(struct mm_struct *mm)
>  			vma_iter_dump_tree(&vmi);
>  		}
>
> -#ifdef CONFIG_DEBUG_VM_RB
> -		if (vma_has_anon_rmap(vma)) {
> -			anon_vma_lock_read(anon_vma);
> -			list_for_each_entry(avc, &vma->anon_vma_chain, same_vma)
> -				anon_rmap_tree_verify(avc);
> -			anon_vma_unlock_read(anon_vma);
> -		}
> -#endif
> +		validate_mm_anon_rmap(vma);
> +
>  		/* Check for a infinite loop */
>  		if (++i > mm->map_count + 10) {
>  			i = -1;
> --
>   Kiryl Shutsemau / Kirill A. Shutemov

--
Cheers, Lorenzo