Re: [RFC PATCH 2/8] mm/damon/core: do parameter testing commit on damon_start()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `damon_commit_ctx()` incorrectly validates `dst` against `test_ctx` instead of `src`, bypassing the safety mechanism.
- [Medium] Reusing a single `test_ctx` across multiple loop iterations in `damon_start()` leaks state between contexts, leading to non-deterministic validation outcomes.
--

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

mm/damon/core: do parameter testing commit on damon_start()

damon_start() and damon_commit_ctx() are the two main API functions for
setting DAMON parameters. This commit embeds the parameter check already
present in damon_commit_ctx() into damon_start(), allowing callers to
safely use damon_start() without manually validating parameters first.

The commit message mentions that damon_commit_ctx() does holistic
parameters testing. Looking at damon_commit_ctx() in the same file:

mm/damon/core.c:damon_commit_ctx() {
	...
	test_ctx = damon_new_test_ctx(dst);
	if (!test_ctx)
		return -ENOMEM;
	err = __damon_commit_ctx(test_ctx, dst);
	if (err)
		goto out;
	err = __damon_commit_ctx(dst, src);
	...
}

[Severity: High]
Does this bypass the safety validation mechanism in damon_commit_ctx()?

The validation incorrectly tests the active context dst against test_ctx
instead of testing the incoming parameters src. 

If invalid parameters are supplied or if a mid-commit allocation fails, the
incoming parameters src are directly applied to the active context dst
without any prior dry-run validation. This leaves the active context dst in a
corrupted state (maybe_corrupted = true), potentially leading to monitoring
thread crashes or unpredictable kernel behavior.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 5ca176eb27e1d..ace202f47dd7c 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -1785,13 +1787,22 @@ static int __damon_start(struct damon_ctx *ctx)
>   */
>  int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
>  {
> +	struct damon_ctx *test_ctx;
>  	int i;
>  	int err = 0;
>  
> +	test_ctx = damon_new_ctx();
> +	if (!test_ctx)
> +		return -ENOMEM;
> +
>  	for (i = 0; i < nr_ctxs; i++) {
> -		if (!is_power_of_2(ctxs[i]->min_region_sz))
> -			return -EINVAL;
> +		err = __damon_commit_ctx(test_ctx, ctxs[i]);

[Severity: Medium]
Does reusing a single test_ctx across multiple loop iterations leak state
between contexts?

If a caller invokes damon_start() with multiple contexts (nr_ctxs > 1),
test_ctx is mutated in the first iteration. In the second iteration,
__damon_commit_ctx() runs on the same, un-cleared test_ctx.

For example, if ctxs[1] has an obsolete target:

mm/damon/core.c:damon_commit_targets() {
	...
		/* target to remove has no matching dst */
		if (src_target->obsolete) {
			err = -EINVAL;
			goto out;
		}
	...
}

It evaluates against the residual targets of ctxs[0]. If ctxs[0] had enough
targets, the obsolete target incorrectly matches and passes validation. If
ctxs[0] had no targets, it fails with -EINVAL, producing non-deterministic
validation results.

> +		if (err) {
> +			damon_destroy_ctx(test_ctx);
> +			return err;
> +		}
>  	}
> +	damon_destroy_ctx(test_ctx);
>  
>  	mutex_lock(&damon_lock);
>  	if ((exclusive && nr_running_ctxs) ||

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