Re: [PATCH 1/2] mm/damon/reclaim: add autotune_monitoring_intervals parameter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Validation check on `damon_reclaim_mon_attrs.aggr_interval` prematurely rejects configurations when autotuning is enabled, and exposes a TOCTOU divide-by-zero panic.
--

commit 34f815f9177f234892c1b9608e13d2fc0957697e
Author: SeongJae Park <sj@kernel.org>

mm/damon/reclaim: add autotune_monitoring_intervals parameter

This commit adds a module parameter for utilizing DAMON's auto-tuning
feature with suggested default setups. When enabled, this feature
overrides manually entered monitoring intervals to capture a meaningful
amount of access events per snapshot.

> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index a60ee800d63e9..7126d47fb8b2f 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c

[ ... ]

> @@ -201,12 +215,21 @@ static int damon_reclaim_apply_parameters(void)
>  		goto out;
>  	}
>  
> -	err = damon_set_attrs(param_ctx, &damon_reclaim_mon_attrs);
> +	attrs = damon_reclaim_mon_attrs;
> +	if (autotune_monitoring_intervals) {
> +		attrs.sample_interval = 5000;
> +		attrs.aggr_interval = 100000;
> +		attrs.intervals_goal.access_bp = 40;
> +		attrs.intervals_goal.aggrs = 3;
> +		attrs.intervals_goal.min_sample_us = 5000;
> +		attrs.intervals_goal.max_sample_us = 10 * 1000 * 1000;
> +	}

In the existing code just above this diff, there is a validation check:

    if (!damon_reclaim_mon_attrs.aggr_interval) {
        err = -EINVAL;
        goto out;
    }

If autotune_monitoring_intervals is enabled, could this check prematurely
reject the configuration if damon_reclaim_mon_attrs.aggr_interval is 0?
The auto-tuning override happens right below the check.

Additionally, because this check evaluates the global module parameter
directly instead of a local copy, could a concurrent sysfs write cause a
divide-by-zero panic? 

If a user sets the global interval to 0 right after this check passes:

    if (!damon_reclaim_mon_attrs.aggr_interval) {
        // passes
    }
    
    // concurrent sysfs write sets damon_reclaim_mon_attrs.aggr_interval = 0
    
    attrs = damon_reclaim_mon_attrs;

If autotune_monitoring_intervals is false, attrs.aggr_interval becomes 0
and is passed to damon_reclaim_new_scheme(), which performs:

    .min_age_region = min_age / aggr_interval,

Would moving this check to evaluate the local attrs.aggr_interval after
the auto-tuning block prevent both of these scenarios?

> +	err = damon_set_attrs(param_ctx, &attrs);
>  	if (err)
>  		goto out;
>  
>  	err = -ENOMEM;
> -	scheme = damon_reclaim_new_scheme();
> +	scheme = damon_reclaim_new_scheme(attrs.aggr_interval);
>  	if (!scheme)
>  		goto out;
>  	damon_set_schemes(param_ctx, &scheme, 1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260501011740.81988-1-sj@kernel.org?part=1