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

[Dan Carpenter]

On Thu, Oct 29, 2026 at 07:10:40AM -0700, SJ Park wrote:
> Hello Xiaolei,
> 
> On Thu, 29 Oct 2026 15:56:09 +0800 Xiaolei Wang <xiaoleiwangxiaolei@xxxxxxxxx> wrote:
> 
> > From: Wang Xiaolei <xiaoleiwangxiaolei@xxxxxxxxx>
> > 
> > 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, mirroring what the sysfs interface 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.
> 
> Makes sense.  Thank you for finding and fixing this.
> 
> > 
> > Fixes: b8ee5575f763 ("mm/damon/sysfs-test: add a unit test for damon_sysfs_set_targets()")
> 
> The impact is leak.  I think we should add Cc: stable@.

I wouldn't bother Cc stable for unit tests.  It's not something
which affects production or real life at all.

I used to be a be more involved in testing and the rule was that you
pulled the tests from the latest kernel and ran them against the stable
kernels.  It feels like that can't make a lot of sense for unit testing
so maybe for unit testing we didn't do that.  I can't remember...

regards,
dan carpenter