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

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

[Cunlong Li]

__damon_va_three_regions() already walks VMAs under rcu_read_lock(), so
the outer mmap_read_lock() is no longer needed.

---
Signed-off-by: Cunlong Li <shenxiaogll@gmail.com>
---
 mm/damon/vaddr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index b069dbc7e3d2..27e34596c43b 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);
 	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.
@@ -104,6 +104,7 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 		}
 next:
 		prev = vma;
+		last_vma_end = vma->vm_end;
 	}
 	rcu_read_unlock();
 
@@ -120,7 +121,7 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 	regions[1].start = ALIGN(first_gap.end, DAMON_MIN_REGION_SZ);
 	regions[1].end = ALIGN(second_gap.start, DAMON_MIN_REGION_SZ);
 	regions[2].start = ALIGN(second_gap.end, DAMON_MIN_REGION_SZ);
-	regions[2].end = ALIGN(prev->vm_end, DAMON_MIN_REGION_SZ);
+	regions[2].end = ALIGN(last_vma_end, DAMON_MIN_REGION_SZ);
 
 	return 0;
 }
@@ -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);
 
 	mmput(mm);
 	return rc;

---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260607-rcu-24b324e0f076

Best regards,
-- 
Cunlong Li <shenxiaogll@gmail.com>

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

[sashiko-bot]

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.

> 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?

>  	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?

>  
>  	mmput(mm);
>  	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607-rcu-v1-1-3c9f5995a3a0@gmail.com?part=1

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

[SeongJae Park]

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

[...]

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

[SeongJae Park]

Hello Cunlong,

On Sun, 07 Jun 2026 21:48:52 +0800 Cunlong Li <shenxiaogll@gmail.com> wrote:

> __damon_va_three_regions() already walks VMAs under rcu_read_lock(), so
> the outer mmap_read_lock() is no longer needed.

The function aims to read a snapshot of the virtual address space.  So I think
mmap_read_lock() is still required.  Please let me know if I'm missing
something.


Thanks,
SJ

[...]

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

[Cunlong Li]

On Sun, Jun 07, 2026 at 10:36:44AM -0700, SeongJae Park wrote:
> Hello Cunlong,
> 
> On Sun, 07 Jun 2026 21:48:52 +0800 Cunlong Li <shenxiaogll@gmail.com> wrote:
> 
> > __damon_va_three_regions() already walks VMAs under rcu_read_lock(), so
> > the outer mmap_read_lock() is no longer needed.
> 
> The function aims to read a snapshot of the virtual address space.  So I think
> mmap_read_lock() is still required.  Please let me know if I'm missing
> something.
> 
> 
> Thanks,
> SJ
> 
> [...]

Yes — I understand this path is meant to obtain a snapshot of the VMA layout.

My thinking was that an exact snapshot may not be required here, and
the RCU-protected traversal inside __damon_va_three_regions() already
makes the walk safe, so the outer mmap_read_lock() might be redundant.

But, if we want a consistent VMA layout here, mmap_read_lock() is the
right mechanism and rcu_read_lock() alone is not enough.  I'll drop
the patch :)

Thanks,
Cunlong