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

[SJ Park <sj@kernel.org>]

On Sun, 05 Jul 2026 16:12:09 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] State Corruption on Error Path: invalid `addr_unit` permanently corrupts global monitoring region parameters.
> - [Medium] API Contract Violation: `ALIGN` macros are invoked with non-power-of-two inputs, computing garbage values.

No blockers.  Please read below for more details.

> --
> 
> commit f2a479c4ed9e6eb22e5e61615eb62f684e1703b1
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/reclaim: remove duplicated min_region_sz power of 2 check
>     
> This commit removes the power-of-two validation for min_region_sz in
> damon_reclaim_apply_parameters(). The change relies on the assumption
> that identical validation in damon_start() and damon_commit_ctx() is
> sufficient to catch invalid parameters.
> 
> > 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]
> By removing this early check, does an invalid addr_unit now proceed to call
> damon_set_region_system_rams_default() before damon_commit_ctx() rejects it?
> 
> If so, could this permanently corrupt the global monitor_region_start and
> monitor_region_end parameters on the error path?
> 
> For example, damon_find_system_rams_range() scales and overwrites these
> global pointers before the context validation has a chance to fail:
> 
> mm/damon/core.c:damon_find_system_rams_range() {
> 	...
> 	*start = damon_res_to_core_addr(arg.res.start, addr_unit);
> 	*end = damon_res_to_core_addr(arg.res.end + 1, addr_unit);
> 	...
> }

Users can show and correct it.  And this happens only when the user entered an
invalid parameter.  No bad consequence like kernel panic happens.  So, this is
just a minor user experience issue.  And I want to make validation mminimum for
simplicity.  This minor issue doesn't deserve to add the complexity in my
opinion.  I will keep this as is unless others chime in.

> 
> [Severity: Medium]
> Additionally, if we allow a non-power-of-two min_region_sz to be passed
> downwards, does this violate the requirements of the ALIGN macros?
> 
> When damon_set_regions() processes the temporary target structure:
> 
> mm/damon/core.c:damon_set_regions() {
> 	...
> 	start = ALIGN_DOWN(ranges[i].start, min_region_sz);
> 	end = ALIGN(ranges[i].end, min_region_sz);
> 	...
> }
> 
> Since ALIGN relies on bitwise operations, won't passing a non-power-of-two
> min_region_sz result in garbage address calculations?

Yes, but anyway it cannot be committed due to the validation in the commit.  No
bad consequene like kernel ppanic happens.  So no issue.

> 
> >  	if (!damon_reclaim_mon_attrs.aggr_interval) {
> >  		err = -EINVAL;
> >  		goto out;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260705155600.96555-1-sj@kernel.org?part=4


Thanks,
SJ

[...]