Re: [RFC PATCH 1/6] 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] Multiplication overflow in DAMON access rate calculations due to 32-bit arithmetic.
--

--- Patch [1]: [RFC PATCH 1/6] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/ ---
commit 928ef68b58766df0da13e507ad449f7ba9d5c79d
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 390e00b3685ef..e710e2789085e 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -880,7 +880,7 @@ 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 isn't a bug introduced by this patch, but can this multiplication in
damon_accesses_bp_to_nr_accesses() overflow 32-bit arithmetic?

If the sample interval is configured to a small value and the aggregation
interval to a large value, damon_nr_samples_per_aggr() can return a large
number. Multiplying accesses_bp by this large number wraps around modulo
2^32 since both operands are 32-bit unsigned integers, resulting in an
incorrectly reduced access count.

>  }
>  
>  /*
> @@ -893,7 +893,7 @@ static unsigned int damon_accesses_bp_to_nr_accesses(
>  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 is a pre-existing issue, but does the use of mult_frac() in
damon_nr_accesses_to_accesses_bp() also suffer from a 32-bit overflow?

When nr_accesses is sufficiently large, the internal calculation in
mult_frac() overflows 32-bit arithmetic.

>  }
>  

[ ... ]

> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index e8c389ad3226f..7e077084cb03a 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -303,7 +303,7 @@ static int damon_lru_sort_apply_parameters(void)
>  		goto out;
>  
>  	err = -ENOMEM;
> -	hot_thres = damon_max_nr_accesses(&attrs) *
> +	hot_thres = damon_nr_samples_per_aggr(&attrs) *
>  		hot_thres_access_freq / 1000;

[Severity: High]
This isn't a bug introduced by this patch, but does this calculation in
damon_lru_sort_apply_parameters() overflow on 32-bit architectures?

Multiplying the sample count by hot_thres_access_freq overflows 32-bit
arithmetic on 32-bit systems when the sample count is large.

>  	hot_scheme = damon_lru_sort_new_hot_scheme(hot_thres);
>  	if (!hot_scheme)

[ ... ]

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index 6bdd1cfd3863a..89c694f6e4fa8 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -112,7 +112,7 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
>  	int hotness;
>  
>  	freq_subscore = r->nr_accesses * DAMON_MAX_SUBSCORE /
> -		damon_max_nr_accesses(&c->attrs);
> +		damon_nr_samples_per_aggr(&c->attrs);

[Severity: High]
This is a pre-existing issue, but could this calculation in
damon_hot_score() result in a 32-bit overflow?

Multiplying r->nr_accesses by DAMON_MAX_SUBSCORE (which is 100) overflows
32-bit arithmetic if the number of samples is near UINT_MAX. This might
cause hot memory to be incorrectly identified as cold.

>  
>  	age_in_sec = (unsigned long)r->age * c->attrs.aggr_interval / 1000000;
>  	if (age_in_sec)

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