Re: [RFC PATCH 6.18.y] 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:
- [High] mm/damon: incomplete zero target_value check in damos_commit_quota_goal()
--

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

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

This commit validates that a DAMOS quota goal's target_value is non-zero in
damos_commit_quota_goal(), returning -EINVAL and propagating the error through
damos_commit_quota_goals() to prevent a division-by-zero in
damos_quota_score().

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/core.c b/mm/damon/core.c
> index 4e6e3d7d0a5c0..8fa788b0d709c 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -807,15 +807,18 @@ static void damos_commit_quota_goal_union(
>  	}
>  }
>  
> -static void damos_commit_quota_goal(
> +static int damos_commit_quota_goal(
>  		struct damos_quota_goal *dst, struct damos_quota_goal *src)
>  {
> +	if (!src->target_value)
> +		return  -EINVAL;

[Severity: High]
Does checking for a zero target_value only in damos_commit_quota_goal() cover
the path where DAMON is started via sysfs in this stable tree?

In mm/damon/sysfs-schemes.c, target_value_store() updates goal->target_value
without holding damon_sysfs_lock:

mm/damon/sysfs-schemes.c:target_value_store() {
	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
			damos_sysfs_quota_goal, kobj);
	int err = kstrtoul(buf, 0, &goal->target_value);

	return err ? err : count;
}

Meanwhile, damos_sysfs_add_quota_score() reads sysfs_goal->target_value twice
without synchronization against target_value_store():

mm/damon/sysfs-schemes.c:damos_sysfs_add_quota_score() {
		if (!sysfs_goal->target_value)
			continue;

		goal = damos_new_quota_goal(sysfs_goal->metric,
				sysfs_goal->target_value);
		...
		damos_add_quota_goal(quota, goal);
}

If target_value_store() writes 0 between those two reads,
damos_new_quota_goal() allocates a goal with target_value == 0 and attaches it
to quota->goals.

When starting DAMON via state_store() -> damon_sysfs_cmd_request_callback() ->
damon_sysfs_handle_cmd() -> damon_sysfs_turn_damon_on(), this branch builds
the context and starts kdamond directly without calling damon_commit_ctx() or
damos_commit_quota_goals():

mm/damon/sysfs.c:damon_sysfs_turn_damon_on() {
	ctx = damon_sysfs_build_ctx(kdamond->contexts->contexts_arr[0]);
	if (IS_ERR(ctx)) {
		kfree(repeat_call_control);
		return PTR_ERR(ctx);
	}
	err = damon_start(&ctx, 1, false);
}

Here damon_sysfs_build_ctx() populates scheme->quota.goals via
damon_sysfs_apply_inputs() -> damon_sysfs_add_schemes() ->
damon_sysfs_mk_scheme() -> damos_sysfs_add_quota_score(), bypassing
damos_commit_quota_goal() entirely.

When kdamond subsequently runs kdamond_fn() -> kdamond_apply_schemes() ->
damos_adjust_quota() -> damos_set_effective_quota() -> damos_quota_score():

mm/damon/core.c:damos_quota_score() {
		highest_score = max(highest_score,
				goal->current_value * 10000 /
				goal->target_value);
}

can this still divide by zero if the race occurs during
damon_sysfs_turn_damon_on()?

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