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

[sashiko-bot@kernel.org]

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?

[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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622141027.29145-1-sj@kernel.org?part=1