Re: [PATCH 5.15.y] mm/damon/vaddr-kunit: check region count in three_regions test

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing checks for expected region counts in DAMON kunit tests cause NULL pointer dereferences and out-of-bounds accesses.
- [High] Missing NULL checks for `damon_new_ctx()` allocations in KUnit tests lead to kernel oopses.
--

commit 18eda53adc9f6ea23835c180d70b57969be3e82f
Author: SJ Park <sj@kernel.org>

    mm/damon/vaddr-kunit: check region count in three_regions test
    
    damon_do_test_apply_three_regions() iterates regions after a setup call
    assuming it would succeed at setting the number of regions to the expected
    one. Fix it by testing if the number of regions was changed as expected
    and exit early for the failure to prevent a NULL dereference.
    
    Link: https://lore.kernel.org/20260718001442.87129-4-sj@kernel.org
    Link: https://lore.kernel.org/20260713144757.39740-1-sj@kernel.org [1]

> diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
> index 72206dda95ecf..ffa76998dd63c 100644
> --- a/mm/damon/vaddr-test.h
> +++ b/mm/damon/vaddr-test.h
> @@ -155,12 +155,17 @@ static void damon_do_test_apply_three_regions(struct kunit *test,
>  
>  	damon_va_apply_three_regions(t, three_regions);
>  
> +	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), nr_expected / 2);
> +	if (damon_nr_regions(t) != nr_expected / 2)
> +		goto out;
> +

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a missing NULL check
for the damon_new_ctx() allocation earlier in this same function:

mm/damon/vaddr-test.h:damon_do_test_apply_three_regions() {
	struct damon_ctx *ctx = damon_new_ctx();
	...
	damon_add_target(ctx, t);
}

If damon_new_ctx() fails under memory pressure, does this cause a NULL pointer
dereference in damon_add_target()?

Similarly, does damon_test_split_evenly() miss a NULL check for
damon_new_ctx()?

mm/damon/vaddr-test.h:damon_test_split_evenly() {
	struct damon_ctx *c = damon_new_ctx();
	...
	damon_destroy_ctx(c);
}

Will damon_destroy_ctx() unconditionally dereference it if c is NULL?

[Severity: High]
This is a pre-existing issue, but it appears there are identical missing
region count checks in other test functions that can lead to NULL pointer
dereferences or out-of-bounds accesses.

For instance, in mm/damon/core-test.h:damon_test_merge_regions_of():

	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
	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]);
	}

If the regions are fewer than expected, will __nth_region_of() return NULL
and be unconditionally dereferenced?

Similarly, in mm/damon/core-test.h:damon_test_split_at():

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

If damon_split_region_at() fails, r remains the only element, so does
damon_next_region(r) resolve to the list head offset, leading to an
out-of-bounds read on r->ar.start?

And in mm/damon/vaddr-test.h:damon_test_split_evenly_succ():

	damon_for_each_region(r, t) {
		if (i == nr_pieces - 1)
			break;
		KUNIT_EXPECT_EQ(test,
				r->ar.start, start + i++ * expected_width);
		KUNIT_EXPECT_EQ(test, r->ar.end, start + i * expected_width);
	}
	KUNIT_EXPECT_EQ(test, r->ar.start, start + i * expected_width);

If damon_va_evenly_split_region() fails, does the loop terminate normally
without hitting the break condition, causing an out-of-bounds read when
accessing r->ar.start after the loop?

>  	for (i = 0; i < nr_expected / 2; i++) {
>  		r = __nth_region_of(t, i);
>  		KUNIT_EXPECT_EQ(test, r->ar.start, expected[i * 2]);
>  		KUNIT_EXPECT_EQ(test, r->ar.end, expected[i * 2 + 1]);
>  	}
>  
> +out:
>  	damon_destroy_ctx(ctx);
>  }

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