Re: [RFC PATCH] mm/damon/core: always put commit-failed target's pids

[SeongJae Park <sj@kernel.org>]

On Thu, 04 Jun 2026 04:37:55 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] `damon_commit_target_fail()` incorrectly skips PID cleanup for sysfs test contexts due to uninitialized callbacks.
> 
> Pre-existing issues:
> - [High] PID reference leak on multiple error paths during context conversion from paddr to vaddr bypasses the newly added cleanup logic.
> --
> 
> commit 9659672aa95ce46b9511dd7216240c3314476900
> Author: SeongJae Park <sj@kernel.org>
> Subject: mm/damon/core: always put commit-failed target's pids
> 
> This commit fixes a PID leak that occurs when damon_commit_target() fails.
> It adds damon_commit_target_fail() to handle putting the already-committed
> PIDs when cleaning up after a failed commit.
> 
> Link: https://lore.kernel.org/20260320020056.835-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1387,6 +1387,31 @@ static int damon_commit_target(
> >  	return 0;
> >  }
> >  
> > +/*
> > + * damon_commit_target_fail() - handle damon_commit_target() failure.
> > + * @dst:	Commit destination context
> > + * @failed:	Commit failed destination target
> > + * @src:	Commit source context
> > + *
> > + * This function is called by damon_commit_targets() for dammon_commit_target()
> > + * failure for immediate states cleanups.
> > + */
> > +static void damon_commit_target_fail(struct damon_ctx *dst,
> > +		struct damon_target *failed, struct damon_ctx *src)
> > +{
> > +	struct damon_target *target;
> > +
> > +	if (!damon_target_has_pid(src))
> > +		return;
> > +	if (damon_target_has_pid(dst))
> > +		return;
> 
> [Severity: High]
> Does this early return handle sysfs test contexts correctly?
> 
> When damon_sysfs_new_test_ctx() creates a context using damon_new_ctx(),
> the ops structure is zero-initialized. Since DAMON_OPS_VADDR evaluates
> to 0, damon_target_has_pid() will return true for this test context,
> but its cleanup_target callback is still NULL.
> 
> If damon_commit_target() fails while copying to this test context,
> will this return early and skip PID cleanup, trusting a NULL callback
> to handle it later during destruction?

Good catch, I will fix this in the next revision.

> 
> > +	damon_for_each_target(target, dst) {
> > +		if (target == failed)
> > +			return;
> > +		put_pid(target->pid);
> > +	}
> > +}
> 
> [ ... ]
> 
> > @@ -1432,6 +1459,7 @@ static int damon_commit_targets(
> >  				src_target, damon_target_has_pid(src),
> >  				src->min_region_sz);
> >  		if (err) {
> > +			damon_commit_target_fail(dst, NULL, src);
> >  			damon_destroy_target(new_target, NULL);
> >  			return err;
> >  		}
> 
> [Severity: High]
> This is a pre-existing issue, but does this cleanup logic miss other
> error paths during a paddr to vaddr conversion?
> 
> When converting a DAMON context from paddr to vaddr, damon_commit_targets()
> copies PIDs and increments their reference count in the first loop.
> If an error occurs before this point in the second loop:
> 
>     damon_for_each_target_safe(src_target, next, src) {
>         ...
>         if (src_target->obsolete)
>             return -EINVAL;
>         new_target = damon_new_target();
>         if (!new_target)
>             return -ENOMEM;
>         ...
> 
> These paths return directly without calling damon_commit_target_fail()
> to clean up the already committed PIDs.
> 
> Additionally, if damon_commit_targets() succeeds but damon_set_attrs()
> fails later in damon_commit_ctx(), it returns before the dst operations
> are updated.
> 
> In all these cases, the destination context is left with paddr
> operations where cleanup_target is NULL. When damon_destroy_ctx() is
> called, will it skip dropping these PID references and leak them?

Good finding.  I will fix these in the next revision.


Thanks,
SJ

[...]