mm/damon: fix uninitialized DAMOS field and kunit exec expectation bugs

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

Pre-existing issues:
- [High] Incorrect PTE advancement for large folios in vaddr stat scanning and migration.
- [High] Commit of DAMON parameters corrupts the active scheme's quota state by zeroing dynamic fields in-place.
- [High] Failure in damos_commit_dests leaves the active scheme in an inconsistent state, leading to a NULL pointer dereference. --

--- Patch [1]: [PATCH 1/7] mm/damon/core: initialize damos->last_applied --- commit 90344a381581ba5ad7a7c87826b18bf948659516
Author: SJ Park <[email protected]>

mm/damon/core: initialize damos->last_applied

This commit fixes an issue where the damos->last_applied field was being used without initialization, leading to DAMON unexpectedly skipping the application of DAMOS actions for a folio the first time per scheme. It addresses this by initializing the field in damon_new_scheme().

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

mm/damon/core.c struct damos *damon_new_scheme(struct damos_access_pattern *pattern, · line 711

Quoted 12 lines

mm/damon/core.c +1 -0

index 603b102ff80f9..20d267c615faf 100644
@@ -705,6 +705,7 @@ struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
 	INIT_LIST_HEAD(&scheme->ops_filters);
 	scheme->stat = (struct damos_stat){};
 	scheme->max_nr_snapshots = 0;
+	scheme->last_applied = NULL;
 	INIT_LIST_HEAD(&scheme->list);
 
 	scheme->quota = *(damos_quota_init(quota));

                        ^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does this in-place zeroing corrupt the active scheme's quota state?

Looking at damon_new_scheme(), it passes the provided quota pointer to damos_quota_init(), which zeros out dynamic fields directly:

mm/damon/core.c:damos_quota_init() {
    ...
    quota->esz = 0;
    quota->total_charged_sz = 0;
    ...
}

When damon_commit_ctx() applies new parameters to an active context, it copies schemes by calling damon_new_scheme() with a pointer to the running context's scheme quota.

Could this wipe out the current quota charge window progress and reset the goal tuner's feedback loop (esz_bp) on every parameter commit, causing inconsistent tuning pressure?

[Severity: High]
This is a pre-existing issue, but can the failure path in damos_commit_dests() lead to a NULL pointer dereference?

In mm/damon/core.c:damos_commit_dests(), if memory allocation fails for dst->node_id_arr or dst->weight_arr, the function returns -ENOMEM:

mm/damon/core.c:damos_commit_dests() {
    ...
    dst->node_id_arr = kmalloc_array(src->nr_dests,
        sizeof(*dst->node_id_arr), GFP_KERNEL);
    if (!dst->node_id_arr) {
        dst->weight_arr = NULL;
        return -ENOMEM;
    }
    ...
}

However, dst->nr_dests is updated later. If the allocation fails while updating an active context during __damon_commit_ctx(), could the active scheme be left with weight_arr == NULL but a non-zero nr_dests?

If so, would this cause a kernel panic when kdamond resumes and calls damos_va_migrate_dests_add(), which iterates up to dests->nr_dests and unconditionally dereferences dests->weight_arr[i]?

[Severity: High]
This is a pre-existing issue, but does the PTE iteration in damos_va_stat_pmd_entry() and damos_va_migrate_pmd_entry() overshoot large folios?

In both functions, when a valid folio is found, the loop increment is set to the total number of pages in the folio:

mm/damon/vaddr.c:damos_va_stat_pmd_entry() {
    ...
    if (!damos_va_filter_out(s, folio, vma, addr, pte, NULL))
        *sz_filter_passed += folio_size(folio);
    nr = folio_nr_pages(folio);
    s->last_applied = folio;
    ...
}

If the current PTE maps a tail page of a large folio (for example, if the VMA starts in the middle of a large folio), wouldn't advancing pte += nr and addr += nr * PAGE_SIZE overshoot the end of the large folio?

Could this cause DAMON to skip subsequent PTEs in the mapped page table that might belong to entirely different folios, missing stats and migration actions for them?

Signature · 2 lines

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

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

Pre-existing issues:
- [High] Kernel memory corruption in damon_apply_min_nr_regions() due to unhandled split failure.
- [Medium] Incomplete test fix for memory allocation failure in damos_filter_match() leading to test crash.
- [Medium] Missing upper bound checks in test loops leading to Out-Of-Bounds array reads. --

--- Patch [2]: [PATCH 2/7] mm/damon/core-kunit: check region count before testing in split_at() --- commit c03465ac93de9ac544447d782c3c2ea15bf60ab1
Author: SJ Park <[email protected]>

mm/damon/core-kunit: check region count before testing in split_at()

damon_test_split_at() tests the next region that is assumed to be created by damon_split_region_at(). But the split might fail. In this case, the test is updated to check if the number of regions changed as expected and exit early.

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

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

Quoted 12 lines

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

index eba643762132f..322a295ac2cec 100644
@@ -155,6 +155,10 @@ static void damon_test_split_at(struct kunit *test)
 	r->age = 10;
 	damon_add_region(r, t);
 	damon_split_region_at(t, r, 25);
+	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 2);
+	if (damon_nr_regions(t) != 2)
+		goto out;
+

