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

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

[SJ Park]

damon_do_test_apply_three_regions() iterates regions after
damon_set_regions() call assuming the function would succeed at setting
the number of regions the same to the expected one.  It might have failed.
In this case, __nth_region_of() in the iteration could return NULL and
NULL dereference can happen in the test.

The consequent user impact (NULL dereference) is quite bad.  The realistic
user impact would be limited, though.  It would affect only test run
setups.

Fix it by testing if the number of regions was also changed as expected
and exit early for the failure.

The issue was discovered [1] by Sashiko.

Link: https://lore.kernel.org/20260718001442.87129-4-sj@kernel.org
Link: https://lore.kernel.org/20260713144757.39740-1-sj@kernel.org [1]
Fixes: 17ccae8bb5c9 ("mm/damon: add kunit tests")
Signed-off-by: SJ Park <sj@kernel.org>
Cc: Brendan Higgins <brendan.higgins@linux.dev>
Cc: <stable@vger.kernel.org> # 5.15.x
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
(cherry picked from commit 5fea07e460874c8c7cf00f728efbe22abc62c8d8)
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/vaddr-test.h | 5 +++++
 1 file changed, 5 insertions(+)

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;
+
 	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);
 }
 
-- 
2.47.3

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

[sashiko-bot]

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

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

[Sasha Levin]

> damon_do_test_apply_three_regions() iterates regions after
> damon_set_regions() call assuming the function would succeed at setting
> the number of regions the same to the expected one.  It might have failed.

Queued for 5.15, thanks.

-- 
Thanks,
Sasha