Re: [RFC PATCH v1.1 2/7] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Multiplications by 10000 in DAMON basis-point conversion functions overflow 32-bit unsigned integers when the number of samples per aggregation interval is large.
--

--- Patch [2]: [RFC PATCH v1.1 2/7] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/ ---
commit f63888911d3e9e966f4abf35c7d87cb96cc0e08b
Author: SJ Park <sj@kernel.org>

mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/

This commit renames damon_max_nr_accesses() to damon_nr_samples_per_aggr()
to better describe that it returns the number of samples DAMON checks for
each region per aggregation interval.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index a15005db82899..177e5abc981ef 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -880,14 +880,14 @@ static unsigned int damon_age_for_new_attrs(unsigned int age,
>  static unsigned int damon_accesses_bp_to_nr_accesses(
>  		unsigned int accesses_bp, struct damon_attrs *attrs)
>  {
> -	return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
> +	return accesses_bp * damon_nr_samples_per_aggr(attrs) / 10000;
>  }

[Severity: High]
This is a pre-existing issue, but does this calculation overflow 32-bit
unsigned integers when the number of samples per aggregation interval is
large?

In damon_accesses_bp_to_nr_accesses() in mm/damon/core.c, both accesses_bp
and the return value of damon_nr_samples_per_aggr() are 32-bit unsigned
integers. If an administrator configures DAMON with a large ratio between
aggr_interval and sample_interval (e.g., > 429,496, achievable via sysfs
with a 1us sample and 500ms aggregation), the multiplication will silently
overflow UINT_MAX, yielding much smaller thresholds than intended.

>  /* Convert nr_accesses to access ratio in bp (per 10,000) */
>  static unsigned int damon_nr_accesses_to_accesses_bp(
>  		unsigned int nr_accesses, struct damon_attrs *attrs)
>  {
> -	return mult_frac(nr_accesses, 10000, damon_max_nr_accesses(attrs));
> +	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can this macro expansion also
suffer from a 32-bit overflow?

The mult_frac() macro adopts the type of its first argument, nr_accesses,
which is an unsigned int. Its internal calculation of the remainder
multiplied by the numerator (r * n_) evaluates as an unsigned int.

When the remainder r (which is bounded by samples_per_aggr) exceeds 429,496,
this multiplication by 10000 will overflow 32-bit boundaries, leading to
incorrect basis-point ratio calculations.

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