Re: [RFC PATCH v1.2 12/17] mm/damon/core: remove nr_accesses_bp setups and updates

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] 32-bit integer overflow in mult_frac() calculation for left_window_bp.

Pre-existing issues:
- [High] Region merge operations do not size-weight average the last_nr_accesses field, leading to inaccurate pseudo moving sum calculations.
--

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

mm/damon/core: remove nr_accesses_bp setups and updates
    
This patch removes the nr_accesses_bp field from struct damon_region and its
associated setups and updates. It states that the field is no longer used for
any real purpose and has been replaced by a simpler function.

The commit message mentions the field is replaced by a simpler function,
which is damon_nr_accesses_mvsum().

Looking at damon_nr_accesses_mvsum(), the fraction of the remaining
aggregation window is calculated like this:

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

[Severity: Medium]
Could this result in a 32-bit integer overflow?

The mult_frac(x, n, d) macro safely computes (x / d) * n + (x % d) * n / d.
Since left_window and window_len are of type unsigned long, on 32-bit
architectures the remainder 'r' and the multiplication 'r * 10000' are
evaluated as 32-bit unsigned integers.

Because userspace can configure an arbitrarily large aggr_interval relative
to sample_interval, window_len can exceed 429,496. If left_window %
window_len is large enough, r * 10000 will overflow 2^32 - 1.

Can this overflow result in an incorrect left_window_bp value, corrupting
the pseudo moving sum access frequency score and misleading DAMON's memory
management decisions on 32-bit systems?

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 94d8ec2965451..be41601163e29 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -3103,7 +3099,6 @@ static void damon_merge_two_regions(struct damon_target *t,
>  
>  	l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
>  			(sz_l + sz_r);
> -	l->nr_accesses_bp = l->nr_accesses * 10000;

[Severity: High]
This is a pre-existing issue, but its impact is amplified to a regression by
this patchset. When damon_merge_two_regions() merges two adjacent regions,
it correctly calculates the size-weighted average for nr_accesses and age,
but fails to merge last_nr_accesses.

Prior to this series, the access score was represented by nr_accesses_bp,
which was correctly size-weighted during a merge. By replacing
nr_accesses_bp with damon_nr_accesses_mvsum(), DAMON now actively relies on
last_nr_accesses to compute the moving sum score.

Because last_nr_accesses is not size-weighted during a merge, merging a
large hot region with a small cold region will entirely wipe out the hot
region's access history.

Could this omission cause inaccurate moving sum scores, leading to incorrect
and suboptimal DAMOS targeting decisions?

>  	l->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
>  	l->ar.end = r->ar.end;
>  	/* todo: do this for only installed probes */

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