[RFC PATCH v1.2 0/5] mm/damon: unurgent fixes for infinite loop, NULL de-ref and races

[RFC PATCH v1.2 0/5] mm/damon: unurgent fixes for infinite loop, NULL de-ref and races

[SJ Park]

Sashiko found a few issues in DAMON that could cause infinite loop, NULL
dereference and monitoring results degradation.  The first two sounds
scary but the infinite loop happens only under unreasonable user setup.
The NULL dereference is only in a unit test.  Monitoring results
degradation is trivial since it is only best-effort, and those happens
from only unlikely races.  Still those are bugs that better to fix if
possible. Fix those.

Changes from RFC v1.1
- RFC v1.1: https://lore.kernel.org/20260712170328.91144-1-sj@kernel.org
- Remove same_target param from __damon_va_check_access().
Changes from RFC
- RFC: https://lore.kernel.org/20260711180409.82093-1-sj@kernel.org
- Rebase to mm-new.

SJ Park (5):
  mm/damon/core: avoid infinite kdamond_merge_regions() internal loop
  mm/damon/tests/core-kunit: catch test failure in
    test_merge_regions_of()
  mm/damon/vaddr: drop last same folio access check optimization
  mm/damon/paddr: drop last same folio access check reuse optimization
  mm/damon/sysfs: read ops_id only once

 mm/damon/core.c             | 13 +++++++++----
 mm/damon/paddr.c            | 20 ++++----------------
 mm/damon/sysfs.c            |  6 ++++--
 mm/damon/tests/core-kunit.h |  3 +++
 mm/damon/vaddr.c            | 33 ++++++---------------------------
 5 files changed, 26 insertions(+), 49 deletions(-)


base-commit: 9d192aa422c3f9c503a40e5eb98093df671accf8
-- 
2.47.3

[RFC PATCH v1.2 1/5] mm/damon/core: avoid infinite kdamond_merge_regions() internal loop

[SJ Park]

Due to online parameter update like events, the number of DAMON regions
could be higher than the user-set upper limit.  kdamond_merge_regions()
repeats merge regions until the number meets the limit, while doubling
the merge threshold up to the theoretical maximum threshold.  It is
tried only up to the theoretical maximum threshold because even the
aggressive merging can fail from reducing the number of regions under
the user-defined upper limit.  For example, there could be many
user-defined non-contiguous regions that cannot be merged.

The threshold based loop break condition is evaluated by comparing the
threshold for the next merging try against the theoretical maximum
threshold.  If max_thres is larger than UINT_MAX / 2, doubling the
threshold could make it overflow, and bypass the loop break condition.
In the case, if the number of regions cannot be reduced under the upper
limit like explained above, the loop will run infinitely.

Prevent the case by doing the break condition check before doubling the
threshold.  Also, prevent the threshold exceeding the maximum threshold,
as it could overflow and apply the wrong merge threshold.

This issue is unlikely to occur in real world, since having the
max_thres higher than UINT_MAX / 2 require unrealistically large
aggregation intervals compared to the sampling interval.  Also, it
requires an unrealistically large number of uncontiguous regions setup.
Nonetheless, the consequence is bad and the fix is simple.

The issue was discovered [1] by Sashiko.

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

Fixes: 310d6c15e910 ("mm/damon/core: merge regions aggressively when max_nr_regions is unmet")
Cc: <stable@vger.kernel.org> # 6.10.x
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 806a67d02a6e9..f3b6a46fdaabd 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3369,15 +3369,20 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
 
 	max_thres = c->attrs.aggr_interval /
 		(c->attrs.sample_interval ?  c->attrs.sample_interval : 1);
-	do {
+	while (true) {
 		nr_regions = 0;
 		damon_for_each_target(t, c) {
 			damon_merge_regions_of(t, threshold, sz_limit, c);
 			nr_regions += damon_nr_regions(t);
 		}
-		threshold = max(1, threshold * 2);
-	} while (nr_regions > c->attrs.max_nr_regions &&
-			threshold / 2 < max_thres);
+		if (nr_regions <= c->attrs.max_nr_regions ||
+				max_thres <= threshold)
+			break;
+		if (threshold < max_thres / 2)
+			threshold = max(1, threshold * 2);
+		else
+			threshold = max_thres;
+	}
 }
 
 #ifdef CONFIG_DAMON_DEBUG_SANITY
