Re: [RFC PATCH v1.2 07/19] mm/damon/core: use abs_diff() instead of abs()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Potential integer overflow in weighted average calculations within `damon_merge_two_regions()` on 32-bit architectures.
- [High] Infinite loop in `kdamond_merge_regions()` causing soft lockup when `aggr_interval` is large.
- [High] Multiple erroneous `r->age` updates per aggregation interval during region merging.
--

--- Patch [7]: [RFC PATCH v1.2 07/19] mm/damon/core: use abs_diff() instead of abs() ---
commit f6ab818d018c3ba8c682c6e198e98ba7a8a69b83
Author: SJ Park <sj@kernel.org>

mm/damon/core: use abs_diff() instead of abs()

Use of abs() in damon_merge_regions_of() could cause a silent integer
overflow. This patch replaces it with abs_diff().

Link: https://lore.kernel.org/20260705213817.100841-1-sj@kernel.org/

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index fea97399b5754..4c7e8a12a9b67 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3261,7 +3261,7 @@ static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
>  	struct damon_region *r, *prev = NULL, *next;
>  
>  	damon_for_each_region_safe(r, next, t) {
> -		if (abs(r->nr_accesses - r->last_nr_accesses) > thres)
> +		if (abs_diff(r->nr_accesses, r->last_nr_accesses) > thres)
>  			r->age = 0;
>  		else if ((r->nr_accesses == 0) != (r->last_nr_accesses == 0))
>  			r->age = 0;
> @@ -3269,7 +3269,7 @@ static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
>  			r->age++;

[Severity: High]
This isn't a bug introduced by this patch, but does this logic apply multiple
age updates per aggregation interval if damon_merge_regions_of() is called
multiple times?

The function damon_merge_regions_of() is called inside a do-while loop in
kdamond_merge_regions():

mm/damon/core.c:kdamond_merge_regions() {
	...
	do {
		nr_regions = 0;
		damon_for_each_target(t, c) {
			damon_merge_regions_of(t, threshold, sz_limit);
			nr_regions += damon_nr_regions(t);
		}
		threshold = max(1, threshold * 2);
	} while (nr_regions > c->attrs.max_nr_regions &&
			threshold / 2 < max_thres);
	...
}

Since last_nr_accesses isn't updated until after this loop completes, if
merging fails to reduce nr_regions sufficiently, the loop repeats. Could this
cause the same access counts to be evaluated repeatedly, artificially aging or
resetting memory multiple times within a single aggregation interval?

[Severity: High]
This is a pre-existing issue, but can the loop in kdamond_merge_regions() run
infinitely if max_thres is extremely large?

Looking at the same snippet above, max_thres is an unsigned int calculated
from aggr_interval. If max_thres is >= 2,147,483,648 (for example, due to a
large aggr_interval), doubling threshold will eventually overflow a 32-bit
unsigned int to 0.

Then max(1, 0) resets it to 1. Since threshold / 2 (which is 0) will always
be less than max_thres, could this cause a soft lockup of the kdamond thread?

>  
>  		if (prev && prev->ar.end == r->ar.start &&
> -		    abs(prev->nr_accesses - r->nr_accesses) <= thres &&
> +		    abs_diff(prev->nr_accesses, r->nr_accesses) <= thres &&
>  		    damon_sz_region(prev) + damon_sz_region(r) <= sz_limit)
>  			damon_merge_two_regions(t, prev, r);

[Severity: High]
This is a pre-existing issue, but can the size-weighted average calculations
in damon_merge_two_regions() overflow on 32-bit architectures?

mm/damon/core.c:damon_merge_two_regions() {
	...
	unsigned long sz_l = damon_sz_region(l), sz_r = damon_sz_region(r);

	l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
			(sz_l + sz_r);
	l->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
	...
}

If sz_l is large (e.g., 100 MB or ~104,857,600 bytes) and nr_accesses or age
is moderate (e.g., 50), their product (~5.24 billion) exceeds the maximum
32-bit unsigned integer value (~4.29 billion).

Since unsigned long is 32 bits on those systems, could this silently overflow
and irreversibly corrupt the merged region's statistics?

>  		else
>  			prev = r;

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