Re: [RFC PATCH 2/6] mm/damon/tests/core-kunit: add damon_commit_probes() test

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing cleanup in the error path of `damon_test_commit_probes_for` causes a memory leak of dynamically allocated test structures.
- [Medium] Missing bounds check in `damon_test_commit_probes_for` can lead to a NULL pointer dereference when a test assertion fails.
--

--- Patch [2]: [RFC PATCH 2/6] mm/damon/tests/core-kunit: add damon_commit_probes() test ---
commit 490c9d2d6b4090811aaa428df3f6d9ce89e2d406
Author: SJ Park <[email protected]>

mm/damon/tests/core-kunit: add damon_commit_probes() test

Add kunit test to ensure damon_commit_probes() updates destination DAMON
context with source probes as expected.

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 6cbe22accabfb..6577410ace3da 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h

[ ... ]

> +static void damon_test_commit_probes_for(struct kunit *test,
> +		unsigned int dst_weights[], int nr_dst_probes,
> +		unsigned int src_weights[], int nr_src_probes)
> +{
> +	struct damon_ctx *dst, *src;
> +	int err;
> +	struct damon_probe *dst_probe, *src_probe;
> +
> +	dst = damon_test_help_setup_probes(dst_weights, nr_dst_probes);
> +	if (!dst)
> +		kunit_skip(test, "dst alloc fail");
> +	src = damon_test_help_setup_probes(src_weights, nr_src_probes);
> +	if (!src) {
> +		damon_destroy_ctx(dst);
> +		kunit_skip(test, "src alloc fail");
> +	}
> +
> +	err = damon_commit_probes(dst, src);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	if (err)
> +		return;

[Severity: Medium]
Does this error path leak the dst and src context structures?

If damon_commit_probes() returns an error, the early return here skips the
cleanup calls to damon_destroy_ctx(dst) and damon_destroy_ctx(src) at the end
of the function.

> +	nr_dst_probes = 0;
> +	damon_for_each_probe(dst_probe, dst)
> +		nr_dst_probes++;
> +	nr_src_probes = 0;
> +	damon_for_each_probe(src_probe, src)
> +		nr_src_probes++;
> +	KUNIT_EXPECT_EQ(test, nr_dst_probes, nr_src_probes);
> +	nr_dst_probes = 0;
> +	damon_for_each_probe(dst_probe, dst) {
> +		src_probe = damon_nth_probe(nr_dst_probes, src);
> +		KUNIT_EXPECT_EQ(test, src_probe->weight, dst_probe->weight);

[Severity: Medium]
Could this result in a NULL pointer dereference if the destination context ends
up with more probes than the source context?

KUNIT_EXPECT_EQ() does not abort test execution if the previous check for
nr_dst_probes == nr_src_probes fails. If dst has more probes, damon_nth_probe()
will return NULL, and the next line accessing src_probe->weight will crash the
test runner.

> +		nr_dst_probes++;
> +	}
> +	damon_destroy_ctx(dst);
> +	damon_destroy_ctx(src);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2