-- 
2.47.3

[RFC PATCH v1.2 2/5] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of()

[SJ Park]

KUNIT_EXPECT_EQ() does not abort the execution of test code when the
expectation is not met.  But damon_test_merge_regions_of() code after
its initial KUNIT_EXPECT_EQ() call assumes the expectation is met.  It
does a per-region test with a hard-coded number of regions that is
correct only if the expectation was met.  As a result, __nth_region_of()
could return NULL, and the test code can dereference NULL pointers.  Fix
the issue by catching the expectation failure and skip the per-region
tests.

The user impact on realistic setups should be negligible, as it is a
unit test.

The issue was discovered [1] by Sashiko.

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

Fixes: 17ccae8bb5c9 ("mm/damon: add kunit tests")
Cc: <stable@vger.kernel.org> # 5.15.x
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/tests/core-kunit.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index 6ad73559dd8ea..a99363720e677 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -260,11 +260,14 @@ static void damon_test_merge_regions_of(struct kunit *test)
 	damon_merge_regions_of(t, 9, 9999, ctx);
 	/* 0-112, 114-130, 130-156, 156-170, 170-230, 230-10170 */
 	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 6u);
+	if (damon_nr_regions(t) != 6)
+		goto out;
 	for (i = 0; i < 6; i++) {
 		r = __nth_region_of(t, i);
 		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
 		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
 	}
+out:
 	damon_free_target(t);
 	damon_destroy_ctx(ctx);
 }
-- 
2.47.3

[RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization

[SJ Park]

The optimization can race when multiple kdamonds are running.
Meanwhile, the impact of the optimization is quite doubtful.  Just
remove it.

The user impact of the issue should be quite trivial.  After all, the
race can happen only when the user intentionally setup DAMON in the way.
Even if it happens, it would be rare and only degrade the best-effort
monitoring results.  No critical consequences like kernel panic or
memory corruption happen.

The race possibility was discovered [1] by Sashiko.

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

Fixes: 3f49584b262c ("mm/damon: implement primitives for the virtual memory address spaces")
Cc: <stable@vger.kernel.org> # 5.15.x
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/vaddr.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index d10b8042adb5b..d487b7a4a1042 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -383,8 +383,6 @@ static void damon_va_prepare_access_checks(struct damon_ctx *ctx)
 }
 
 struct damon_young_walk_private {
-	/* size of the folio for the access checked virtual memory address */
-	unsigned long *folio_sz;
 	bool young;
 };
 
@@ -411,7 +409,6 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
 					mmu_notifier_test_young(walk->mm,
 						addr))
 			priv->young = true;
-		*priv->folio_sz = HPAGE_PMD_SIZE;
 huge_out:
 		spin_unlock(ptl);
 		return 0;
@@ -430,7 +427,6 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
 	if (pte_young(ptent) || !folio_test_idle(folio) ||
 			mmu_notifier_test_young(walk->mm, addr))
 		priv->young = true;
-	*priv->folio_sz = folio_size(folio);
 out:
 	pte_unmap_unlock(pte, ptl);
 	return 0;
