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

[sashiko-bot@kernel.org]

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

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);
	...
}

[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?

>  	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