Re: [RFC PATCH v1.1 1/8] mm/damon/core: safely validate src on damon_commit_ctx()

[SJ Park <sj@kernel.org>]

On Thu, 02 Jul 2026 21:21:42 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] damon_commit_ctx() silently corrupts the quota charging state of running schemes.
> --
> 
> commit b274d76c061808ff0cd97c6e9433199f816cc1de
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: safely validate src on damon_commit_ctx()
> 
> damon_commit_ctx() does its holistic parameter set validation while
> applying the new parameter in the set one by one.  If it finds a
> parameter is invalid, because some invalid parameters may already be
> committed (it is called "commit" but not atomic and irreversable), it
> stops the running DAMON context.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index c6fc35be633b3..b523a18a7b971 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> > @@ -1702,6 +1689,52 @@ int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
> >  	return 0;
> >  }
> >  
> > +static struct damon_ctx *damon_new_test_ctx(struct damon_ctx *dst)
> > +{
> > +	struct damon_ctx *test_ctx;
> > +	int err;
> > +
> > +	test_ctx = damon_new_ctx();
> > +	if (!test_ctx)
> > +		return NULL;
> > +	err = __damon_commit_ctx(test_ctx, dst);
> 
> [Severity: High]
> This is a pre-existing issue, but does this unintentionally corrupt the running
> quota state?
> 
> When __damon_commit_ctx() is called with the running context (dst) as the
> source, it copies the schemes via damon_commit_schemes(), which calls
> damon_new_scheme() and passes a pointer to the active scheme's quota:
> 
> mm/damon/core.c:damon_commit_schemes() {
> 	...
> 	new_scheme = damon_new_scheme(&src_scheme->pattern,
> 				src_scheme->action,
> 				src_scheme->apply_interval_us,
> 				&src_scheme->quota, &src_scheme->wmarks,
> 				NUMA_NO_NODE);
> 	...
> }
> 
> damon_new_scheme() then calls damos_quota_init() on that provided quota
> pointer:
> 
> mm/damon/core.c:damon_new_scheme() {
> 	...
> 	scheme->quota = *(damos_quota_init(quota));
> 	...
> }
> 
> damos_quota_init() zeroes out the fields in-place:
> 
> mm/damon/core.c:damos_quota_init() {
> 	...
> 	quota->esz = 0;
> 	quota->total_charged_sz = 0;
> 	...
> }
> 
> This mutates the running scheme's quota state, which would make the quota
> effectively "full" mid-interval and prevent the scheme from applying its
> actions until the next reset interval.

That doesn't matter because it is only testing purpose scheme, not the schemes
of the running context.

> 
> Since this patch moves this test context logic into the core
> damon_commit_ctx(), doesn't this expose all API users like damon_reclaim
> and damon_lru_sort to this state corruption when parameters are committed?

So, no problem.


Thanks,
SJ

[...]