Re: [PATCH 2/7] mm/damon/core-kunit: check region count before testing in split_at()

On Fri, 17 Jul 2026 00:44:17 +0000 [email protected] wrote:

> 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]
> 
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index eba643762132f..322a295ac2cec 100644
> > --- a/mm/damon/tests/core-kunit.h
> > +++ b/mm/damon/tests/core-kunit.h
> > @@ -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?

That's what fixed by a later patch of this series.

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

Good finding, not necessarily a blocker of this series.  I will separately work
on this.

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

Those are fixed by the later patches of this series.

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


Thanks,
SJ