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

On Wed, 29 Jul 2026 15:00:27 +0000 [email protected] wrote:

> 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.

Good catch, I will fix.

> 
> > +	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.

Good catch, I will fix.

> 
> > +		nr_dst_probes++;
> > +	}
> > +	damon_destroy_ctx(dst);
> > +	damon_destroy_ctx(src);
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
> 


Thanks,
SJ