Re: [RFC PATCH 07/16] mm/damon/core: extend merge function to work with probe hits

[SJ Park <sj@kernel.org>]

On Sun, 05 Jul 2026 21:11:01 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Incompatible scale and domain mismatch between weighted scores and unweighted merge thresholds.
> - [High] Unsafe use of `abs()` macro on large unsigned integer score differences causes silent signed integer overflow.
> - [Medium] Calculating the weighted sum of probe hits in `damon_probe_hits_wsum` can easily overflow a 32-bit `unsigned int`.
> 
> Pre-existing issues:
> - [High] Weighted average calculations in `damon_merge_two_regions` silently overflow 32-bit `unsigned long` on 32-bit architectures.
> --
> 
> commit 54d6e0bb9900e63a1a016ffee9632b6aa602175b
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: extend merge function to work with probe hits
> 
> This commit extends damon_merge_regions_of() to consider the weighted sum
> of probe hits when calculating regions to merge, introducing
> damon_has_probe_weights() and modifying the scoring logic to use these weights.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index c370eaff5795d..f6e03746b170e 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -3187,6 +3192,16 @@ static void damon_merge_two_regions(struct damon_target *t,
> >  	damon_destroy_region(r, t);
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can the weighted average
> calculations in damon_merge_two_regions() silently overflow a 32-bit
> unsigned long on 32-bit architectures?
> 
> On 32-bit architectures, sz_l is a 32-bit unsigned long.
> 
> If the region size is large (e.g., hundreds of megabytes), multiplying sz_l
> by nr_accesses or probe_hits could exceed the 4.29B maximum of a 32-bit
> integer, corrupting the merged region's statistics.
> 
> mm/damon/core.c:damon_merge_two_regions() {
> ...
> 	l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
> 			(sz_l + sz_r);
> ...
> 	for (i = 0; i < DAMON_MAX_PROBES; i++)
> 		l->probe_hits[i] = (l->probe_hits[i] * sz_l + r->probe_hits[i]
> 				* sz_r) / (sz_l + sz_r);
> ...
> }

This doesn't cause a critical consequenes like kernel crash.  Knnown low
priority issue.

> 
> > +static unsigned int damon_merge_score(struct damon_region *r, bool last,
> > +		struct damon_ctx *ctx, bool use_probe_hits)
> > +{
> > +	if (use_probe_hits)
> > +		return damon_probe_hits_wsum(r, last, ctx);
> 
> [Severity: Medium]
> Can the multiplication in damon_probe_hits_wsum() overflow a 32-bit
> unsigned int?
> 
> probe->weight is an unsigned int.
> 
> If users supply a large weight, multiplying it by r->probe_hits[i] (which can
> be up to 255) might exceed UINT_MAX, causing the sum to wrap around and yield
> artificially small scores for highly active regions.
> 
> mm/damon/core.c:damon_probe_hits_wsum() {
> ...
> 		if (last)
> 			sum += r->last_probe_hits[i++] * probe->weight;
> 		else
> 			sum += r->probe_hits[i++] * probe->weight;
> ...
> }

No critical consequence like kernel crash.  Known low priority issue.

> 
> > +	if (last)
> > +		return r->last_nr_accesses;
> > +	return r->nr_accesses;
> > +}
> > +
> >  /*
> >   * Merge adjacent regions having similar access frequencies
> >   *
> > @@ -3195,24 +3210,37 @@ static void damon_merge_two_regions(struct damon_target *t,
> >   * sz_limit	size upper limit of each region
> >   */
> >  static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
> > -				   unsigned long sz_limit)
> > +		unsigned long sz_limit, struct damon_ctx *ctx)
> >  {
> >  	struct damon_region *r, *prev = NULL, *next;
> > +	bool use_probe_hits = damon_has_probe_weights(ctx);
> >  
> >  	damon_for_each_region_safe(r, next, t) {
> > -		if (abs(r->nr_accesses - r->last_nr_accesses) > thres)
> > +		unsigned int score, last_score;
> > +
> > +		score = damon_merge_score(r, false, ctx, use_probe_hits);
> > +		last_score = damon_merge_score(r, true, ctx, use_probe_hits);
> > +
> > +		if (abs(score - last_score) > thres)
> 
> [Severity: High]
> Does the use of abs() here cause a silent signed integer overflow for large
> unsigned differences?
> 
> The macro abs() implicitly casts an unsigned int to a signed int.
> 
> If the mathematical difference between score and last_score exceeds INT_MAX
> (which might happen with large user-defined probe weights), the cast wraps to
> a negative number.
> 
> The abs() macro then negates it into an artificially small positive integer,
> which could cause the code to incorrectly bypass the > thres check and merge
> regions with vastly different access profiles.

Pre-existing issue.  I will fix this separately.

> 
> [Severity: High]
> Is there a domain mismatch between the weighted score differences and the
> unweighted thres parameter?
> 
> When use_probe_hits is true, the score is a weighted sum derived from probe
> hits and user-defined weights. However, thres operates in the unweighted access
> frequency domain.
> 
> This mismatch appears to carry over to kdamond_merge_regions(), where max_thres
> is also scaled in the unweighted domain (aggr_interval / sample_interval).
> 
> Because weighted scores operate in a different numerical domain, could they
> easily exceed max_thres, forcing the merge loop in kdamond_merge_regions() to
> terminate prematurely?
> 
> mm/damon/core.c:kdamond_merge_regions() {
> ...
> 	max_thres = c->attrs.aggr_interval /
> 		(c->attrs.sample_interval ?  c->attrs.sample_interval : 1);
> 	do {
> ...
> 		threshold = max(1, threshold * 2);
> 	} while (nr_regions > c->attrs.max_nr_regions &&
> 			threshold / 2 < max_thres);
> ...
> }

Yes, and that will be fixed by the later commit of this series.

> 
> >  			r->age = 0;
> > -		else if ((r->nr_accesses == 0) != (r->last_nr_accesses == 0))
> > +		else if ((score == 0) != (last_score == 0))
> >  			r->age = 0;
> >  		else
> >  			r->age++;
> >  
> > -		if (prev && prev->ar.end == r->ar.start &&
> > -		    abs(prev->nr_accesses - r->nr_accesses) <= thres &&
> > -		    damon_sz_region(prev) + damon_sz_region(r) <= sz_limit)
> > -			damon_merge_two_regions(t, prev, r);
> > -		else
> > -			prev = r;
> > +		if (!prev)
> > +			goto set_prev_continue;
> > +		if (prev->ar.end != r->ar.start)
> > +			goto set_prev_continue;
> > +		if (abs(damon_merge_score(prev, false, ctx, use_probe_hits) -
> > +					score) > thres)
> 
> [Severity: High]
> Does this abs() usage also suffer from the same silent signed integer overflow
> as mentioned above when comparing large unsigned score differences?

Pre-existing issue.  I will separately work on this.

> 
> > +			goto set_prev_continue;
> > +		if (damon_sz_region(prev) + damon_sz_region(r) > sz_limit)
> > +			goto set_prev_continue;
> > +		damon_merge_two_regions(t, prev, r);
> > +		continue;
> > +set_prev_continue:
> > +		prev = r;
> >  	}
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260705205743.98656-1-sj@kernel.org?part=7


Thanks,
SJ