Re: [RFC PATCH v1.3 6/7] mm/damon/core: update probe hits for new parameter commit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unbounded iteration over `ctx->probes` list writing to the statically sized `probe_hits` array can lead to buffer overflow.
- [Medium] Scaling `probe_hits` and `last_probe_hits` using `damon_nr_samples_for_new_attrs()` assigns an `unsigned int` to an `unsigned char`, causing silent truncation and data corruption when the scaled count exceeds 255.
--

--- Patch [6]: [RFC PATCH v1.3 6/7] mm/damon/core: update probe hits for new parameter commit ---
commit 5d7c7277de18e07d88cf49d1f16d870a9dfc0f23
Author: SJ Park <sj@kernel.org>

mm/damon/core: update probe hits for new parameter commit

Users can update DAMON parameters at runtime.  If the samples and/or
aggregation intervals are updated in this way, monitoring results
depending on the intervals should also be updated for a more accurate
snapshot.  The age and nr_accesses are properly updated, while
probe_hits are not updated in the way.  Do the update.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 800ff6b6d2473..15d31285515d6 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -897,10 +897,32 @@ static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr,
>  			damon_sample_count_to_bp(nr, old_attrs), new_attrs);
>  }
>  
> +static void damon_update_probe_hits(struct damon_region *r,
> +		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs,
> +		bool aggregating, struct damon_ctx *ctx)
> +{
> +	struct damon_probe *p;
> +	int i = 0;
> +
> +	damon_for_each_probe(p, ctx) {

[Severity: High]
Does this loop overflow the r->last_probe_hits[] and r->probe_hits[] arrays?

The probe_hits and last_probe_hits members are defined as fixed-size arrays of
size DAMON_MAX_PROBES. Since there is no bounds checking on i, if there are
more than DAMON_MAX_PROBES in the ctx->probes list, this will write past the
end of the arrays and corrupt adjacent fields in struct damon_region.

> +		r->last_probe_hits[i] = damon_nr_samples_for_new_attrs(
> +				r->last_probe_hits[i], old_attrs, new_attrs);

[Severity: Medium]
Will this assignment cause silent truncation and data corruption?

The last_probe_hits and probe_hits arrays are declared as unsigned char
(8-bit) in struct damon_region, but damon_nr_samples_for_new_attrs() returns an
unsigned int (32-bit).

If the scaled count exceeds 255, assigning it directly will truncate the value
to a meaningless modulo-256 wrapped value rather than clamping it at a maximum.

> +		if (!aggregating)
> +			r->probe_hits[i] = damon_nr_samples_for_new_attrs(
> +					r->probe_hits[i], old_attrs,
> +					new_attrs);
> +		else
> +			r->probe_hits[i] = 0;
> +		i++;
> +	}
> +}
> +

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