[Severity: Medium]
This isn't a bug introduced by this patch, but it looks like the exact same pattern being fixed here is present in damos_test_filter_out().

If damos_filter_match() fails to allocate memory, it returns without splitting the region. The test logs a failure for the region count mismatch but execution continues unconditionally:

mm/damon/tests/core-kunit.h:damos_test_filter_out() {
        /* filter should have split the region */
        ...
        KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 2);
        r2 = damon_next_region(r);
        ...
        damon_destroy_region(r2, t);
}

Since r is the only item in the list, does damon_next_region(r) resolve to an out-of-bounds pointer that is then passed to damon_destroy_region(), causing list and slab corruption during test execution?

[Severity: High]
This is a pre-existing issue, but since we are handling split failures in the tests, does mm/damon/core.c:damon_apply_min_nr_regions() suffer from a similar problem in production code?

mm/damon/core.c:damon_apply_min_nr_regions() {
        damon_for_each_target(t, ctx) {
                damon_for_each_region_safe(r, next, t) {
                        while (damon_sz_region(r) > max_region_sz) {
                                damon_split_region_at(t, r, max_region_sz);
                                r = damon_next_region(r);
                        }
                }
        }
}

If damon_split_region_at() fails to allocate memory, it silently returns. The loop then unconditionally advances r = damon_next_region(r). If r was the last region, this returns a bogus pointer derived from the list head.

The loop condition damon_sz_region(r) then evaluates garbage memory. If it happens to evaluate to greater than max_region_sz, the loop re-enters and passes the bogus pointer to damon_split_region_at(). Could this lead to out-of-bounds writes and list corruption under memory pressure?

[Severity: Medium]
This is also a pre-existing issue, but while reviewing the kunit tests, I noticed several test loops lack upper bound checks against the destination counts.

For example, in damos_test_commit_dests_for():

mm/damon/tests/core-kunit.h: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 the code under test incorrectly leaves dst.nr_dests greater than src_nr_dests (for instance, when src_nr_dests is 0), the loop iterates out of bounds and dereferences src_node_id_arr[i].

The same unbounded iteration pattern appears to exist in damos_test_commit_quota_goals_for() and damon_test_commit_target_regions_for(). Could these also be fixed to prevent out-of-bounds reads during test failures?

[ ... ]

Signature · 1 line

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

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

Pre-existing issues:
- [High] The patch adds an early exit check if (damon_nr_regions(t) != 2) goto out; to prevent an invalid pointer dereference when the first region split test fails, but misses the exact same vulnerability in the second region split test directly below it.
- [High] In damon_apply_min_nr_regions, if damon_split_region_at fails to allocate memory, the inner loop advances to the next list entry without inserting a new region, leading to an out-of-bounds read and write when it reaches the list head. --

--- Patch [4]: [PATCH 4/7] mm/damon/core-kunit: handle region split failure in filter_out() --- commit 705cf12a4e8501ec816031495df35460614c42c2
Author: SJ Park <[email protected]>

mm/damon/core-kunit: handle region split failure in filter_out()

damos_test_filter_out() checks if damos_filter_match() of an address filter splits the region as expected. But the test continued regardless of whether the split succeeded, which could lead to dereferencing invalid pointers or corrupting memory. This adds an early exit check on the first test block to prevent continuing on failure.

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

Quoted 14 lines

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

index 322a295ac2cec..bd3bbd421392f 100644
@@ -1365,6 +1365,8 @@ static void damos_test_filter_out(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, r->ar.start, 1);
 	KUNIT_EXPECT_EQ(test, r->ar.end, 2);
 	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 2);
+	if (damon_nr_regions(t) != 2)
+		goto out;
 	r2 = damon_next_region(r);
 	KUNIT_EXPECT_EQ(test, r2->ar.start, 2);
 	KUNIT_EXPECT_EQ(test, r2->ar.end, 4);

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

