Re: [RFC PATCH v1.1 01/13] mm/damon: introduce damon_nr_accesses_mvsum()

[SeongJae Park <sj@kernel.org>]

On Sat, 20 Jun 2026 10:39:30 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Sat, 20 Jun 2026 17:36:32 +0000 sashiko-bot@kernel.org wrote:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] Integer overflow in `mult_frac` within `damon_nr_accesses_mvsum()` corrupts DAMON moving sum access score on 32-bit architectures.
> > --
> > 
> > commit 33b6652e4ab7390d9a1e1c6a292edc9a0fec212a
> > Author: SeongJae Park <sj@kernel.org>
> > 
> > mm/damon: introduce damon_nr_accesses_mvsum()
> > 
> > This commit introduces damon_nr_accesses_mvsum() which returns a pseudo
> > moving sum of a given region's nr_accesses for the last aggregation interval.
> > 
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index 7e4b9affc5b06..af95783d3402a 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > 
> > [ ... ]
> > 
> > > @@ -210,6 +210,63 @@ static struct damon_probe *damon_nth_probe(int n, struct damon_ctx *ctx)
> > >  	return NULL;
> > >  }
> > >  
> > > +/*
> > > + * damon_mvsum() - Returns pseudo moving sum value for a time window.
> > > + * @current_nr:		The value of the current aggregation window.
> > 
> > [ ... ]
> > 
> > > +unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
> > > +		struct damon_ctx *ctx)
> > > +{
> > > +	unsigned long sample_interval, aggr_interval;
> > > +	unsigned long window_len, left_window, left_window_bp;
> > > +
> > > +	sample_interval = ctx->attrs.sample_interval ? : 1;
> > > +	aggr_interval = ctx->attrs.aggr_interval ? : 1;
> > > +	window_len = aggr_interval / sample_interval;
> > > +	left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
> > > +	left_window_bp = mult_frac(left_window, 10000, window_len);
> > 
> > [Severity: High]
> > Can the intermediate multiplication inside mult_frac() overflow on 32-bit
> > architectures?
> > 
> > The mult_frac(x, n, d) macro computes (x / d) * n + (x % d) * n / d. Since
> > left_window is generally less than window_len, the remainder (x % d) evaluates
> > directly to left_window.
> > 
> > On 32-bit architectures, both left_window and the resulting product are 32-bit
> > unsigned long values. If window_len exceeds 429,496 (for example, a 5ms
> > sample_interval coupled with a ~36 minute aggr_interval), the intermediate
> > calculation of left_window * 10000 can exceed ULONG_MAX (4,294,967,295) and
> > silently wrap around.
> > 
> > This wrap around would cause left_window_bp to calculate as a truncated,
> > artificially low value, leading to an incorrect access frequency score that
> > might affect memory reclaim or protection decisions.
> 
> This can happen only under a weird setup that we are not aiming to support for
> now.  We could revisit this if a real user claims this as an issue and ask a
> support.

FYI, I'm hacking on making a central validation point of DAMON parameters.
Once it is done, we may be able to strictly avoid the weird setup by adding a
validation for that.


Thanks,
SJ

[...]