Re: [PATCH] mm/damon/vaddr: drop redundant mmap_read_lock from damon_va_three_regions

[SeongJae Park <sj@kernel.org>]

On Sun, 07 Jun 2026 14:00:16 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Unsafe lockless VMA iteration exposes `SLAB_TYPESAFE_BY_RCU` freelist pointer corruption
> 
> Pre-existing issues:
> - [High] Missing `check_stable_address_space()` validation during external `mm_struct` VMA iteration
> --
> 
> commit fc1954fba1bc736b5f26aad51449f15c4774293c
> Author: Cunlong Li <shenxiaogll@gmail.com>
> 
> mm/damon/vaddr: drop redundant mmap_read_lock from damon_va_three_regions
> 
> __damon_va_three_regions() already walks VMAs under rcu_read_lock(), so
> the outer mmap_read_lock() is no longer needed.

Is this correct?  The rcu_read_lock() is for protecting only vma traversal.
mmap_read_lock() is required for reading the entire VMAs of the snapshot.  Am I
missing something?

> 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index b069dbc7e3d25..27e34596c43b8 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -77,7 +77,7 @@ static int __damon_va_three_regions(struct mm_struct *mm,
> >  	struct damon_addr_range first_gap = {0}, second_gap = {0};
> >  	VMA_ITERATOR(vmi, mm, 0);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but is it safe to iterate over
> the VMAs of an external mm_struct here without first checking
> check_stable_address_space(mm)?
> 
> If the target address space becomes unstable (for example, due to an OOM
> reap or a failed dup_mmap()), the maple tree may contain XA_ZERO_ENTRY
> markers and the mm is flagged MMF_UNSTABLE.
> 
> Without validating the address space, could the maple tree iterator
> return XA_ZERO_ENTRY as a VMA pointer and lead to a kernel panic when
> vma->vm_start is dereferenced?

We hold mmap_read_lock(), so I believe it is safe.  Correct me if I'm wrong.

> 
> >  	struct vm_area_struct *vma, *prev = NULL;
> > -	unsigned long start;
> > +	unsigned long start = 0, last_vma_end = 0;
> >  
> >  	/*
> >  	 * Find the two biggest gaps so that first_gap > second_gap > others.
> 
> [ ... ]
> 
> > @@ -140,9 +141,7 @@ static int damon_va_three_regions(struct damon_target *t,
> >  	if (!mm)
> >  		return -EINVAL;
> >  
> > -	mmap_read_lock(mm);
> >  	rc = __damon_va_three_regions(mm, regions);
> > -	mmap_read_unlock(mm);
> 
> [Severity: High]
> Does removing this mmap_read_lock() expose the VMA iteration to
> SLAB_TYPESAFE_BY_RCU freelist pointer corruption?
> 
> Since the VMA slab cache is created with SLAB_TYPESAFE_BY_RCU, a VMA can
> be freed while an RCU reader is still accessing it. When this happens,
> the SLUB allocator overwrites the first word of the structure with its
> freelist pointer (vm_freeptr).
> 
> Because vm_freeptr is in a union with vm_start and vm_end, locklessly
> reading vma->vm_start from a recently freed VMA in
> __damon_va_three_regions() will yield an unvalidated kernel freelist
> pointer instead of a valid address.
> 
> Could subtracting prev->vm_end from this corrupted pointer cause massive
> arithmetic underflows, tricking DAMON into tracking bogus memory regions?

Sounds like making sense to me.  Cunlong, what do you think?


Thanks,
SJ

[...]