Re: [RFC PATCH v1.1 05/16] mm/damon/core: implement damon_probe_hits_wsum()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The calculation of the weighted sum in `damon_probe_hits_wsum()` is vulnerable to integer overflow, resulting in a silent wrap-around.
- [Low] Misspelled words in the kernel-doc comment for `damon_probe_hits_wsum`.

Pre-existing issues:
- [High] `r->probe_hits[i]` is an 8-bit `unsigned char` but serves as a sample counter across an aggregation interval, making it highly susceptible to wrap-around.
--

commit 627b12cd883dbd832e93cc6b4f4e3373c791b6d0
Author: SJ Park <sj@kernel.org>

mm/damon/core: implement damon_probe_hits_wsum()

This commit implements damon_probe_hits_wsum() to calculate the weighted
sum of probe hits for a DAMON region. This weighted sum is intended to be
used by users as well as DAMON internal logic like region merging.

[ ... ]

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index dbd1f86a0832f..42f27849af722 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -390,6 +390,30 @@ static bool damon_is_last_region(struct damon_region *r,
>  	return list_is_last(&r->list, &t->regions_list);
>  }
>  
> +/**
> + * damon_probe_hits_wsum() - Returns probe hits weighted sum of a region.
> + * @r:		region to get the weigted sum of.
> + * @last:	if the request is for last-window agggregated probe hits.

[Severity: Low]
This isn't a bug, but there are spelling errors in this kernel-doc block.

"weigted" should be "weighted" and "agggregated" should be "aggregated".

> + * @ctx:	context of &r.
> + *
> + * Return: the weighted sum of probe hits of the region.
> + */
> +unsigned int damon_probe_hits_wsum(struct damon_region *r, bool last,
> +		struct damon_ctx *ctx)
> +{
> +	struct damon_probe *probe;
> +	unsigned int sum = 0;
> +	int i = 0;
> +
> +	damon_for_each_probe(probe, ctx) {
> +		if (last)
> +			sum += r->last_probe_hits[i++] * probe->weight;
> +		else
> +			sum += r->probe_hits[i++] * probe->weight;

[Severity: High]
Can the accumulation into sum silently overflow?

If a large probe weight is configured via sysfs (e.g., > 16,843,009), the
multiplication of this 32-bit unsigned int weight by r->probe_hits[i] could
wrap around when accumulated into sum.

This might corrupt the DAMON region merge and split logic that relies on this
score.

[Severity: High]
This is a pre-existing issue, but can r->probe_hits[i] wrap around during long
aggregation intervals?

The probe_hits array is defined as an 8-bit unsigned char. If the user
configures aggr_interval and sample_interval such that their ratio exceeds 255
(for example, 1s aggregation and 1ms sample), the counter will silently wrap
around to 0 without any bounds checking.

This could severely degrade the accuracy of DAMON's monitoring, and is
exacerbated here by relying on the potentially corrupted counter for the new
weighted sum logic.

> +	}
> +	return sum;
> +}

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