mm/damon: unurgent fixes for infinite loop, NULL de-ref and races

PATCH In review SJ Park 2 participants 11 messages Jul 14

Subsystems (4)

Stable branch Memory management Kselftest DAMON

lore

Share & tools

lore

SJ Park

author maintainer

2026-07-14 13:52 UTC #0 lore raw

cover mm/damon: unurgent fixes for infinite loop, NULL de-ref and races

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.2
- RFC v1.2: https://lore.kernel.org/[email protected]
- Drop RFC tag.
- Rebase to latest mm-new.
Changes from RFC v1.1
- RFC v1.1: https://lore.kernel.org/[email protected]
- Remove same_target param from __damon_va_check_access(). Changes from RFC
- RFC: https://lore.kernel.org/[email protected]
- 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: 93cecef8d85fe01bc004e07591501e85c132b343

Signature · 1 line

2.47.3

SJ Park

author maintainer

2026-07-14 13:52 UTC #1 lore raw

PATCH 1/5 mm/damon/core: avoid infinite kdamond_merge_regions() internal loop

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/[email protected]

Fixes 310d6c15e910 ("mm/damon/core: merge regions aggressively when max_nr_regions is unmet")

Cc <[email protected]> # 6.10.x

Signed-off-by SJ Park <[email protected]>

---
 mm/damon/core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

mm/damon/core.c +9 -4

index 6c4215cc809ec..603b102ff80f9 100644
@@ -3372,7 +3372,7 @@ 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,

@@ -3380,9 +3380,14 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
 			nr_regions += damon_nr_regions(t);
 		}
 		count_age = false;
-		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

Signature · 1 line

2.47.3

SJ Park

author maintainer

2026-07-14 13:52 UTC #2 lore raw

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

In reply to 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/[email protected]

Fixes 17ccae8bb5c9 ("mm/damon: add kunit tests")

Cc <[email protected]> # 5.15.x

Signed-off-by SJ Park <[email protected]>

---
 mm/damon/tests/core-kunit.h | 3 +++
 1 file changed, 3 insertions(+)

mm/damon/tests/core-kunit.h +3 -0

