Re: [PATCH] mm/damon/vaddr: remove redundant RCU lock

[SJ Park <sj@kernel.org>]

On Mon, 29 Jun 2026 17:06:27 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] The removal of `rcu_read_lock()` breaks the KUnit test `damon_test_three_regions_in_vmas()`, causing a lockdep splat.
> 
> Pre-existing issues:
> - [High] VMA tree iteration on an external mm_struct is performed without checking if the address space is stable, risking a crash on unstable MMs.
> --
> 
> commit e3efcd87bda8a6d5dd6bcc064e6f5876f9980a31
> Author: IgorpetinDev <igorpetindev@gmail.com>
> 
> mm/damon/vaddr: remove redundant RCU lock
> 
> This patch removes the rcu_read_lock() and rcu_read_unlock() calls from
> __damon_va_three_regions(), assuming that the mmap_read_lock is always
> held by the caller.
> 
> > __damon_va_three_regions() is called only by damon_va_three_regions(),
> > which already holds mmap_read_lock(). Since mmap_read_lock() is held,
> 
> [Severity: High]
> This is a pre-existing issue, but when damon_va_three_regions() acquires the
> mmap_read_lock on an external task's mm_struct, is it safe to begin
> traversing the VMA tree without first verifying address space stability?
> 
> If the target process is concurrently undergoing an OOM kill or failed a
> dup_mmap(), the mm_struct could be flagged MMF_UNSTABLE and the maple tree
> could contain XA_ZERO_ENTRY markers. Should damon_va_three_regions() call
> check_stable_address_space() before __damon_va_three_regions() traverses
> the tree, to prevent a potential kernel panic from dereferencing invalid
> markers?

A similar question was asked [1] before.  As I also replied at that time, I
believe this is safe.  Correct me if I'm wrong.

> 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index e73ec1ce016e5..882cc7cb09027 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -82,7 +82,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
> >  	 * If this is too slow, it can be optimised to examine the maple
> >  	 * tree gaps.
> >  	 */
> > -	rcu_read_lock();
> >  	for_each_vma(vmi, vma) {

> 
> [Severity: Medium]
> Will the removal of this lock break the KUnit test
> damon_test_three_regions_in_vmas() located in mm/damon/tests/vaddr-kunit.h?
> 
> The test framework calls __damon_va_three_regions() directly using a dummy
> mm_struct and a maple tree initialized without the MT_FLAGS_LOCK_EXTERN flag.
> Since the test environment doesn't hold the internal maple tree lock or an
> external mmap_lock, won't removing this rcu_read_lock() cause a lockdep
> warning (suspicious rcu_dereference_check usage) when for_each_vma()
> attempts to traverse the tree?

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

[1] https://lore.kernel.org/damon/20260607172650.93719-1-sj@kernel.org/


Thanks,
SJ

[...]