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

On Wed, 15 Jul 2026 03:19:07 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Medium] The patch correctly identifies that `KUNIT_EXPECT_EQ()` does not abort test execution on failure, which causes undefined behavior in subsequent code if the test expectation is not met. However, it leaves the exact same anti-pattern unfixed in several other test helper functions within the DAMON test suite. This can lead to out-of-bounds (OOB) array reads, NULL pointer dereferences, and slab corruption when test failures occur.
> --
> 
> --- Patch [2]: [PATCH v1.1 2/6] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of() ---
> commit ce2690fbbc97573d0a06744653f652063e18c91b
> Author: SJ Park <[email protected]>
> Date:   Wed Jul 15 03:09:57 2026 +0000
> 
> mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of()
> 
> This commit fixes an issue where KUNIT_EXPECT_EQ() failures do not abort
> the test, leading to potential NULL pointer dereferences in subsequent
> code. It addresses this in damon_test_merge_regions_of() by explicitly
> checking the expectation and skipping the rest of the test if it fails.
> 
> Link: https://lore.kernel.org/[email protected]
> 
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index 485472ddebd19..eba643762132f 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, 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);
> >  }
> 
> [Severity: Medium]
> This is a pre-existing issue, but while you are fixing this anti-pattern
> here, the exact same missing abort condition exists in several other
> functions within the DAMON test suite.
> 
> For example, in 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 expectation fails, could this loop read out-of-bounds on
> src_node_id_arr since it iterates based on dst.nr_dests?
> 
> Similarly, in mm/damon/tests/core-kunit.h:damon_test_split_at():
> 
>     r_new = damon_next_region(r);
>     KUNIT_EXPECT_EQ(test, r_new->ar.start, 25ul);
> 
> Could damon_next_region() yield an invalid pointer if the split fails,
> causing a crash when r_new->ar.start is accessed unconditionally?
> 
> The same unbounded array access or unsafe pointer dereference pattern
> appears to exist in:
> - mm/damon/tests/core-kunit.h:damos_test_commit_quota_goals_for()
> - mm/damon/tests/core-kunit.h:damon_test_commit_target_regions_for()
> - mm/damon/tests/core-kunit.h:damos_test_filter_out()
> - mm/damon/tests/vaddr-kunit.h:damon_do_test_apply_three_regions()
> 
> Would it make sense to update these remaining occurrences in a follow-up or
> v2 of this patch to prevent slab corruption or NULL pointer dereferences
> during test failures?

I will fix those as a followup.

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


Thanks,
SJ