Re: [RFC PATCH v1.2 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 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?

>  	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