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,  6 Jul 2026 16:52:00 -0700 SJ Park <sj@kernel.org> wrote:

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

Now I think this has no reason to fix right now.  I'll fix it in the next
revision.


Thanks,
SJ

[...]