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

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

[IgorpetinDev]

__damon_va_three_regions() is called only by damon_va_three_regions(),
which already holds mmap_read_lock(). Since mmap_read_lock() is held,
the maple tree and VMA list are protected from concurrent changes.
Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.

Signed-off-by: IgorpetinDev <igorpetindev@gmail.com>
---
 mm/damon/vaddr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index d27147603..345b89b5d 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -84,7 +84,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) {
 		unsigned long gap;
 
@@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 next:
 		prev = vma;
 	}
-	rcu_read_unlock();
 
 	if (!sz_range(&second_gap) || !sz_range(&first_gap))
 		return -EINVAL;
-- 
2.47.3

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

[sashiko-bot]

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629165349.4260-1-igorpetindev@gmail.com?part=1

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

[SJ Park]

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

[...]

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

[SJ Park]

On Mon, 29 Jun 2026 19:53:49 +0300 IgorpetinDev <igorpetindev@gmail.com> wrote:

> __damon_va_three_regions() is called only by damon_va_three_regions(),
> which already holds mmap_read_lock(). Since mmap_read_lock() is held,
> the maple tree and VMA list are protected from concurrent changes.
> Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.
> 
> Signed-off-by: IgorpetinDev <igorpetindev@gmail.com>

We prefer using a real name.  I show you used 'Igor Putko' as your name before.
Any reason to change the name?

> ---
>  mm/damon/vaddr.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index d27147603..345b89b5d 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -84,7 +84,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) {
>  		unsigned long gap;
>  
> @@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
>  next:
>  		prev = vma;
>  	}
> -	rcu_read_unlock();

As Sashiko also commented, this may cause lockdep warning.  Actually this was
added to fix it.  Refer to the previous discussions [1] for more details.

How about moving rcu_read_[un]lock() to the caller test code?

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


Thanks,
SJ

[...]

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

[Igor Putko]

From: IgorpetinDev <igorpetindev@gmail.com>

__damon_va_three_regions() is called only by damon_va_three_regions(),
which already holds mmap_read_lock(). Since mmap_read_lock() is held,
the maple tree and VMA list are protected from concurrent changes.
Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.

Signed-off-by: IgorpetinDev <igorpetindev@gmail.com>
---
 mm/damon/vaddr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index d27147603..345b89b5d 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -84,7 +84,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) {
 		unsigned long gap;
 
@@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 next:
 		prev = vma;
 	}
-	rcu_read_unlock();
 
 	if (!sz_range(&second_gap) || !sz_range(&first_gap))
 		return -EINVAL;
-- 
2.47.3

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

[Igor Putko]

__damon_va_three_regions() is called only by damon_va_three_regions(),
which already holds mmap_read_lock(). Since mmap_read_lock() is held,
the maple tree and VMA list are protected from concurrent changes.
Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.

Signed-off-by: Igor Putko <igorpetindev@gmail.com>
---
 mm/damon/tests/vaddr-kunit.h | 2 ++
 mm/damon/vaddr.c             | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/damon/tests/vaddr-kunit.h b/mm/damon/tests/vaddr-kunit.h
index 563fbc7e3..70cb7825e 100644
--- a/mm/damon/tests/vaddr-kunit.h
+++ b/mm/damon/tests/vaddr-kunit.h
@@ -81,7 +81,9 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
 	if (__link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)))
 		kunit_skip(test, "Failed to create VMA tree");
 
+	rcu_read_lock();
 	__damon_va_three_regions(&mm, regions);
+	rcu_read_unlock();
 
 	KUNIT_EXPECT_EQ(test, 10ul, regions[0].start);
 	KUNIT_EXPECT_EQ(test, 25ul, regions[0].end);
diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index d27147603..345b89b5d 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -84,7 +84,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) {
 		unsigned long gap;
 
@@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 next:
 		prev = vma;
 	}
-	rcu_read_unlock();
 
 	if (!sz_range(&second_gap) || !sz_range(&first_gap))
 		return -EINVAL;
-- 
2.47.3

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

[Igor Putko]

Hi SJ,

On Mon, 29 Jun 2026, SJ Park wrote:
> We prefer using a real name. I show you used 'Igor Putko' as your name before.
> Any reason to change the name?

Apologies for the confusion. I recently reinstalled my OS and misconfigured
my git setup. I will revert back to 'Igor Putko' for the v2 submission.

> As Sashiko also commented, this may cause lockdep warning. Actually this was
> added to fix it. Refer to the previous discussions [1] for more details.
> 
> How about moving rcu_read_[un]lock() to the caller test code?