Quoted 3 lines

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

@@ -1384,6 +1386,7 @@ static void damos_test_filter_out(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, r2->ar.end, 8);
 	damon_destroy_region(r2, t);
 

[Severity: High]
This isn't a bug introduced by this patch, but does this second region split test have the same vulnerability as the one fixed above?

If damos_filter_match() fails to split the region in this second test block (e.g. due to memory allocation failure), damon_nr_regions(t) would remain 1. r2 would be assigned damon_next_region(r), which could incorrectly resolve to the damon_target list head.

Calling damon_destroy_region(r2, t) would then call kmem_cache_free() on this out-of-bounds pointer, potentially corrupting memory. Should another early exit check be added here as well?

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

Quoted 5 lines

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

+out:
 	damon_free_target(t);
 	damos_free_filter(f);
 }

[Severity: High]
This is a pre-existing issue, but while looking at region splits, could a similar unchecked failure cause memory corruption in mm/damon/core.c?

In damon_apply_min_nr_regions():

			while (damon_sz_region(r) > max_region_sz) {
				damon_split_region_at(t, r, max_region_sz);
				r = damon_next_region(r);
			}

If damon_split_region_at() fails due to memory pressure, it does not split the region. The call to damon_next_region(r) then erroneously advances r to the next element.

If r was the last region, it becomes the list head (&t->regions_list). The loop condition then evaluates damon_sz_region(r) on the list head, which reads memory 32 bytes before the damon_target struct allocation. If this difference exceeds max_region_sz, damon_split_region_at() could be called on the list head, which might subsequently write to r->ar.end, corrupting SLUB metadata or preceding objects.

Should damon_split_region_at() return an error code so callers can verify if the split actually succeeded?

Signature · 2 lines

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

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

Pre-existing issues:
- [Medium] Missing error check in damon_test_commit_target_regions_for() leads to an out-of-bounds read if damon_commit_target_regions() fails. --

--- Patch [6]: [PATCH 6/7] mm/damon/core-kunit: skip wrong quota goal walk in commit_quota_goals() --- commit 9bf8891739c4d265498b06b272b3083da8b65ed2
Author: SJ Park <[email protected]>

mm/damon/core-kunit: skip wrong quota goal walk in commit_quota_goals()

This commit fixes an out-of-bounds access in damos_test_commit_quota_goals_for() by verifying the number of quota goals matches the expected amount before traversing them. This prevents test setups from crashing if damos_commit_quota_goals() creates an unexpected number of goals.

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

Quoted 2 lines

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

[ ... ]

Quoted 5 lines

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

@@ -861,6 +862,14 @@ static void damos_test_commit_quota_goals_for(struct kunit *test,
 
 	damos_commit_quota_goals(&dst, &src);
 
+	damos_for_each_quota_goal(goal, &dst)

mm/damon/tests/core-kunit.h static void damos_test_commit_quota_goals_for(struct kunit *test, · line 875

Quoted 10 lines

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

+		nr_dst++;
+	damos_for_each_quota_goal(goal, &src)
+		nr_src++;
+	KUNIT_EXPECT_EQ(test, nr_dst, nr_src);
+	if (nr_dst != nr_src)
+		goto out;
+
 	i = 0;
 	damos_for_each_quota_goal(goal, (&dst)) {
 		KUNIT_EXPECT_EQ(test, goal->metric, src_goals[i].metric);

[Severity: Medium]
This is a pre-existing issue, but since this patch fixes this exact pattern for quota goals, does damon_test_commit_target_regions_for() need a similar fix for target regions?

If damon_commit_target_regions() fails (for example, due to a memory allocation failure), it returns an error code and leaves dst_target's regions unmodified. The test code currently ignores the return value and blindly iterates over dst_target:

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

	damon_commit_target_regions(dst_target, src_target, 1);
	i = 0;
	damon_for_each_region(r, dst_target) {
		KUNIT_EXPECT_EQ(test, r->ar.start, expect_start_end[i][0]);
		KUNIT_EXPECT_EQ(test, r->ar.end, expect_start_end[i][1]);
		i++;
	}

...
}

In a case where the original number of destination regions is larger than the number of expected regions, could this loop execute more times than the size of expect_start_end[] and cause an out-of-bounds read on the stack array?

Signature · 2 lines

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

Fix a few Sashiko-found unurgent bugs. Patch 1 fixes use of uninitialized damos->last_applied field. Patches 2-7 fix DAMON kunit tests that do invalid memory access under test failures.