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

[SJ Park <sj@kernel.org>]

On Thu, 09 Jul 2026 14:32:09 +0000 sashiko-bot@kernel.org wrote:

> 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?

Sounds making sense.  Apparently not fit to be fixed in this series, though.  I
will further check and separately work on this.

> 
> [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);

Seems a good finding.  Not related to this series, as  Sashiko also says.  I
will further check separatelt work on this.

> 
> [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?

Known low priority issue.

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


Thanks,
SJ