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

[SJ Park <sj@kernel.org>]

On Wed, 01 Jul 2026 15:20:50 +0000 sashiko-bot@kernel.org wrote:

> 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.

Correct.  Same finding was replied to the patch 1.  I promised to fix it in the
next revision.

> 
> > 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.

Good finding.  I will allocate text context for each user-given context in the
next revision.


Thanks,
SJ

[...]