That makes total sense. I will move the rcu_read_lock() and rcu_read_unlock()
calls directly into the KUnit test code in mm/damon/tests/vaddr-kunit.h to
prevent the lockdep splat.

Regarding the pre-existing high-severity issue reported by Sashiko AI about
missing check_stable_address_space(), I believe it is out of scope for this
particular cleanup and should be addressed in a separate, dedicated patch.

I have just sent the v2 patch out.

Thanks,
Igor

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] VMA iteration on an external mm_struct is performed without checking check_stable_address_space(mm), which can lead to iterating a corrupted maple tree.
--

commit fdc4435ee0611e8373511ef284a90ecc98420eec
Author: Igor Putko <igorpetindev@gmail.com>

mm/damon/vaddr: remove redundant RCU lock

This commit removes rcu_read_lock() and rcu_read_unlock() from
__damon_va_three_regions() and adds them to its kunit test. It asserts that
mmap_read_lock() is sufficient to protect the maple tree during VMA iteration.

> 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: High]
This is a pre-existing issue, but while reviewing the VMA traversal here,
I noticed a potential problem with how the mmap_read_lock is used by the
callers of this function.

When traversing VMAs on an external mm_struct, does this code need to check
check_stable_address_space(mm) before iterating? 

Looking at damon_va_three_regions() and damon_va_walk_page_range(), they
acquire mmap_read_lock(mm) and proceed directly to VMA traversal:

mm/damon/vaddr.c:damon_va_three_regions() {
	...
	mmap_read_lock(mm);
	rc = __damon_va_three_regions(mm, regions);
	mmap_read_unlock(mm);
	...
}

If the OOM reaper runs on the target process, it can mark the mm as unstable
and leave XA_ZERO_ENTRY markers in the maple tree. Iterating this corrupted
tree without first verifying check_stable_address_space(mm) can lead to
kernel crashes.

Could this be triggered under system memory pressure while DAMON is
monitoring?

>  		unsigned long gap;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630130253.3843-1-igorpetindev@gmail.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Removing `rcu_read_lock()` breaks the KUnit test `damon_test_three_regions_in_vmas()`, causing a lockdep splat.

Pre-existing issues:
- [High] VMA iteration on an external mm_struct is performed without checking for address space stability.
--

commit d9d9a562dbd8520f8d06d146def9565fda08562d
Author: IgorpetinDev <igorpetindev@gmail.com>

mm/damon/vaddr: remove redundant RCU lock

This commit removes the rcu_read_lock() and rcu_read_unlock() calls from
__damon_va_three_regions(). The commit message states that since
mmap_read_lock() is already held by the caller, the maple tree and VMA list
are protected from concurrent changes, making the RCU locks redundant.

> 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 removing this rcu_read_lock() cause a lockdep splat in the KUnit tests?

Looking at damon_test_three_regions_in_vmas() in mm/damon/tests/vaddr-kunit.h,
it calls __damon_va_three_regions() directly on a mocked maple tree:

	__damon_va_three_regions(&mm, regions);

Because this KUnit test doesn't hold mmap_read_lock() or configure an external
tree lock, removing the localized rcu_read_lock() here will cause for_each_vma()
to trigger RCU lockdep assertions for suspicious RCU usage.

[Severity: High]
This isn't a bug introduced by this patch, but does this function safely iterate
over an external mm_struct?

Looking at the caller damon_va_three_regions() in mm/damon/vaddr.c, it acquires
mmap_read_lock() on an external mm, but check_stable_address_space(mm) is never
called before iterating the VMAs.

If the target process fails dup_mmap() during fork or is processed by the OOM
reaper, its mm is flagged MMF_UNSTABLE and the maple tree slots may contain
XA_ZERO_ENTRY markers. Iterating over these markers without checking address
space stability can cause the iterator to return invalid VMA pointers, which
will cause a panic when dereferenced.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630130158.3826-1-igorpetindev@gmail.com?part=1

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

[SJ Park]

On Tue, 30 Jun 2026 16:07:43 +0300 Igor Putko <igorpetindev@gmail.com> wrote:

> Hi SJ,
> 
> On Mon, 29 Jun 2026, SJ Park wrote:
> > We prefer using a real name. I show you used 'Igor Putko' as your name before.
> > Any reason to change the name?
> 
> Apologies for the confusion. I recently reinstalled my OS and misconfigured
> my git setup. I will revert back to 'Igor Putko' for the v2 submission.

