Re: [PATCH 6.1.y] mm/damon/tests/core-kunit: catch test failure in test_merge_regions_of()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing failure check in `damon_test_split_at()` assumes `damon_split_region_at()` always succeeds, leading to a heap out-of-bounds read if it fails.
- [High] Missing failure check in `damon_test_set_regions()` can lead to a stack out-of-bounds array read if `damon_set_regions()` creates more than 3 regions.
--

commit 734d67a5ef8e651363b657168887a0a497532892
Author: SJ Park <sj@kernel.org>

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. This commit fixes a potential NULL pointer
dereference in damon_test_merge_regions_of() by explicitly checking the
number of regions and skipping the per-region test if it doesn't match
the expected value. The user impact on realistic setups should be negligible
as it is a unit test.

Link: https://lore.kernel.org/20260715031002.108504-3-sj@kernel.org
Link: https://lore.kernel.org/20260710144937.26981-1-sj@kernel.org [1]

> diff --git a/mm/damon/core-test.h b/mm/damon/core-test.h
> index d8fef225930a5..094714172e581 100644
> --- a/mm/damon/core-test.h
> +++ b/mm/damon/core-test.h
> @@ -253,11 +253,14 @@ static void damon_test_merge_regions_of(struct kunit *test)
>  	damon_merge_regions_of(t, 9, 9999);
>  	/* 0-112, 114-130, 130-156, 156-170 */
>  	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
> +	if (damon_nr_regions(t) != 5)
> +		goto out;
>  	for (i = 0; i < 5; 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);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but since the patch specifically
aims to fix missing test-failure aborts, there is a similar missing failure
check in damon_test_split_at() that can lead to a heap out-of-bounds read:

mm/damon/core-test.h:damon_test_split_at() {
    ...
    damon_split_region_at(t, r, 25);
    KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
    KUNIT_EXPECT_EQ(test, r->ar.end, 25ul);

    r = damon_next_region(r);
    KUNIT_EXPECT_EQ(test, r->ar.start, 25ul);
    ...
}

If memory allocation fails internally in damon_split_region_at(), the region
is not split. KUNIT_EXPECT_EQ() records a failure but does not abort
execution.

Will the test unconditionally execute r = damon_next_region(r) in this
scenario?

Because the region was not split and remains the only element,
damon_next_region() evaluates to an invalid list head pointer. Could
dereferencing this to read r->ar.start cause a heap out-of-bounds read?

[Severity: High]
This is a pre-existing issue, but it shares the exact anti-pattern that the
patch fixes nearby. Another identical omission exists in
damon_test_set_regions():

mm/damon/core-test.h:damon_test_set_regions() {
    ...
    damon_set_regions(t, &range, 1);

    KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 3);
    damon_for_each_region(r, t) {
        KUNIT_EXPECT_EQ(test, r->ar.start, expects[expect_idx++]);
        KUNIT_EXPECT_EQ(test, r->ar.end, expects[expect_idx++]);
    }
    ...
}

If a logic bug in damon_set_regions() outputs 4 or more regions, the test
continues without aborting.

Since the test unconditionally loops over all regions via
damon_for_each_region(r, t) and accesses the 6-element static array expects,
can this exceed the array bounds?

Would using expects[expect_idx++] twice per loop lead to a stack out-of-bounds
read if more than 3 regions exist?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909034257.139428-1-sj@kernel.org?part=1