Re: [RFC PATCH 6.12.y v2] mm/damon/core: error damos_commit_quota_goal() for zero target_value

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/damon/sysfs: pid leak in damon_sysfs_turn_damon_on() on invalid start ctx
--

commit dc23c827b1a01b5ed768b0a218bb6a67def40106
Author: SJ Park <sj@kernel.org>

mm/damon/core: error damos_commit_quota_goal() for zero target_value

This commit validates that DAMOS quota goal target_value is non-zero in
damos_commit_quota_goal() and damos_commit_quota_goals() to prevent a
division-by-zero error. For the stable backport, it also adds
damon_sysfs_valid_start_ctx() to reject zero target_value before starting
DAMON in damon_sysfs_turn_damon_on().

Link: https://lore.kernel.org/20260903010722.94244-1-sj@kernel.org
Link: https://lore.kernel.org/20260903010722.94244-2-sj@kernel.org
Link: https://lore.kernel.org/20260803134034.15217-1-sj@kernel.org [1]

> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 5aa23c6545cb8..2c8749b62c621 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -1497,6 +1497,20 @@ static struct damon_ctx *damon_sysfs_build_ctx(
>  	return ctx;
>  }
>  
> +static bool damon_sysfs_valid_start_ctx(struct damon_ctx *ctx)
> +{
> +	struct damos *s;
> +	struct damos_quota_goal *g;
> +
> +	damon_for_each_scheme(s, ctx) {
> +		damos_for_each_quota_goal(g, (&s->quota)) {
> +			if (!g->target_value)
> +				return false;
> +		}
> +	}
> +	return true;
> +}
> +
>  static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
>  {
>  	struct damon_ctx *ctx;
> @@ -1517,6 +1531,10 @@ static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
>  	ctx = damon_sysfs_build_ctx(kdamond->contexts->contexts_arr[0]);
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
> +	if (!damon_sysfs_valid_start_ctx(ctx)) {
> +		damon_destroy_ctx(ctx);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Does this error path leak the target struct pid references (t->pid) when
damon_target_has_pid(ctx) is true?

When damon_sysfs_build_ctx() constructs ctx via damon_sysfs_apply_inputs() ->
damon_sysfs_add_targets() -> damon_sysfs_add_target(), it acquires a struct
pid reference for each target:

mm/damon/sysfs.c:damon_sysfs_add_target() {
	...
	if (damon_target_has_pid(ctx)) {
		t->pid = find_get_pid(sys_target->pid);
		if (!t->pid)
			goto destroy_targets_out;
	}
	...
}

Because damon_sysfs_valid_start_ctx(ctx) fails before damon_start() is
called, kdamond never runs and ctx->callback.before_terminate
(damon_sysfs_before_terminate()) is never invoked to release those
references. Calling damon_destroy_ctx(ctx) invokes damon_destroy_targets(ctx),
which frees each target without calling put_pid(t->pid):

mm/damon/core.c:damon_destroy_targets() {
	struct damon_target *t, *next_t;

	if (ctx->ops.cleanup) {
		ctx->ops.cleanup(ctx);
		return;
	}

	damon_for_each_target_safe(t, next_t, ctx)
		damon_destroy_target(t);
}

In comparison, damon_sysfs_commit_input() cleans up an unstarted context from
damon_sysfs_build_ctx() by calling damon_sysfs_destroy_targets() before
damon_destroy_ctx():

mm/damon/sysfs.c:damon_sysfs_commit_input() {
	...
	err = damon_commit_ctx(kdamond->damon_ctx, param_ctx);
	damon_sysfs_destroy_targets(param_ctx);
	damon_destroy_ctx(param_ctx);
	return err;
}

Should damon_sysfs_turn_damon_on() also call damon_sysfs_destroy_targets(ctx)
before damon_destroy_ctx(ctx) here?

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