Re: [RFC PATCH v3 05/28] mm/damon/core: introduce damon_region->probe_hits

[SeongJae Park <sj@kernel.org>]

On Sat, 16 May 2026 18:54:35 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Data type overflow of `probe_hits` array due to 8-bit `unsigned char` limit.
> - [High] Integer overflow in `probe_hits` weighted average calculation on 32-bit systems.
> --
> 
> commit b34f3f1254d83e9491235fb4e8e097729be04e60
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: introduce damon_region->probe_hits
> 
> This commit adds a probe_hits array to the damon_region structure to track
> the number of positive samples per region for each probe. It limits the
> number of probes to a fixed maximum for a simpler implementation.
> 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 32005b8b1f8cd..9e70239e5305f 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> [ ... ]
> > @@ -80,6 +83,7 @@ struct damon_region {
> >  	unsigned long sampling_addr;
> >  	unsigned int nr_accesses;
> >  	unsigned int nr_accesses_bp;
> > +	unsigned char probe_hits[DAMON_MAX_PROBES];
> 
> Does probe_hits need a wider type to prevent wrapping around?
> 
> Since the number of samples in an aggregation interval is determined by
> aggr_interval / sample_interval, user-configurable values could cause the
> sample count to exceed the 8-bit limit of unsigned char (255). This might
> lead to silently wrapped monitoring statistics.

Yes, but such configurations may be rare, so no blocker.
For more details, please read my previous reply [1] for same question.

> 
> >  	struct list_head list;
> >  
> >  	unsigned int age;
> [ ... ]
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 15f2795a1156f..e5667660b4cd9 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -2754,12 +2757,17 @@ static void damon_merge_two_regions(struct damon_target *t,
> >  		struct damon_region *l, struct damon_region *r)
> >  {
> >  	unsigned long sz_l = damon_sz_region(l), sz_r = damon_sz_region(r);
> > +	int i;
> >  
> >  	l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
> >  			(sz_l + sz_r);
> >  	l->nr_accesses_bp = l->nr_accesses * 10000;
> >  	l->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
> >  	l->ar.end = r->ar.end;
> > +	/* todo: do this for only installed probes */
> > +	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);
> 
> Could this weighted average calculation overflow on 32-bit architectures?
> 
> Both sz_l and sz_r are unsigned long, which is 32 bits on 32-bit systems.
> If a region size is large (e.g., > 16.8MB) and probe_hits is up to 255,
> multiplying them can exceed the 32-bit limit and wrap around, corrupting
> the monitoring statistics.
> 
> It appears the nr_accesses calculation just above this has the exact same
> flaw, and this patch propagates it to the new probe_hits array.

On 32bit systems, users could use the addr_unit to avoid this kind of issues.
So no blocker.

[1] https://lore.kernel.org/20260516003559.141476-1-sj@kernel.org


Thanks,
SJ

[...]