Re: [RFC PATCH v1.1 14/16] Docs/mm/damon/design: document attrs-only monitoring

[SJ Park <sj@kernel.org>]

On Mon, 06 Jul 2026 15:34:57 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Multiplication and accumulation in `damon_probe_hits_wsum()` can silently overflow a 32-bit `unsigned int`.
> - [High] Silently disables all monitoring if probe weights are configured for operations sets that lack probe support (e.g., `vaddr`).
> 
> Pre-existing issues:
> - [High] The `abs()` macro behaves incorrectly for large `unsigned int` differences, breaking region adjustment logic.
> - [High] The `probe_hits` array relies on 8-bit `unsigned char`, leading to silent wraparound during long aggregation intervals.
> --
> 
> commit 61ffda736cfeed6cc995344cdd1d753a189751a8
> Author: SJ Park <sj@kernel.org>
> 
> Docs/mm/damon/design: document attrs-only monitoring
> 
> This patch series introduces Attributes-only Monitoring for DAMON, adding
> probe weights to dictate memory management scoring. When weights are
> configured, data access monitoring is disabled and regions are evaluated
> based on weighted probe hits instead.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 3c5a46e1c079a..6e5a029989a0c 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> > @@ -389,6 +401,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.
> > + * @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]
> Does sum need protection against integer overflow here?
> 
> If a user configures a large priority weight for a probe via sysfs, this
> multiplication can silently overflow the 32-bit unsigned int sum (for
> example, 255 * 4294967295).
> 
> Could this integer overflow wrap around the score metric, making heavily
> weighted regions incorrectly appear as "cold" regions and corrupting the core
> memory management data?

Will fix in the next version of this series, or separately.

> 
> [Severity: High]
> This is a pre-existing issue, but since Attributes-only Monitoring makes
> probe hits the primary metric, could probe_hits overflow its 8-bit limit?
> 
> In include/linux/damon.h, the array is defined as an 8-bit value:
> 
> struct damon_region {
> 	...
> 	unsigned char probe_hits[DAMON_MAX_PROBES];
> 	...
> };
> 
> If a user configures an aggregation window that encompasses more than 255
> samples, the counter increment in damon_pa_apply_probes():
> 
> 	r->probe_hits[i]++;
> 
> will repeatedly wrap around, destroying the statistical data and corrupting
> the region scoring.

Known low priority issue.  No blocker.  But I will consider fixing this in the
next revision or seaprately.

> 
> > +	}
> > +	return sum;
> > +}
> 
> [ ... ]
> 
> > @@ -3211,24 +3259,37 @@ static void damon_merge_two_regions(struct damon_target *t,
> >   * sz_limit	size upper limit of each region
> >   */
> >  static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
> > -				   unsigned long sz_limit)
> > +		unsigned long sz_limit, struct damon_ctx *ctx)
> >  {
> >  	struct damon_region *r, *prev = NULL, *next;
> > +	bool use_probe_hits = damon_has_probe_weights(ctx);
> >  
> >  	damon_for_each_region_safe(r, next, t) {
> > -		if (abs(r->nr_accesses - r->last_nr_accesses) > thres)
> > +		unsigned int score, last_score;
> > +
> > +		score = damon_merge_score(r, false, ctx, use_probe_hits);
> > +		last_score = damon_merge_score(r, true, ctx, use_probe_hits);
> > +
> > +		if (abs(score - last_score) > thres)
> 
> [Severity: High]
> This is a pre-existing issue, but it seems exacerbated now that arbitrary
> weight multipliers are introduced. Does abs() behave correctly for large
> unsigned int differences?
> 
> The abs() macro implicitly casts its arguments to signed int. If the
> absolute difference between score and last_score exceeds INT_MAX, it
> will overflow and be evaluated incorrectly as a smaller positive number.
> 
> Because weights can now easily push scores past INT_MAX, will this cause
> damon_merge_regions_of() to improperly ignore large score differences?

Will fix in the next revision.

> 
> >  			r->age = 0;
> > -		else if ((r->nr_accesses == 0) != (r->last_nr_accesses == 0))
> > +		else if ((score == 0) != (last_score == 0))
> >  			r->age = 0;
> 
> [ ... ]
> 
> > @@ -3658,25 +3718,38 @@ static int kdamond_fn(void *data)
> >  		unsigned long next_aggregation_sis = ctx->next_aggregation_sis;
> >  		unsigned long next_ops_update_sis = ctx->next_ops_update_sis;
> >  		unsigned long sample_interval = ctx->attrs.sample_interval;
> > +		bool access_check_disabled = damon_has_probe_weights(ctx);
> > +		unsigned int max_merge_score = 0, max_wsum;
> > +		bool get_max_wsum;
> >  
> >  		if (kdamond_wait_activation(ctx))
> >  			break;
> >  
> > -		if (ctx->ops.prepare_access_checks)
> > +		if (!access_check_disabled && ctx->ops.prepare_access_checks)
> >  			ctx->ops.prepare_access_checks(ctx);
> >  
> >  		kdamond_usleep(sample_interval);
> >  		ctx->passed_sample_intervals++;
> >  
> > -		if (ctx->ops.check_accesses)
> > -			max_nr_accesses = ctx->ops.check_accesses(ctx);
> > -		if (ctx->ops.apply_probes)
> > -			ctx->ops.apply_probes(ctx);
> > +		if (!access_check_disabled && ctx->ops.check_accesses)
> > +			max_merge_score = ctx->ops.check_accesses(ctx);
> > +		if (ctx->ops.apply_probes) {
> 
> [Severity: High]
> If a user configures probe weights via sysfs for an operations set that
> doesn't implement apply_probes (e.g., DAMON_OPS_VADDR), will this silently
> disable all monitoring?
> 
> Because access_check_disabled becomes true unconditionally when weights are
> present, default access checking is skipped. Without ctx->ops.apply_probes,
> no probes are applied either, effectively breaking the DAMON monitoring thread
> for that operations set.

I will fix this in the next revision, or separately.

> 
> > +			if (time_after_eq(ctx->passed_sample_intervals,
> > +						next_aggregation_sis) &&
> > +					access_check_disabled)
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260706141912.88445-1-sj@kernel.org?part=14


Thanks,
SJ

[...]