@@ -458,7 +454,6 @@ static int damon_young_hugetlb_entry(pte_t *pte, unsigned long hmask,
 	if (pte_young(entry) || !folio_test_idle(folio) ||
 	    mmu_notifier_test_young(walk->mm, addr))
 		priv->young = true;
-	*priv->folio_sz = huge_page_size(h);
 
 	folio_put(folio);
 
@@ -470,11 +465,9 @@ static int damon_young_hugetlb_entry(pte_t *pte, unsigned long hmask,
 #define damon_young_hugetlb_entry NULL
 #endif /* CONFIG_HUGETLB_PAGE */
 
-static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
-		unsigned long *folio_sz)
+static bool damon_va_young(struct mm_struct *mm, unsigned long addr)
 {
 	struct damon_young_walk_private arg = {
-		.folio_sz = folio_sz,
 		.young = false,
 	};
 
@@ -494,28 +487,17 @@ static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
  * r	the region to be checked
  */
 static void __damon_va_check_access(struct mm_struct *mm,
-				struct damon_region *r, bool same_target)
+				struct damon_region *r)
 {
-	static unsigned long last_addr;
-	static unsigned long last_folio_sz = PAGE_SIZE;
-	static bool last_accessed;
+	bool accessed;
 
 	if (!mm) {
 		damon_update_region_access_rate(r, false);
 		return;
 	}
 
-	/* If the region is in the last checked page, reuse the result */
-	if (same_target && (ALIGN_DOWN(last_addr, last_folio_sz) ==
-				ALIGN_DOWN(r->sampling_addr, last_folio_sz))) {
-		damon_update_region_access_rate(r, last_accessed);
-		return;
-	}
-
-	last_accessed = damon_va_young(mm, r->sampling_addr, &last_folio_sz);
-	damon_update_region_access_rate(r, last_accessed);
-
-	last_addr = r->sampling_addr;
+	accessed = damon_va_young(mm, r->sampling_addr);
+	damon_update_region_access_rate(r, accessed);
 }
 
 static unsigned int damon_va_check_accesses(struct damon_ctx *ctx)
@@ -524,15 +506,12 @@ static unsigned int damon_va_check_accesses(struct damon_ctx *ctx)
 	struct mm_struct *mm;
 	struct damon_region *r;
 	unsigned int max_nr_accesses = 0;
-	bool same_target;
 
 	damon_for_each_target(t, ctx) {
 		mm = damon_get_mm(t);
-		same_target = false;
 		damon_for_each_region(r, t) {
-			__damon_va_check_access(mm, r, same_target);
+			__damon_va_check_access(mm, r);
 			max_nr_accesses = max(r->nr_accesses, max_nr_accesses);
-			same_target = true;
 		}
 		if (mm)
 			mmput(mm);
-- 
2.47.3

[RFC PATCH v1.2 4/5] mm/damon/paddr: drop last same folio access check reuse optimization

[SJ Park]

It can race when multiple kdamonds are being used.  The problem from the
race is doubtful, but the gain from the optimization is also doubtful.
Simply drop the optimization in favor of code simplicity.

The user impact is doubtfully trivial.  After all, this kind of
interference can happen only by intentional user setup.  Even if it
happens, it will be rare, and the consequence is degradation of the
best-effort monitoring results.  No critical consequences like kernel
panic or memory corruption happen.

The race was discovered [1] by Sashiko.

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

Fixes: a28397beb55b ("mm/damon: implement primitives for physical address space monitoring")
Cc: <stable@vger.kernel.org> # 5.16.x
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/paddr.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index b85f88a7a38f4..e4f98d67461f5 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -65,7 +65,7 @@ static void damon_pa_prepare_access_checks(struct damon_ctx *ctx)
 	}
 }
 
-static bool damon_pa_young(phys_addr_t paddr, unsigned long *folio_sz)
+static bool damon_pa_young(phys_addr_t paddr)
 {
 	struct folio *folio = damon_get_folio(PHYS_PFN(paddr));
 	bool accessed;
@@ -74,7 +74,6 @@ static bool damon_pa_young(phys_addr_t paddr, unsigned long *folio_sz)
 		return false;
 
 	accessed = damon_folio_young(folio);
-	*folio_sz = folio_size(folio);
 	folio_put(folio);
 	return accessed;
 }
@@ -82,23 +81,12 @@ static bool damon_pa_young(phys_addr_t paddr, unsigned long *folio_sz)
 static void __damon_pa_check_access(struct damon_region *r,
 		unsigned long addr_unit)
 {
-	static phys_addr_t last_addr;
-	static unsigned long last_folio_sz = PAGE_SIZE;
-	static bool last_accessed;
+	bool accessed;
 	phys_addr_t sampling_addr = damon_pa_phys_addr(
 			r->sampling_addr, addr_unit);
 
-	/* If the region is in the last checked page, reuse the result */
-	if (ALIGN_DOWN(last_addr, last_folio_sz) ==
-				ALIGN_DOWN(sampling_addr, last_folio_sz)) {
-		damon_update_region_access_rate(r, last_accessed);
-		return;
-	}
-
-	last_accessed = damon_pa_young(sampling_addr, &last_folio_sz);
-	damon_update_region_access_rate(r, last_accessed);
-
-	last_addr = sampling_addr;
+	accessed = damon_pa_young(sampling_addr);
+	damon_update_region_access_rate(r, accessed);
 }
 
 static unsigned int damon_pa_check_accesses(struct damon_ctx *ctx)
