[PATCH] mm/damon/tests/sysfs-kunit: fix leaked pid reference in add_targets test

[PATCH] mm/damon/tests/sysfs-kunit: fix leaked pid reference in add_targets test

[Xiaolei Wang]

From: Wang Xiaolei <xiaoleiwangxiaolei@gmail.com>

damon_sysfs_test_add_targets() creates its monitoring context with
damon_new_ctx(), which leaves ctx->ops zero-initialized.  Since
DAMON_OPS_VADDR is zero, damon_target_has_pid() returns true for the
context, and damon_sysfs_add_targets() therefore calls
damon_set_target_pid() for each target it adds.  That function grabs a
reference to the target's struct pid via find_get_pid().

The test then tears the context down with damon_destroy_ctx(), which
releases such a reference from ctx->ops.cleanup_target().  For the vaddr
operations set that callback is damon_va_cleanup_target(), and it is the
only place where the reference is put.  The test never selects an
operations set, though, so cleanup_target is NULL and both references --
one for each of the two damon_sysfs_add_targets() calls -- are leaked on
every run of the test.  Running the test with CONFIG_DEBUG_KMEMLEAK
enabled reports the leaked struct pid objects as unreferenced.

Fix this by selecting the vaddr operations set for the context before
adding the targets, as the real usage path (kdamond_init_ctx()) does.
damon_select_ops() fails if the vaddr operations set is not registered,
in which case the test cannot exercise the pid handling and skips
itself.

Fixes: b8ee5575f763 ("mm/damon/sysfs-test: add a unit test for damon_sysfs_set_targets()")
Cc: Liam R. Howlett <liam@infradead.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Vlastimil Babka <vbabka@kernel.org>
Signed-off-by: Wang Xiaolei <xiaoleiwangxiaolei@gmail.com>
---
 mm/damon/tests/sysfs-kunit.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mm/damon/tests/sysfs-kunit.h b/mm/damon/tests/sysfs-kunit.h
index 138a4b8d14e7..634387c6178b 100644
--- a/mm/damon/tests/sysfs-kunit.h
+++ b/mm/damon/tests/sysfs-kunit.h
@@ -79,6 +79,14 @@ static void damon_sysfs_test_add_targets(struct kunit *test)
 		kunit_skip(test, "ctx alloc fail");
 	}
 
+	if (damon_select_ops(ctx, DAMON_OPS_VADDR)) {
+		kfree(sysfs_targets->targets_arr);
+		kfree(sysfs_targets);
+		kfree(sysfs_target->regions);
+		kfree(sysfs_target);
+		kunit_skip(test, "vaddr ops not registered");
+	}
+
 	damon_sysfs_add_targets(ctx, sysfs_targets);
 	KUNIT_EXPECT_EQ(test, 1u, nr_damon_targets(ctx));
 
-- 
2.53.0

Re: [PATCH] mm/damon/tests/sysfs-kunit: fix leaked pid reference in add_targets test

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/damon: memory leak of ctx in damon_sysfs_test_add_targets() on skip
--

commit 15e9d751202cdf7c593524fe7b4266b6f6dd0dbf
Author: Wang Xiaolei <xiaoleiwangxiaolei@gmail.com>

mm/damon/tests/sysfs-kunit: fix leaked pid reference in add_targets test

This commit fixes a struct pid reference leak in
damon_sysfs_test_add_targets() by calling damon_select_ops() to select
DAMON_OPS_VADDR on the monitoring context before adding targets, ensuring
that damon_destroy_ctx() invokes damon_va_cleanup_target() to release the
pid references. If DAMON_OPS_VADDR is not registered, the test cleans up
its allocations and skips itself.

