Re: [RFC PATCH 1/5] mm/damon/core: fix nr_accesses_bp underflow in damon_moving_sum

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unsigned integer overflow in DAMON's moving sum and access rate tracking for large intervals.
--

commit 1d58ed9728ddbaf4579ec265e453a7b3a233285c
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>

mm/damon/core: fix nr_accesses_bp underflow in damon_moving_sum

Guard against unsigned integer underflow when nomvsum/len_window exceeds
mvsum. When that subtraction wraps, the moving sum returns a near maximum
value and corrupts nr_accesses_bp. If subtrahend > mvsum, return new_value.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 3a8725e400c6b..9975f3d9ebfe9 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3449,7 +3449,11 @@ int damon_set_region_system_rams_default(struct damon_target *t,
>  static unsigned int damon_moving_sum(unsigned int mvsum, unsigned int nomvsum,
>  		unsigned int len_window, unsigned int new_value)
>  {

This isn't a bug introduced by this patch, but could the nomvsum argument
passed here be silently overflowing?

Looking at damon_update_region_access_rate(), damon_moving_sum() is called
with r->last_nr_accesses * 10000:

mm/damon/core.c:damon_update_region_access_rate() {
	...
	if (attrs->sample_interval)
		len_window = damon_max_nr_accesses(attrs);
	r->nr_accesses_bp = damon_moving_sum(r->nr_accesses_bp,
			r->last_nr_accesses * 10000, len_window,
			accessed ? 10000 : 0);
	...
}

DAMON allows configuring aggr_interval / sample_interval up to the maximum
32-bit unsigned integer (for example, 10ms sampling and 1.5 hours
aggregation).

This means len_window, which bounds last_nr_accesses, can exceed 429,496.

If this happens, won't the multiplication r->last_nr_accesses * 10000
silently overflow the 32-bit unsigned int limit of 4,294,967,295?

If this overflows, the corrupted, wrapped-around value is passed as nomvsum,
which would cause subtrahend to be artificially small.

Additionally, would this cause r->nr_accesses_bp (which accumulates mvsum)
to also continuously wrap around 32-bit limits?

Could these silent integer overflows corrupt memory access tracking and lead
to erroneous memory management decisions, like reclaiming hot memory?

A proper fix might require bounding len_window to 429,496 or using 64-bit
integers.

> -	return mvsum - nomvsum / len_window + new_value;
> +	unsigned int subtrahend = nomvsum / len_window;
> +
> +	if (subtrahend > mvsum)
> +		return new_value;
> +	return mvsum - subtrahend + new_value;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516210357.2247-1-ravis.opensrc@gmail.com?part=1