index 485472ddebd19..eba643762132f 100644
@@ -260,11 +260,14 @@ static void damon_test_merge_regions_of(struct kunit *test)
 	damon_merge_regions_of(t, 9, 9999, ctx, true);
 	/* 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);
 }

Signature · 1 line

2.47.3

[email protected]

AI review

2026-07-14 14:01 UTC #3 lore raw

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

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 patch attempts to fix a crash where unit tests continue execution despite a failed KUNIT_EXPECT_EQ() condition and misses several identical vulnerable patterns in the same file. In multiple other test functions, if a tested function fails and the KUNIT expectation is not met, the tests will continue and perform out-of-bounds array accesses or invalid list traversals leading to slab corruption. --

--- Patch [2]: [PATCH 2/5] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of() --- commit 951f8b5681b80f05d0ef2d36767a3d72e546ac33
Author: SJ Park <[email protected]>

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

This commit addresses a panic in damon_test_merge_regions_of(). If the initial KUNIT_EXPECT_EQ() expectation is not met, the code previously continued executing a per-region test assuming the incorrect hard-coded number of regions, leading to a NULL pointer dereference. The fix catches the expectation failure and skips the per-region tests.

Link https://lore.kernel.org/[email protected] open here

mm/damon/tests/core-kunit.h static void damon_test_merge_regions_of(struct kunit *test) · line 264

Quoted 10 lines

mm/damon/tests/core-kunit.h +2 -0

index 485472ddebd19..eba643762132f 100644
@@ -260,11 +260,14 @@ static void damon_test_merge_regions_of(struct kunit *test)
 	damon_merge_regions_of(t, 9, 9999, ctx, true);
 	/* 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 while addressing the crash here, could we also fix identical pre-existing regressions in the rest of this file?

In several other test functions, if a tested function fails and the KUNIT_EXPECT_EQ() condition is not met, execution continues and causes out-of-bounds array accesses or invalid pointer dereferences.

For example, in damos_test_commit_dests_for():

	KUNIT_EXPECT_EQ(test, dst.nr_dests, src_nr_dests);
	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 larger than src_nr_dests, doesn't this loop read out of bounds on src_node_id_arr[]?

Similarly, in damon_test_split_at() and damos_test_filter_out():

	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 2);
	r2 = damon_next_region(r);
	KUNIT_EXPECT_EQ(test, r2->ar.start, 2);
	KUNIT_EXPECT_EQ(test, r2->ar.end, 4);
	damon_destroy_region(r2, t);

If the split fails and there is only 1 region, does damon_next_region() return an invalid pointer for the list head? If so, passing it to damon_destroy_region() could lead to slab corruption.

A similar pattern also exists in damon_test_commit_target_regions_for() and damos_test_commit_quota_goals_for(). Should similar expectation checks and early exits be applied to these functions to prevent kernel panics during test failures?

Quoted 9 lines

mm/damon/tests/core-kunit.h +1 -0

 	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);
 }

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

SJ Park

author maintainer

2026-07-14 14:24 UTC #4 lore raw

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

Quoted 57 lines · On Tue, 14 Jul 2026 14:01:45 +0000 [email protected] 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 patch attempts to fix a crash where unit tests continue execution despite a failed KUNIT_EXPECT_EQ() condition and misses several identical vulnerable patterns in the same file. In multiple other test functions, if a tested function fails and the KUNIT expectation is not met, the tests will continue and perform out-of-bounds array accesses or invalid list traversals leading to slab corruption. --
--- Patch [2]: [PATCH 2/5] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of() --- commit 951f8b5681b80f05d0ef2d36767a3d72e546ac33
Author: SJ Park <[email protected]>
mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of()
This commit addresses a panic in damon_test_merge_regions_of(). If the initial KUNIT_EXPECT_EQ() expectation is not met, the code previously continued executing a per-region test assuming the incorrect hard-coded number of regions, leading to a NULL pointer dereference. The fix catches the expectation failure and skips the per-region tests.
Link https://lore.kernel.org/[email protected] open here
mm/damon/tests/core-kunit.h static void damon_test_merge_regions_of(struct kunit *test) · line 264
Quoted 10 lines
mm/damon/tests/core-kunit.h +2 -0
index 485472ddebd19..eba643762132f 100644
@@ -260,11 +260,14 @@ static void damon_test_merge_regions_of(struct kunit *test)
 	damon_merge_regions_of(t, 9, 9999, ctx, true);
 	/* 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 while addressing the crash here, could we also fix identical pre-existing regressions in the rest of this file?
In several other test functions, if a tested function fails and the KUNIT_EXPECT_EQ() condition is not met, execution continues and causes out-of-bounds array accesses or invalid pointer dereferences.
For example, in damos_test_commit_dests_for():
	KUNIT_EXPECT_EQ(test, dst.nr_dests, src_nr_dests);
	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 larger than src_nr_dests, doesn't this loop read out of bounds on src_node_id_arr[]?
Similarly, in damon_test_split_at() and damos_test_filter_out():
	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 2);
	r2 = damon_next_region(r);
	KUNIT_EXPECT_EQ(test, r2->ar.start, 2);
	KUNIT_EXPECT_EQ(test, r2->ar.end, 4);
	damon_destroy_region(r2, t);

Quoted 8 lines

If the split fails and there is only 1 region, does damon_next_region() return an invalid pointer for the list head? If so, passing it to damon_destroy_region() could lead to slab corruption.
A similar pattern also exists in damon_test_commit_target_regions_for() and damos_test_commit_quota_goals_for(). Should similar expectation checks and early exits be applied to these functions to prevent kernel panics during test failures?

Same class of issues were reported before, and I'm separately working on this.

Quoted 13 lines

Quoted 10 lines

mm/damon/tests/core-kunit.h +1 -0

 	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);
 }

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Thanks,
SJ

SJ Park

author maintainer

2026-07-14 13:52 UTC #5 lore raw

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

In reply to 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/[email protected]

Fixes 3f49584b262c ("mm/damon: implement primitives for the virtual memory address spaces")

Cc <[email protected]> # 5.15.x

Signed-off-by SJ Park <[email protected]>

---
 mm/damon/vaddr.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)

mm/damon/vaddr.c +6 ~~-27~~

index d10b8042adb5b..d487b7a4a1042 100644
@@ -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);

Signature · 1 line

2.47.3

SJ Park

author maintainer

2026-07-14 13:52 UTC #6 lore raw

PATCH 4/5 mm/damon/paddr: drop last same folio access check reuse optimization

In reply to 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/[email protected]

Fixes a28397beb55b ("mm/damon: implement primitives for physical address space monitoring")

Cc <[email protected]> # 5.16.x

Signed-off-by SJ Park <[email protected]>

---
 mm/damon/paddr.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

mm/damon/paddr.c +4 ~~-16~~

index b85f88a7a38f4..e4f98d67461f5 100644
@@ -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)

Signature · 1 line

2.47.3

SJ Park

author maintainer

2026-07-14 13:52 UTC #7 lore raw

PATCH 5/5 mm/damon/sysfs: read ops_id only once

In reply to 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/[email protected]

Signed-off-by SJ Park <[email protected]>

---
 mm/damon/sysfs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/damon/sysfs.c +4 -2

index b5fe036f78015..60a1a9e4ada34 100644
@@ -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;

Signature · 1 line

2.47.3

[email protected]

AI review

2026-07-14 14:25 UTC #8 lore raw

Re: [PATCH 5/5] mm/damon/sysfs: read ops_id only once

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch implements an incomplete fix for a TOCTOU data race. It correctly uses READ_ONCE() for ops_id to prevent min_region_sz corruption, but leaves an identical unresolved race condition for addr_unit immediately below it. --

--- Patch [5]: [PATCH 5/5] mm/damon/sysfs: read ops_id only once --- commit f0f9d921acb64429b2f4e2895a8088cc3b5477d5
Author: SJ Park <[email protected]>

mm/damon/sysfs: read ops_id only once

This commit addresses a time-of-check to time-of-use race condition in damon_sysfs_apply_inputs() by reading ops_id only once. This prevents a concurrent sysfs write from causing min_region_sz to be incorrectly configured.

Link https://lore.kernel.org/[email protected] open here

Quoted 5 lines

mm/damon/sysfs.c +0 -0

index b5fe036f78015..60a1a9e4ada34 100644
@@ -2094,14 +2094,16 @@ static int damon_sysfs_apply_inputs(struct damon_ctx *ctx,

[ ... ]

mm/damon/sysfs.c static inline bool damon_sysfs_kdamond_running( · line 2108

Quoted 6 lines

mm/damon/sysfs.c +1 -1

 	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);

[Severity: Medium]
Is there a similar race condition remaining for sys_ctx->addr_unit?

While this patch successfully fixes the race for ops_id, sys_ctx->addr_unit is read twice sequentially in this same block of code without protection.

A concurrent sysfs write to addr_unit between the assignment to ctx->addr_unit and the division in the max() calculation could cause DAMON to inconsistently calculate min_region_sz, degrading monitoring precision.

Should addr_unit also be read only once using READ_ONCE() here to fully prevent the race?

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

SJ Park

author maintainer

2026-07-14 14:29 UTC #9 lore raw

Re: [PATCH 5/5] mm/damon/sysfs: read ops_id only once

Quoted 38 lines · On Tue, 14 Jul 2026 14:25:08 +0000 [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch implements an incomplete fix for a TOCTOU data race. It correctly uses READ_ONCE() for ops_id to prevent min_region_sz corruption, but leaves an identical unresolved race condition for addr_unit immediately below it. --
--- Patch [5]: [PATCH 5/5] mm/damon/sysfs: read ops_id only once --- commit f0f9d921acb64429b2f4e2895a8088cc3b5477d5
Author: SJ Park <[email protected]>
mm/damon/sysfs: read ops_id only once
This commit addresses a time-of-check to time-of-use race condition in damon_sysfs_apply_inputs() by reading ops_id only once. This prevents a concurrent sysfs write from causing min_region_sz to be incorrectly configured.
Link https://lore.kernel.org/[email protected] open here
Quoted 5 lines
mm/damon/sysfs.c +0 -0
index b5fe036f78015..60a1a9e4ada34 100644
@@ -2094,14 +2094,16 @@ static int damon_sysfs_apply_inputs(struct damon_ctx *ctx,
[ ... ]
mm/damon/sysfs.c static inline bool damon_sysfs_kdamond_running( · line 2108
Quoted 6 lines
mm/damon/sysfs.c +1 -1
 	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);
[Severity: Medium]
Is there a similar race condition remaining for sys_ctx->addr_unit?
While this patch successfully fixes the race for ops_id, sys_ctx->addr_unit is read twice sequentially in this same block of code without protection.

Quoted 6 lines

A concurrent sysfs write to addr_unit between the assignment to ctx->addr_unit and the division in the max() calculation could cause DAMON to inconsistently calculate min_region_sz, degrading monitoring precision.
Should addr_unit also be read only once using READ_ONCE() here to fully prevent the race?

Good catch. I also found I forgot adding Fixes: tag. I'll fix this in the next revision.

Quoted 3 lines

Signature · 2 lines

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

Thanks,
SJ

SJ Park

author maintainer

2026-07-14 14:32 UTC #10 lore raw

In reply to SJ Park

Quoted 7 lines · On Tue, 14 Jul 2026 06:52:28 -0700 SJ Park <[email protected]> wrote:

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.

Sashiko found one more bug that may better to be fixed together with this series. Also patch 5 mistakenly lacks its Fixes: tag. I will post a new version of this series with the fixes. Please don't pick this series into mm-new for now.

Thanks,
SJ

[...]