> diff --git a/mm/damon/tests/sysfs-kunit.h b/mm/damon/tests/sysfs-kunit.h
> index 138a4b8d14e73..634387c6178b9 100644
> --- a/mm/damon/tests/sysfs-kunit.h
> +++ b/mm/damon/tests/sysfs-kunit.h
> @@ -79,6 +79,14 @@ static void damon_sysfs_test_add_targets(struct kunit *test)
>  		kunit_skip(test, "ctx alloc fail");
>  	}
>  
> +	if (damon_select_ops(ctx, DAMON_OPS_VADDR)) {
> +		kfree(sysfs_targets->targets_arr);
> +		kfree(sysfs_targets);
> +		kfree(sysfs_target->regions);
> +		kfree(sysfs_target);
> +		kunit_skip(test, "vaddr ops not registered");
> +	}

[Severity: Medium]
Does this error path leak ctx when damon_select_ops() fails (for example,
when CONFIG_DAMON_VADDR is disabled)?

Earlier in damon_sysfs_test_add_targets(), ctx is allocated via
damon_new_ctx():

	ctx = damon_new_ctx();
	if (!ctx) {
		kfree(sysfs_targets->targets_arr);
		kfree(sysfs_targets);
		kfree(sysfs_target->regions);
		kfree(sysfs_target);
		kunit_skip(test, "ctx alloc fail");
	}

When damon_select_ops(ctx, DAMON_OPS_VADDR) returns a non-zero error,
kunit_skip() aborts the test immediately without calling
damon_destroy_ctx(ctx). Should damon_destroy_ctx(ctx) be called before
kunit_skip() here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029015725.17378-1-xiaoleiwangxiaolei@gmail.com?part=1

Re: [PATCH] mm/damon/tests/sysfs-kunit: fix leaked pid reference in add_targets test

[Wang Xiaolei]

在 2026/10/29 10:02, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] mm/damon: memory leak of ctx in damon_sysfs_test_add_targets() on skip
> --
>
> commit 15e9d751202cdf7c593524fe7b4266b6f6dd0dbf
> Author: Wang Xiaolei <xiaoleiwangxiaolei@gmail.com>
>
> mm/damon/tests/sysfs-kunit: fix leaked pid reference in add_targets test
>
> This commit fixes a struct pid reference leak in
> damon_sysfs_test_add_targets() by calling damon_select_ops() to select
> DAMON_OPS_VADDR on the monitoring context before adding targets, ensuring
> that damon_destroy_ctx() invokes damon_va_cleanup_target() to release the
> pid references. If DAMON_OPS_VADDR is not registered, the test cleans up
> its allocations and skips itself.
>
>> diff --git a/mm/damon/tests/sysfs-kunit.h b/mm/damon/tests/sysfs-kunit.h
>> index 138a4b8d14e73..634387c6178b9 100644
>> --- a/mm/damon/tests/sysfs-kunit.h
>> +++ b/mm/damon/tests/sysfs-kunit.h
>> @@ -79,6 +79,14 @@ static void damon_sysfs_test_add_targets(struct kunit *test)
>>   		kunit_skip(test, "ctx alloc fail");
>>   	}
>>   
>> +	if (damon_select_ops(ctx, DAMON_OPS_VADDR)) {
>> +		kfree(sysfs_targets->targets_arr);
>> +		kfree(sysfs_targets);
>> +		kfree(sysfs_target->regions);
>> +		kfree(sysfs_target);
>> +		kunit_skip(test, "vaddr ops not registered");
>> +	}
> [Severity: Medium]
> Does this error path leak ctx when damon_select_ops() fails (for example,
> when CONFIG_DAMON_VADDR is disabled)?
>
> Earlier in damon_sysfs_test_add_targets(), ctx is allocated via
> damon_new_ctx():
>
> 	ctx = damon_new_ctx();
> 	if (!ctx) {
> 		kfree(sysfs_targets->targets_arr);
> 		kfree(sysfs_targets);
> 		kfree(sysfs_target->regions);
> 		kfree(sysfs_target);
> 		kunit_skip(test, "ctx alloc fail");
> 	}
>
> When damon_select_ops(ctx, DAMON_OPS_VADDR) returns a non-zero error,
> kunit_skip() aborts the test immediately without calling
> damon_destroy_ctx(ctx). Should damon_destroy_ctx(ctx) be called before
> kunit_skip() here as well?

Agree, fixed in v2

thanks

xiaolei

>