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

[SJ Park <sj@kernel.org>]

On Mon, 06 Jul 2026 14:55:38 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] API Misuse / Silent Overflow: Using `abs()` on the difference of two large unsigned integers causes incorrect results.
> - [High] Integer Overflow: `damon_probe_hits_wsum()` wraps around the 32-bit boundary when large weights are used.
> --
> 
> commit b2b82783307a6d10c78010502dd30f5168036627
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: extend merge function to work with probe hits
> 
> When probe weights are set, this commit extends damon_merge_regions_of() to
> work with the weighted sum of probe hits. It also introduces a placeholder
> detection function to allow safe incremental changes.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 42f27849af722..7aff741d43bd4 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> > @@ -3230,6 +3235,16 @@ static void damon_merge_two_regions(struct damon_target *t,
> >  	damon_destroy_region(r, t);
> >  }
> >  
> > +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: High]
> Does this code wrap around the 32-bit boundary when large weights are used?
> 
> Looking at damon_probe_hits_wsum(), it accumulates probe_hits[i] * probe->weight
> into a 32-bit unsigned int sum:
> 
> mm/damon/core.c:damon_probe_hits_wsum() {
>     ...
>     unsigned int sum = 0;
>     ...
>         if (last)
>             sum += r->last_probe_hits[i++] * probe->weight;
>         else
>             sum += r->probe_hits[i++] * probe->weight;
>     ...
>     return sum;
> }
> 
> If a user configures a large weight (e.g., > 16,843,000) for a probe via
> sysfs, this could wrap around (e.g., 255 hits * 40,000,000 weight = 10.2
> billion, which wraps to ~1.6 billion).
> 
> Could this break the scoring mechanism, causing damon_merge_regions_of() to
> merge active and inactive regions incorrectly?

Known low priority issue.  No blocker.

> 
> > +	if (last)
> > +		return r->last_nr_accesses;
> > +	return r->nr_accesses;
> > +}
> > +
> 
> [ ... ]
> 
> > @@ -3238,24 +3253,37 @@ 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]
> Can this call to abs() cause incorrect results due to silent overflow?
> 
> Since score and last_score are unsigned integers, abs(score - last_score)
> casts the unsigned difference to a signed int.
> 
> If the actual difference exceeds the maximum signed integer value, it wraps
> to a negative value, which abs() then negates into a smaller positive value.
> 
> Could this falsely small difference cause the threshold check > thres to fail
> incorrectly, allowing vastly different regions to be merged and affecting the
> accuracy of the monitor?

Same issue exists for the above abs() call.  User impact is minor and I'm
separately working [1] on it.

> 
> >  			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 second use of abs() on the unsigned difference of the merge scores
> suffer from the exact same signed-integer overflow vulnerability?

Ditto.

> 
> > +			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/20260706141912.88445-1-sj@kernel.org?part=7

[1] https://lore.kernel.org/20260705211101.F2F061F000E9@smtp.kernel.org


Thanks,
SJ

[...]