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

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

[SJ Park]

Hello Xiaolei,

On Thu, 29 Oct 2026 15:56:09 +0800 Xiaolei Wang <xiaoleiwangxiaolei@gmail.com> wrote:

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

Cc: <stable@vger.kernel.org> # 6.7.x

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

Other than Cc: stable@,

Reviewed-by: SJ Park <sj@kernel.org>

Assuming you agree to Cc stable@, I applied this patch to damon/next [1] tree
after adding the Cc.  I will repost it next Monday, as an mm.git merge request.
So, no action from your side is needed for now.

[1] https://origin.kernel.org/doc/html/latest/mm/damon/maintainer-profile.html#scm-trees


Thanks,
SJ

[...]

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@gmail.com> wrote:
> 
> > 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, 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

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

[SJ Park]

On Thu, 29 Oct 2026 18:18:05 +0300 Dan Carpenter <error27@gmail.com> wrote:

> 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@gmail.com> wrote:
> > 
> > > 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, 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.

Thank you for your input, Dan!

I have no strong opinion.  But we got CVEs [1] for this class of unit test code
in the past.  I'd like to keep it consistent to the past for now.

[1] https://lore.kernel.org/2024102930-CVE-2024-50068-2e3c@gregkh/


Thanks,
SJ

[...]