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

[SJ Park <sj@kernel.org>]

On Wed, 08 Jul 2026 03:05:43 +0000 sashiko-bot@kernel.org wrote:

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

That cannot happen since DAMON_SYSFS is not accepting that input.   Read
nr_probes_store() for more details.

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

Known low priority issue.

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


Thanks,
SJ