No worries!  I'm glad that I helped you finding a misconfiguration!

> 
> > As Sashiko also commented, this may cause lockdep warning. Actually this was
> > added to fix it. Refer to the previous discussions [1] for more details.
> > 
> > How about moving rcu_read_[un]lock() to the caller test code?
> 
> That makes total sense. I will move the rcu_read_lock() and rcu_read_unlock()
> calls directly into the KUnit test code in mm/damon/tests/vaddr-kunit.h to
> prevent the lockdep splat.
> 
> Regarding the pre-existing high-severity issue reported by Sashiko AI about
> missing check_stable_address_space(), I believe it is out of scope for this
> particular cleanup and should be addressed in a separate, dedicated patch.

I was thinking it is not a real issue.  Do you think it is a real?

> 
> I have just sent the v2 patch out.

From next time, please give enough time before sending a new revision (say, ~12
hours at least?), to ensure the discussion on the previous revision is
concluded.


Thanks,
SJ

[...]

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

[SJ Park]

Hello Igor,


Thank you for this patch.

From next time, please give enough time for others to comment before posting a
new revision of the patch.

Also, please post a new revision as a new thread with a changelog [1] including
links to previous revisions, rather than a reply to the previous revision.

On Tue, 30 Jun 2026 16:02:53 +0300 Igor Putko <igorpetindev@gmail.com> wrote:

> __damon_va_three_regions() is called only by damon_va_three_regions(),
> which already holds mmap_read_lock(). Since mmap_read_lock() is held,
> the maple tree and VMA list are protected from concurrent changes.
> Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.

This is not explaining the test part change.  Could you please update for that?

Also, have you had a chance to theoretically or experiemently confirm the
problem [2] that motivated to add the rcu_read_lock() at that place is not
reintroduced by this change?  If so, could you please add that to the commit
log, too?


Thanks,
SJ

[1] https://docs.kernel.org/process/submitting-patches.html#commentary
[2] https://lore.kernel.org/b83651a0-5b24-4206-b860-cb54ffdf209b@roeck-us.net

[...]

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

[Igor Putko]

Hi SJ,

Thanks for the reply and the pointers!
Regarding the issue Sashiko AI flagged - I dug into damon/vaddr.c, and I think it's a real bug, but it's isolated to __damon_va_three_regions().
damon_va_walk_page_range() is fine here, since lock_vma_under_rcu() handles stability on its own. __damon_va_three_regions() is the problematic one: right after mmap_read_lock(mm) is taken in the caller, it sets up a VMA_ITERATOR and runs for_each_vma(). But mmap_read_lock doesn't stop the OOM reaper from running concurrently, setting MMF_UNSTABLE, and leaving empty markers in the maple tree. Walking the tree in that state without a check_stable_address_space(mm) call could lead to a NULL pointer dereference.
Since this looks like a separate issue, I'll prepare a dedicated fix and send it as its own patch thread, so it doesn't hold up the current cleanup. Let me know if that approach works for you.

Thanks,
Igor

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

[SJ Park]

On Tue, 30 Jun 2026 17:25:22 +0300 Igor Putko <igorpetindev@gmail.com> wrote:

> Hi SJ,
> 
> Thanks for the reply and the pointers!

Thank you for quick reply.  Nonetheless, from the next time, please reply to a
mail you are replying to, without top-posting [1].  Also, please wrap long
lines for reasonable length of columns (say, 80 columns?)

> Regarding the issue Sashiko AI flagged - I dug into damon/vaddr.c, and I think it's a real bug, but it's isolated to __damon_va_three_regions().
> damon_va_walk_page_range() is fine here, since lock_vma_under_rcu() handles stability on its own. __damon_va_three_regions() is the problematic one: right after mmap_read_lock(mm) is taken in the caller, it sets up a VMA_ITERATOR and runs for_each_vma(). But mmap_read_lock doesn't stop the OOM reaper from running concurrently, setting MMF_UNSTABLE, and leaving empty markers in the maple tree. Walking the tree in that state without a check_stable_address_space(mm) call could lead to a NULL pointer dereference.

Thank you for detailed explanation.

> Since this looks like a separate issue, I'll prepare a dedicated fix and send it as its own patch thread, so it doesn't hold up the current cleanup. Let me know if that approach works for you.

Makes sense!

[1] https://subspace.kernel.org/etiquette.html


Thanks,
SJ

[...]