Re: [PATCH 1/4] mm/damon/sysfs: fix param_ctx leak on damon_sysfs_new_test_ctx() failure

Re: [PATCH 1/4] mm/damon/sysfs: fix param_ctx leak on damon_sysfs_new_test_ctx() failure

[SeongJae Park]

On Thu, 19 Mar 2026 15:57:39 +0000 Josh Law <objecting@objecting.org> wrote:

> When damon_sysfs_new_test_ctx() fails in damon_sysfs_commit_input(),
> param_ctx is leaked because the early return skips the cleanup at the
> out label. Destroy param_ctx before returning.

Nice catch, thank you!

The problematic failure can happen only when the arguably too small to fail
allocations fail.  So, the user impact may be not big.  But, still the
consequence is bad.  I think it is better to add Fixes: and Cc stable@, as
below.

Fixes: f0c5118ebb0e ("mm/damon/sysfs: catch commit test ctx alloc failure")
Cc: <stable@vger.kernel.org> # 6.18.x

> 
> Signed-off-by: Josh Law <objecting@objecting.org>

Other than the above,

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

> ---
>  mm/damon/sysfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 576d1ddd736b..b573b9d60784 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -1524,8 +1524,10 @@ static int damon_sysfs_commit_input(void *data)
>  	if (IS_ERR(param_ctx))
>  		return PTR_ERR(param_ctx);
>  	test_ctx = damon_sysfs_new_test_ctx(kdamond->damon_ctx);
> -	if (!test_ctx)
> +	if (!test_ctx) {
> +		damon_destroy_ctx(param_ctx);
>  		return -ENOMEM;
> +	}
>  	err = damon_commit_ctx(test_ctx, param_ctx);
>  	if (err)
>  		goto out;

Sashiko added below comment.  But that's orthogonal to this patch, so never a
blocker of this patch.

: If damon_commit_ctx() fails midway through damon_commit_targets(), could
: struct pid references be leaked?
: 
: When transitioning a DAMON context from DAMON_OPS_PADDR to DAMON_OPS_VADDR,
: param_ctx is built with VADDR ops, while test_ctx inherits PADDR ops from
: the running context.
: 
: Inside damon_commit_targets(), it iterates over targets and calls get_pid()
: for each target since param_ctx has VADDR ops, adding them to test_ctx.
: 
: If a subsequent target fails to allocate memory (like -ENOMEM in
: damon_commit_target_regions()), damon_commit_ctx() returns early and skips
: the dst->ops = src->ops assignment.
: 
: This leaves test_ctx->ops as PADDR, which lacks a cleanup_target callback.
: 
: When the error path jumps to the out label and calls
: damon_destroy_ctx(test_ctx), put_pid() is skipped for the partially
: committed targets because the context still has PADDR ops, permanently
: leaking the struct pid references.
: 
: Is there a way to ensure test_ctx is cleaned up with the correct ops
: if damon_commit_ctx() fails?

# review url: https://sashiko.dev/#/patchset/20260319155742.186627-2-objecting@objecting.org

Sounds like correct.  But defninitely orthogonal to this patch, so no blocker
for this patch.  I will work on this later.


Thanks,
SJ

[...]