Re: [RFC PATCH v1.2 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 2 potential issue(s) to consider:

New issues:
- [Low] The KUnit tests fail to build because they still use the deleted function name `damon_max_nr_accesses()`. Furthermore, the test function uses an incorrect pass-by-value argument and is missing from the test cases array.

Pre-existing issues:
- [High] Multiplication involving `damon_nr_samples_per_aggr()` can easily overflow a 32-bit unsigned integer.
--

--- Patch [2]: [RFC PATCH v1.2 2/7] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/ ---
commit 41a943f324d729eeccaf0110b19b34cddf7fa203
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 its actual behavior returning the number of samples per
aggregation interval.

> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 19b7e839bde0b..63f596957c1f1 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -1056,15 +1056,17 @@ static inline bool damon_target_has_pid(const struct damon_ctx *ctx)
>  	return ctx->ops.id == DAMON_OPS_VADDR || ctx->ops.id == DAMON_OPS_FVADDR;
>  }
>  
> -static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs)
> +/* Returns number of samples per aggregation interval */
> +static inline unsigned int damon_nr_samples_per_aggr(
> +		const struct damon_attrs *attrs)

[Severity: Low]
Does this rename break the KUnit tests? 

In mm/damon/tests/core-kunit.h, the damon_test_max_nr_accesses() test still
references the old damon_max_nr_accesses() name and passes attrs by value
instead of as a pointer. 

Furthermore, it appears damon_test_max_nr_accesses() is missing from the
damon_test_cases array.

>  {
>  	unsigned long sample_interval;
> -	unsigned long max_nr_accesses;
> +	unsigned long nr_samples;
>  
>  	sample_interval = attrs->sample_interval ? : 1;
> -	max_nr_accesses = min(attrs->aggr_interval / sample_interval,
> +	nr_samples = min(attrs->aggr_interval / sample_interval,
>  			(unsigned long)UINT_MAX);
> -	return max_nr_accesses ? : 1;
> +	return nr_samples ? : 1;
>  }

[ ... ]

> 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 can this multiplication overflow a
32-bit unsigned integer?

Since damon_nr_samples_per_aggr() can return a large value (up to UINT_MAX),
multiplying it by accesses_bp (which can be up to 10000) could cause an
intermediate overflow wrap-around before the division.

For example, a 35-minute aggregation interval with a 5ms sampling interval
yields about 420,000 samples. Multiplying this by 10,000 exceeds 4.2
billion, which wraps around a 32-bit unsigned integer.

[ ... ]

> 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 is also a pre-existing issue, but does this multiplication have the
same 32-bit overflow risk?

If the aggregation interval is sufficiently large, multiplying
damon_nr_samples_per_aggr() by hot_thres_access_freq could similarly wrap
around before the division.

>  	hot_scheme = damon_lru_sort_new_hot_scheme(hot_thres);

[ ... ]

> 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 another pre-existing issue, but could this multiplication also
overflow?

Multiplying r->nr_accesses by DAMON_MAX_SUBSCORE (100) will overflow a
32-bit unsigned integer if the region has accumulated more than
42,949,672 accesses.

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

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