Re: [RFC PATCH 4/8] mm/damon/reclaim: remove duplicated min_region_sz power of 2 check

[SJ Park <sj@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Removing the early `is_power_of_2()` check allows invalid `min_region_sz` inputs to reach `damon_commit_ctx()`, which exposes a bug introduced earlier in this series that corrupts the live context and silently terminates the DAMON background thread.
> --
> 
> commit f5d47eadcc06079427c142dd59c5a8aa8721caec
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/reclaim: remove duplicated min_region_sz power of 2 check
> 
> DAMON_RECLAIM validates the user input for min_region_sz.  The same
> validation is done inside damon_start() and damon_commit_ctx().  Remove
> the duplicate.
> 
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index 11b70d0a9a6f0..6469b25cc34f9 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> > @@ -208,11 +208,6 @@ static int damon_reclaim_apply_parameters(void)
> >  	param_ctx->addr_unit = addr_unit;
> >  	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
> >  
> > -	if (!is_power_of_2(param_ctx->min_region_sz)) {
> > -		err = -EINVAL;
> > -		goto out;
> > -	}
> 
> [Severity: High]
> Does removing this early check expose a bug introduced earlier in this series?
> 
> If the user provides an invalid addr_unit (e.g., 3), min_region_sz will not be
> a power of 2. By removing the check here, this invalid parameter is passed
> to damon_commit_ctx().
> 
> Due to an issue in damon_commit_ctx() (from "mm/damon/core: safely validate
> src on damon_commit_ctx()"), the dry-run validates dst instead of src:
> 
> mm/damon/core.c:damon_commit_ctx() {
> 	...
> 	err = __damon_commit_ctx(test_ctx, dst);
> 	if (err)
> 		goto out;
> 	err = __damon_commit_ctx(dst, src);
> 	...
> }
> 
> When __damon_commit_ctx() is called with the live dst and invalid src, it
> sets dst->maybe_corrupted before returning -EINVAL:
> 
> mm/damon/core.c:__damon_commit_ctx() {
> 	...
> 	dst->maybe_corrupted = true;
> 	if (!is_power_of_2(src->min_region_sz))
> 		return -EINVAL;
> 	...
> }
> 
> Could this cause the background kdamond thread to see maybe_corrupted as true
> on its next iteration and permanently terminate, silently turning a simple
> configuration error into a denial of service for the subsystem?

Yes, I will fix damon_commit_ctx() in the next revision.


Thanks,
SJ

[...]