Re: [RFC PATCH v1.2 1/2] mm/damon/core: handle zero intervals in damon_max_nr_accesses()

[SeongJae Park <sj@kernel.org>]

On Mon, 22 Jun 2026 14:29:36 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Integer underflow and state corruption in `nr_accesses_bp` calculation when `sample_interval == 0`.
> - [High] 32-bit integer overflow in access rate scaling and hotness scoring silently breaks region statistics for long aggregation intervals.
> --
> 
> commit 4e734fc35404d39d1209a202d1c72aa4e9ef543f
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: handle zero intervals in damon_max_nr_accesses()
> 
> This commit addresses a divide-by-zero error in damon_max_nr_accesses()
> by forcing a denominator of 1 when the sampling interval is zero. It also
> ensures the function always returns at least 1.
> 
> Link: https://lore.kernel.org/20260619202459.145010-1-sj@kernel.org
> 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 6f7edb3590ef9..888570f55b416 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -1065,9 +1065,13 @@ static inline bool damon_target_has_pid(const struct damon_ctx *ctx)
> >  
> >  static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs)
> >  {
> > -	/* {aggr,sample}_interval are unsigned long, hence could overflow */
> > -	return min(attrs->aggr_interval / attrs->sample_interval,
> > +	unsigned long sample_interval;
> > +	unsigned long max_nr_accesses;
> > +
> > +	sample_interval = attrs->sample_interval ? : 1;
> > +	max_nr_accesses = min(attrs->aggr_interval / sample_interval,
> >  			(unsigned long)UINT_MAX);
> > +	return max_nr_accesses ? : 1;
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while this commit fixes the
> zero interval handling inside damon_max_nr_accesses(), there is a bypass in
> mm/damon/core.c that still relies on sample_interval:
> 
> mm/damon/core.c:damon_update_region_access_rate()
> 	if (attrs->sample_interval)
> 		len_window = damon_max_nr_accesses(attrs);
> 
> When sample_interval is 0, this bypass circumvents the properly fixed
> damon_max_nr_accesses() function, locking len_window at 1. During the
> next aggregation window, damon_moving_sum() recalculates the moving
> average. Because len_window is stuck at 1, the entire nomvsum is
> subtracted from a smaller mvsum.
> 
> Will this cause nr_accesses_bp to massively underflow its unsigned int
> type and wrap around, corrupting the region's access metrics?

Good finding.  Not a blocker of this patch, though.  I will separately work on
this.

> 
> [Severity: High]
> This is also a pre-existing issue, but does damon_update_monitoring_result()
> suffer from a 32-bit integer overflow for long aggregation intervals?
> 
> When a long aggregation interval and short sample interval are configured,
> the accesses can be quite large:
> 
> mm/damon/core.c:damon_update_monitoring_result()
> 	r->nr_accesses_bp = r->nr_accesses * 10000;
> 
> If nr_accesses is very high, does this 32-bit unsigned int arithmetic
> silently wrap around, heavily corrupting nr_accesses_bp?
> 
> A similar overflow appears in damon_hot_score():
> 
> mm/damon/ops-common.c:damon_hot_score()
> 	freq_subscore = r->nr_accesses * DAMON_MAX_SUBSCORE /
> 			damon_max_nr_accesses(&c->attrs);
> 
> Does the numerator multiplication overflow the 32-bit bounds before
> the division, resulting in an artificially skewed, low hotness score?

Known low priority issue.  No blocker of this patch.


Thanks,
SJ

[...]