-- 
2.47.3

[RFC PATCH v1.2 5/5] mm/damon/sysfs: read ops_id only once

[SJ Park]

damon_sysfs_apply_inputs() reads ops_id twice.  It could race with
ops_id_store().  As a result, the min_region_sz could wrongly be set up.
Read it once.

The user impact is trivial.  Sane users ain't update the parameter in
parallel.  Even if it happens, only monitoring itself runs differently
than expected.  No critical consequences like kernel panic or memory
corruption happen.

The issue was discovered [1] by Sashiko.

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

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/sysfs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index b5fe036f78015..60a1a9e4ada34 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -2094,14 +2094,16 @@ static inline bool damon_sysfs_kdamond_running(
 static int damon_sysfs_apply_inputs(struct damon_ctx *ctx,
 		struct damon_sysfs_context *sys_ctx)
 {
+	enum damon_ops_id ops_id;
 	int err;
 
-	err = damon_select_ops(ctx, sys_ctx->ops_id);
+	ops_id = READ_ONCE(sys_ctx->ops_id);
+	err = damon_select_ops(ctx, ops_id);
 	if (err)
 		return err;
 	ctx->addr_unit = sys_ctx->addr_unit;
 	/* addr_unit is respected by only DAMON_OPS_PADDR */
-	if (sys_ctx->ops_id == DAMON_OPS_PADDR)
+	if (ops_id == DAMON_OPS_PADDR)
 		ctx->min_region_sz = max(
 				DAMON_MIN_REGION_SZ / sys_ctx->addr_unit, 1);
 	ctx->pause = sys_ctx->pause;
-- 
2.47.3

Re: [RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete fix: identical data race in `mm/damon/paddr.c` causes access statistics corruption
--

--- Patch [3]: [RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization ---
commit d42bea4a157d94e45112b2190d4aba2a5eeffaa2
Author: SJ Park <sj@kernel.org>

mm/damon/vaddr: drop last same folio access check optimization

This patch removes the "last same folio access check" optimization in
vaddr.c to fix a data race when multiple kdamonds are running.

Link: https://lore.kernel.org/20260621204050.10993-1-sj@kernel.org

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index d10b8042adb5b..d487b7a4a1042 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c

[ ... ]

> @@ -494,28 +487,17 @@ static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
>   * r	the region to be checked
>   */
>  static void __damon_va_check_access(struct mm_struct *mm,
> -				struct damon_region *r, bool same_target)
> +				struct damon_region *r)
>  {
> -	static unsigned long last_addr;
> -	static unsigned long last_folio_sz = PAGE_SIZE;
> -	static bool last_accessed;

[Severity: High]
This isn't a bug introduced by this patch, but does the same data race still
exist in mm/damon/paddr.c?

Looking at __damon_pa_check_access() in mm/damon/paddr.c, it seems to have the
exact same caching optimization using static variables:

	static phys_addr_t last_addr;
	static unsigned long last_folio_sz = PAGE_SIZE;
	static bool last_accessed;

Since these static variables aren't protected by any locks, could concurrent
kdamond threads using DAMON_OPS_PADDR clobber each other's state in the exact
same way?

For example, if Thread A processes a 2MB huge page and updates last_folio_sz
to 2MB, could Thread B evaluate its next sampling_addr using Thread A's 2MB
last_folio_sz?

This would cause ALIGN_DOWN(last_addr, 2MB) == ALIGN_DOWN(sampling_addr, 2MB)
to incorrectly evaluate to true for up to 512 consecutive 4KB pages, assigning
them all the last_accessed value without actually checking them. Should this
identical optimization be removed from paddr.c as well to completely resolve
the data race?

> +	bool accessed;
>  
>  	if (!mm) {
>  		damon_update_region_access_rate(r, false);
>  		return;
>  	}
>  
> -	/* If the region is in the last checked page, reuse the result */
> -	if (same_target && (ALIGN_DOWN(last_addr, last_folio_sz) ==
> -				ALIGN_DOWN(r->sampling_addr, last_folio_sz))) {
> -		damon_update_region_access_rate(r, last_accessed);
> -		return;
> -	}
> -
> -	last_accessed = damon_va_young(mm, r->sampling_addr, &last_folio_sz);
> -	damon_update_region_access_rate(r, last_accessed);
> -
> -	last_addr = r->sampling_addr;
> +	accessed = damon_va_young(mm, r->sampling_addr);
> +	damon_update_region_access_rate(r, accessed);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713135838.32730-1-sj@kernel.org?part=3

Re: [RFC PATCH v1.2 2/5] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of()

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the fix is incomplete. The exact same vulnerability (NULL dereferences or out-of-bounds array accesses upon test expectation failure) exists in several other DAMON KUnit test functions. The patch correctly notes that `KUNIT_EXPECT_EQ()` does not abort execution, leading to NULL dereferences if subsequent code assumes the expectation was met. However, identical brittle patterns exist in `damon_do_test_apply_three_regions()` (which dereferences the result of `__nth_region_of()` without checking for NULL), `damos_test_commit_dests_for()` (which loops over `dst.nr_dests` and can dereference a `NULL` `src_node_id_arr`), and `damos_test_commit_quota_goals_for()` / `damon_test_commit_target_regions_for()` (which iterate based on the destination's size and can read out of bounds from the source arrays).
--

--- Patch [2]: [RFC PATCH v1.2 2/5] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of() ---
commit 5b98cb20c5802f828e4e3cf09a813b6c47d58774
Author: SJ Park <sj@kernel.org>

mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of()

KUNIT_EXPECT_EQ() does not abort the execution of test code when the
expectation is not met.  But damon_test_merge_regions_of() code after
its initial KUNIT_EXPECT_EQ() call assumes the expectation is met.

Link: https://lore.kernel.org/20260710144937.26981-1-sj@kernel.org

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 6ad73559dd8ea..a99363720e677 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -260,11 +260,14 @@ static void damon_test_merge_regions_of(struct kunit *test)
>  	damon_merge_regions_of(t, 9, 9999, ctx);
>  	/* 0-112, 114-130, 130-156, 156-170, 170-230, 230-10170 */
>  	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 6u);
> +	if (damon_nr_regions(t) != 6)
> +		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but does this fix need to be
applied to several other DAMON test functions that follow the same pattern?

If a test expectation fails, these functions also assume the expectation
was met, which can lead to NULL pointer dereferences or stack out-of-bounds
reads.

In mm/damon/tests/vaddr-kunit.h:damon_do_test_apply_three_regions():

    for (i = 0; i < nr_expected / 2; i++) {
        r = __nth_region_of(t, i);
        KUNIT_EXPECT_EQ(test, r->ar.start, expected[i * 2]);

If damon_set_regions() leaves fewer regions than expected,
__nth_region_of() returns NULL and r->ar.start dereferences it.

In mm/damon/tests/core-kunit.h:damos_test_commit_dests_for():

    for (i = 0; i < dst.nr_dests; i++) {
        KUNIT_EXPECT_EQ(test, dst.node_id_arr[i], src_node_id_arr[i]);

If dst.nr_dests is greater than src_nr_dests, it can access
src_node_id_arr[i] out of bounds (which can also be NULL).

In mm/damon/tests/core-kunit.h:damos_test_commit_quota_goals_for():

    damos_for_each_quota_goal(goal, (&dst)) {
        KUNIT_EXPECT_EQ(test, goal->metric, src_goals[i].metric);

This iterates over dst's goals without verifying against nr_src_goals,
potentially accessing the src_goals stack array out of bounds.

In mm/damon/tests/core-kunit.h:damon_test_commit_target_regions_for():

    damon_for_each_region(r, dst_target) {
        KUNIT_EXPECT_EQ(test, r->ar.start, expect_start_end[i][0]);

This blindly loops over dst_target's regions, leading to potential
out-of-bounds reads on expect_start_end if it exceeds nr_expect_regions.

Could these functions be updated as well to prevent the test suite
from oopsing on expectation failures?

>  	for (i = 0; i < 6; i++) {
>  		r = __nth_region_of(t, i);
>  		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
>  		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
>  	}
> +out:
>  	damon_free_target(t);
>  	damon_destroy_ctx(ctx);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713135838.32730-1-sj@kernel.org?part=2

Re: [RFC PATCH v1.2 2/5] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of()

[SJ Park]

On Mon, 13 Jul 2026 14:08:06 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the fix is incomplete. The exact same vulnerability (NULL dereferences or out-of-bounds array accesses upon test expectation failure) exists in several other DAMON KUnit test functions. The patch correctly notes that `KUNIT_EXPECT_EQ()` does not abort execution, leading to NULL dereferences if subsequent code assumes the expectation was met. However, identical brittle patterns exist in `damon_do_test_apply_three_regions()` (which dereferences the result of `__nth_region_of()` without checking for NULL), `damos_test_commit_dests_for()` (which loops over `dst.nr_dests` and can dereference a `NULL` `src_node_id_arr`), and `damos_test_commit_quota_goals_for()` / `damon_test_commit_target_regions_for()` (which iterate based on the destination's size and can read out of bounds from the source arrays).
> --
> 
> --- Patch [2]: [RFC PATCH v1.2 2/5] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of() ---
> commit 5b98cb20c5802f828e4e3cf09a813b6c47d58774
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of()
> 
> KUNIT_EXPECT_EQ() does not abort the execution of test code when the
> expectation is not met.  But damon_test_merge_regions_of() code after
> its initial KUNIT_EXPECT_EQ() call assumes the expectation is met.
> 
> Link: https://lore.kernel.org/20260710144937.26981-1-sj@kernel.org
> 
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index 6ad73559dd8ea..a99363720e677 100644
> > --- a/mm/damon/tests/core-kunit.h
> > +++ b/mm/damon/tests/core-kunit.h
> > @@ -260,11 +260,14 @@ static void damon_test_merge_regions_of(struct kunit *test)
> >  	damon_merge_regions_of(t, 9, 9999, ctx);
> >  	/* 0-112, 114-130, 130-156, 156-170, 170-230, 230-10170 */
> >  	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 6u);
> > +	if (damon_nr_regions(t) != 6)
> > +		goto out;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this fix need to be
> applied to several other DAMON test functions that follow the same pattern?
> 
> If a test expectation fails, these functions also assume the expectation
> was met, which can lead to NULL pointer dereferences or stack out-of-bounds
> reads.
> 
> In mm/damon/tests/vaddr-kunit.h:damon_do_test_apply_three_regions():
> 
>     for (i = 0; i < nr_expected / 2; i++) {
>         r = __nth_region_of(t, i);
>         KUNIT_EXPECT_EQ(test, r->ar.start, expected[i * 2]);
> 
> If damon_set_regions() leaves fewer regions than expected,
> __nth_region_of() returns NULL and r->ar.start dereferences it.
> 
> In mm/damon/tests/core-kunit.h:damos_test_commit_dests_for():
> 
>     for (i = 0; i < dst.nr_dests; i++) {
>         KUNIT_EXPECT_EQ(test, dst.node_id_arr[i], src_node_id_arr[i]);
> 
> If dst.nr_dests is greater than src_nr_dests, it can access
> src_node_id_arr[i] out of bounds (which can also be NULL).
> 
> In mm/damon/tests/core-kunit.h:damos_test_commit_quota_goals_for():
> 
>     damos_for_each_quota_goal(goal, (&dst)) {
>         KUNIT_EXPECT_EQ(test, goal->metric, src_goals[i].metric);
> 
> This iterates over dst's goals without verifying against nr_src_goals,
> potentially accessing the src_goals stack array out of bounds.
> 
> In mm/damon/tests/core-kunit.h:damon_test_commit_target_regions_for():
> 
>     damon_for_each_region(r, dst_target) {
>         KUNIT_EXPECT_EQ(test, r->ar.start, expect_start_end[i][0]);
> 
> This blindly loops over dst_target's regions, leading to potential
> out-of-bounds reads on expect_start_end if it exceeds nr_expect_regions.
> 
> Could these functions be updated as well to prevent the test suite
> from oopsing on expectation failures?

I didn't check each one, but sounds right to me.  I will further check and work
on those later.

> 
> >  	for (i = 0; i < 6; i++) {
> >  		r = __nth_region_of(t, i);
> >  		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
> >  		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
> >  	}
> > +out:
> >  	damon_free_target(t);
> >  	damon_destroy_ctx(ctx);
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713135838.32730-1-sj@kernel.org?part=2


Thanks,
SJ

Re: [RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization

[SJ Park]

On Mon, 13 Jul 2026 14:06:53 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Incomplete fix: identical data race in `mm/damon/paddr.c` causes access statistics corruption
> --
> 
> --- Patch [3]: [RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization ---
> commit d42bea4a157d94e45112b2190d4aba2a5eeffaa2
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/vaddr: drop last same folio access check optimization
> 
> This patch removes the "last same folio access check" optimization in
> vaddr.c to fix a data race when multiple kdamonds are running.
> 
> Link: https://lore.kernel.org/20260621204050.10993-1-sj@kernel.org
> 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index d10b8042adb5b..d487b7a4a1042 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> 
> [ ... ]
> 
> > @@ -494,28 +487,17 @@ static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
> >   * r	the region to be checked
> >   */
> >  static void __damon_va_check_access(struct mm_struct *mm,
> > -				struct damon_region *r, bool same_target)
> > +				struct damon_region *r)
> >  {
> > -	static unsigned long last_addr;
> > -	static unsigned long last_folio_sz = PAGE_SIZE;
> > -	static bool last_accessed;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the same data race still
> exist in mm/damon/paddr.c?
> 
> Looking at __damon_pa_check_access() in mm/damon/paddr.c, it seems to have the
> exact same caching optimization using static variables:
> 
> 	static phys_addr_t last_addr;
> 	static unsigned long last_folio_sz = PAGE_SIZE;
> 	static bool last_accessed;
> 
> Since these static variables aren't protected by any locks, could concurrent
> kdamond threads using DAMON_OPS_PADDR clobber each other's state in the exact
> same way?
> 
> For example, if Thread A processes a 2MB huge page and updates last_folio_sz
> to 2MB, could Thread B evaluate its next sampling_addr using Thread A's 2MB
> last_folio_sz?
> 
> This would cause ALIGN_DOWN(last_addr, 2MB) == ALIGN_DOWN(sampling_addr, 2MB)
> to incorrectly evaluate to true for up to 512 consecutive 4KB pages, assigning
> them all the last_accessed value without actually checking them. Should this
> identical optimization be removed from paddr.c as well to completely resolve
> the data race?

Yes, that's what the next patch in this series is fixing.

> 
> > +	bool accessed;
> >  
> >  	if (!mm) {
> >  		damon_update_region_access_rate(r, false);
> >  		return;
> >  	}
> >  
> > -	/* If the region is in the last checked page, reuse the result */
> > -	if (same_target && (ALIGN_DOWN(last_addr, last_folio_sz) ==
> > -				ALIGN_DOWN(r->sampling_addr, last_folio_sz))) {
> > -		damon_update_region_access_rate(r, last_accessed);
> > -		return;
> > -	}
> > -
> > -	last_accessed = damon_va_young(mm, r->sampling_addr, &last_folio_sz);
> > -	damon_update_region_access_rate(r, last_accessed);
> > -
> > -	last_addr = r->sampling_addr;
> > +	accessed = damon_va_young(mm, r->sampling_addr);
> > +	damon_update_region_access_rate(r, accessed);
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713135838.32730-1-sj@kernel.org?part=3


Thanks,
SJ