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

[sashiko-bot@kernel.org]

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.

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

>  	damon_verify_merge_two_regions(l, r);
>  	damon_destroy_region(r, t);
>  }
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516183712.81393-1-sj@kernel.org?part=5