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

[sashiko-bot@kernel.org]

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604042349.67720-1-sj@kernel.org?part=1