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

[SeongJae Park <sj@kernel.org>]

On Fri, 19 Jun 2026 19:44:28 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Kernel panic due to division by zero in `damon_nr_accesses_mvsum()` when `aggr_interval` is configured to 0.
> - [Medium] Integer overflow in `mult_frac()` leading to severely incorrect access scoring on 32-bit systems when `window_len` is large.
> --
> 
> commit 0c6de91631a66fe41c1e81f68011cf31d86b4825
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon: introduce damon_nr_accesses_mvsum()
> 
> This commit introduces damon_nr_accesses_mvsum() to calculate a pseudo
> moving sum of a given region's nr_accesses for the last aggregation
> interval. The function is designed to be executed on-demand rather than
> at every sampling interval.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > +unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
> > +		struct damon_ctx *ctx)
> > +{
> > +	unsigned long sample_interval, window_len, left_window, left_window_bp;
> > +
> > +	sample_interval = ctx->attrs.sample_interval ? : 1;
> > +	window_len = ctx->attrs.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]
> What happens here if aggr_interval is configured to 0?
> 
> If userspace configures both aggr_interval and sample_interval to 0
> via sysfs, sample_interval evaluates to 1 through the fallback, but
> window_len becomes 0.
> 
> When window_len is passed as the denominator to mult_frac(), does this
> result in a divide-by-zero kernel panic?

Good catch.  I will add a handling of the case in the next revision.

> 
> [Severity: Medium]
> Is there a risk of an integer overflow here on 32-bit systems?
> 
> The mult_frac(x, n, d) macro evaluates the fractional part using the type
> of x, which is an unsigned long here. On 32-bit architectures, this has
> a maximum value of ~4.29 billion.
> 
> If a 5-second aggregation interval and 1-microsecond sample interval are
> configured, window_len and left_window could reach 5,000,000.
> 
> The intermediate multiplication in mult_frac():
> (left_window % window_len) * 10000
> 
> could evaluate to around 50,000,000,000, which overflows a 32-bit
> unsigned long. Could this lead to inaccurate access frequency calculations?

I don't think that's a sane input that need a support.  We can revisit if a
real user report from this is made.


Thanks,
SJ

[...]