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

On Mon, 13 Jul 2026 14:08:06 +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 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 <[email protected]>
> 
> 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/[email protected]
> 
> > 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/[email protected]?part=2


Thanks,
SJ