Re: [RFC PATCH v1.2 03/17] mm/damon/core: always update ->last_nr_accesses for intervals change

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Arithmetic overflow in DAMON access scaling functions when interval ratio is large.
- [High] The patch attempts to fix a 'wrong value' returned by `damon_nr_accesses_mvsum()`, but misdiagnoses the root cause and leaves a major double-counting bug unresolved.
--

commit 6636564dd42488eb260d7102569887a00bdd3d71
Author: SeongJae Park <sj@kernel.org>

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

This commit modifies damon_update_monitoring_result() to unconditionally
update last_nr_accesses when aggregation intervals change. It aims to
prevent incorrect calculations in damon_nr_accesses_mvsum() that could occur
if called with a stale last_nr_accesses value alongside a fresh aggregation
timestamp.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index addd14e4e70bf..bc40a6d10f277 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -873,6 +873,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 this scaling trigger an arithmetic
overflow when the interval ratio is large?

When damon_nr_accesses_for_new_attrs() is called, it performs 32-bit
arithmetic in damon_nr_accesses_to_accesses_bp() and
damon_accesses_bp_to_nr_accesses().

Specifically:

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

mm/damon/core.c:damon_accesses_bp_to_nr_accesses() {
    ...
    return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
}

If the ratio of aggr_interval to sample_interval exceeds 429,496 (for
example, a 1s aggregation and 1us sample), the intermediate multiplications
like r * 10000 inside mult_frac() are computed as 32-bit unsigned int,
which overflows the 32-bit maximum.

Can this silently corrupt the region access counts upon attribute updates,
leading to completely incorrect monitoring results?


[Severity: High]
This isn't a bug introduced by this patch, but does scaling
last_nr_accesses fully resolve the incorrect values returned by
damon_nr_accesses_mvsum(), or does it leave a double-counting flaw intact?

The commit message notes that damon_nr_accesses_mvsum() can return the
wrong value. However, the root cause appears to be a state desynchronization
rather than just a stale last_nr_accesses.

When damon_set_attrs() is called, it advances ctx->next_aggregation_sis
into the future:

mm/damon/core.c:damon_set_attrs() {
    ...
    ctx->next_aggregation_sis = ctx->passed_sample_intervals +
                                attrs->aggr_interval / sample_interval;
}

If a subsequent call to damon_nr_accesses_mvsum() evaluates this future
timestamp, it assumes it is at the start of a new interval, meaning
left_window_bp evaluates to 10000.

mm/damon/core.c:damon_nr_accesses_mvsum() {
    ...
    return damon_mvsum(r->nr_accesses, r->last_nr_accesses, left_window_bp);
}

Because kdamond_reset_aggregated() has not yet executed, r->nr_accesses
still holds the fully accumulated count of the just-completed interval.
This means damon_mvsum() adds two full intervals' worth of access counts
together, effectively calculating:
r->nr_accesses + (r->last_nr_accesses * 10000 / 10000).

Does unconditionally scaling last_nr_accesses merely adjust the second term
of this sum while preserving the underlying double-counting logic flaw?

>  	if (!aggregating) {
>  		r->nr_accesses = damon_nr_accesses_for_new_attrs(
>  				r->nr_accesses, old_attrs, new_attrs);
> @@ -884,8 +886,6 @@ static void damon_update_monitoring_result(struct damon_region *r,
>  		 * interval.  In other words, make the status like
>  		 * kdamond_reset_aggregated() is called.
>  		 */
> -		r->last_nr_accesses = damon_nr_accesses_for_new_attrs(
> -				r->last_nr_accesses, old_attrs, new_attrs);
>  		r->nr_accesses_bp = r->last_nr_accesses * 10000;
>  		r->nr_accesses = 0;
>  	}

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