Re: [RFC PATCH v3 7/9] mm/damon/tests/drain-kunit: kunit for report rings and ring drain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mm: damon: KUnit tests leak struct pid on success path
- [Medium] mm: damon: test bsearch_unsorted_regions mistakenly creates sorted list
- [Medium] mm: damon: memory leak of region r1 on allocation error path
--

commit 19081629f237047138afa26eda20068a417837a2
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>
Subject: mm/damon/tests/drain-kunit: kunit for report rings and ring drain

This commit adds kunit test coverage for the per-context perf report rings
and the unified paddr/vaddr report drain path.

> diff --git a/mm/damon/tests/drain-kunit.h b/mm/damon/tests/drain-kunit.h
> new file mode 100644
> index 0000000000000..8e5023ebf6df7
> --- /dev/null
> +++ b/mm/damon/tests/drain-kunit.h

[ ... ]

> +static void damon_test_unified_vaddr_match(struct kunit *test)
> +{

[ ... ]

> +	t = damon_new_target();
> +	if (!t) {
> +		damon_destroy_ctx(ctx);
> +		kunit_skip(test, "target alloc failed");
> +	}
> +	t->pid = get_pid(task_tgid(current));

[Severity: Medium]
Does this code leak the struct pid reference? Because ops.cleanup_target
is not defined for this test context, damon_destroy_ctx() won't drop the
target's PID reference acquired by get_pid().

[ ... ]

> +static void damon_test_unified_vaddr_tgid_mismatch(struct kunit *test)
> +{

[ ... ]

> +	t = damon_new_target();
> +	if (!t) {
> +		damon_destroy_ctx(ctx);
> +		kunit_skip(test, "target alloc failed");
> +	}
> +	t->pid = get_pid(task_tgid(current));

[Severity: Medium]
Similarly, does this test also leak the struct pid reference on teardown
for the same reason?

[ ... ]

> +static void damon_test_bsearch_unsorted_regions(struct kunit *test)
> +{

[ ... ]

> +	/*
> +	 * Insert regions in REVERSE order so the list is unsorted: r2 at lower
> +	 * address first, r1 at higher address second.  DAMON normally inserts
> +	 * in sorted order; we bypass that here to exercise the bsearch path
> +	 * with unsorted input.
> +	 */
> +	r1 = damon_new_region(0x10000, 0x20000);
> +	r2 = damon_new_region(0x30000, 0x40000);
> +	if (!r1 || !r2) {
> +		damon_free_target(t);
> +		damon_destroy_ctx(ctx);
> +		kunit_skip(test, "region alloc failed");
> +	}

[Severity: Medium]
If r1 is allocated successfully but r2 fails to allocate, does this error
path leak r1? Since r1 has not been added to the target's region list
yet, damon_free_target() won't free it.

> +	/* Add in reverse (high first) to create an unsorted list. */
> +	list_add(&r2->list, &t->regions_list);
> +	list_add(&r1->list, &t->regions_list);

[Severity: Medium]
Does this sequence actually create a sorted list instead of an unsorted
one? Because list_add() prepends to the head, adding r2 (higher address)
then r1 (lower address) places r1 before r2. This results in the regions
being sorted in ascending order, which defeats the purpose of the test.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-damon-perf-rfc-v3-send-2026-10-03-v3-0-0f00417b41bc@gmail.com?part=7