Re: [PATCH 03/18] mm/damon/core: always update ->last_nr_accesses for intervals change

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A 32-bit unsigned integer overflow in `mult_frac()` silently corrupts region access basis point conversions when the aggregation interval is significantly larger than the sample interval.
--

commit f786cbe1a79f2e1958843b60ee1875084aebedbc
Author: SJ Park <sj@kernel.org>

mm/damon/core: always update ->last_nr_accesses for intervals change

Each iteration of kdamond_fn() caches the next aggregation time. This patch
ensures last_nr_accesses is always updated to prevent returning wrong values
when damon_nr_accesses_mvsum() is called during an intervals change.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -871,6 +871,8 @@ static void damon_update_monitoring_result(struct damon_region *r,
>  		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs,
>  		bool aggregating)
>  {
> +	r->last_nr_accesses = damon_nr_accesses_for_new_attrs(
> +			r->last_nr_accesses, old_attrs, new_attrs);

[Severity: High]
This is a pre-existing issue, but does the underlying access basis point
conversion silently overflow on 32-bit values?

When this calls damon_nr_accesses_for_new_attrs(), it relies on
damon_nr_accesses_to_accesses_bp() to calculate the scaling:

mm/damon/core.c:damon_nr_accesses_to_accesses_bp() {
    return mult_frac(nr_accesses, 10000, damon_max_nr_accesses(attrs));
}

Because mult_frac() evaluates the multiplication using the type of
nr_accesses (a 32-bit unsigned int), multiplying by 10000 will silently wrap
around if nr_accesses exceeds ~429,496. This could happen if a large ratio
between the aggregation interval and sample interval is configured via sysfs,
causing hot memory regions to be incorrectly evaluated as cold.

A similar issue appears to exist in damon_nr_accesses_mvsum() where
left_window (a 32-bit unsigned long on 32-bit architectures) is multiplied
by 10000 inside mult_frac():

mm/damon/core.c:damon_nr_accesses_mvsum() {
    ...
    left_window_bp = mult_frac(left_window, 10000, window_len);
    ...
}

Could these operands be safely cast to a larger type to prevent the overflow
before passing them to mult_frac()?

>  	if (!aggregating) {
>  		r->nr_accesses = damon_nr_accesses_for_new_attrs(
>  				r->nr_accesses, old_attrs, new_attrs);

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