Re: [PATCH 08/19] mm/damon/core: extend merge function to work with probe hits

[SJ Park <sj@kernel.org>]

On Fri, 10 Jul 2026 14:05:31 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Unscaled merge threshold breaks region merging when using weighted probe hits.
> - [Medium] Potential 32-bit integer overflow in `damon_probe_hits_wsum()` when accumulating weighted sums.
> 
> Pre-existing issues:
> - [High] Region ages are evaluated and mutated inside a retry loop, leading to age corruption.
> - [High] Calculations for merged regions' weighted averages overflow on 32-bit architectures.
> - [Medium] Test NULL pointer dereference in `damon_test_merge_regions_of`.
> --
> 
> --- Patch [8]: [PATCH 08/19] mm/damon/core: extend merge function to work with probe hits ---
> commit 5e9a094bb8d36feea59f27615cb45bcbce00e4c3
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: extend merge function to work with probe hits
> 
> When probe weights are set, users may want DAMON monitoring results to
> be optimized for the weights.  For that, regions adjustment should work
> for the weighted sum of probe hits.  Extend damon_merge_regions_of() to
> detect if the weights are set, and work with probe hits in the case.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 4c7e8a12a9b67..7d49420ea26c4 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -3248,6 +3253,16 @@ static void damon_merge_two_regions(struct damon_target *t,
> [Severity: High]
> This is a pre-existing issue, but can the calculations for merged regions'
> weighted averages overflow on 32-bit architectures?
> 
> mm/damon/core.c:damon_merge_two_regions() {
>     unsigned long sz_l = damon_sz_region(l), sz_r = damon_sz_region(r);
>     ...
>     l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
>                      (sz_l + sz_r);
>     l->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
>     ...
> }
> 
> Since sz_l and sz_r are unsigned long (32 bits on 32-bit architectures),
> multiplying them by a 32-bit unsigned int produces a 32-bit intermediate
> product. For large region sizes, this might wrap around UINT_MAX prior to
> division, leading to incorrect page reclaim decisions.

Known low priority issue.  No blocker.

> 
> >  	damon_destroy_region(r, t);
> >  }
> >  
> > +static unsigned int damon_merge_score(struct damon_region *r, bool last,
> > +		struct damon_ctx *ctx, bool use_probe_hits)
> > +{
> > +	if (use_probe_hits)
> > +		return damon_probe_hits_wsum(r, last, ctx);
> [Severity: Medium]
> Can accumulating the weighted sums in damon_probe_hits_wsum() overflow a
> 32-bit integer?
> 
> mm/damon/core.c:damon_probe_hits_wsum() {
>     ...
>     unsigned int sum = 0;
>     ...
>         if (last)
>             sum += r->last_probe_hits[i++] * probe->weight;
>         else
>             sum += r->probe_hits[i++] * probe->weight;
>     ...
> }
> 
> If user space configures a large weight, multiplying it by the probe hits
> across multiple probes might easily exceed UINT_MAX, causing silent
> overflow. This wrapped around value would then corrupt the region merging
> heuristic.

No issue.  Nobody calls this function at the moment.  A later patch in this
series will prevent such user input before this can be really used.

> 
> > +	if (last)
> > +		return r->last_nr_accesses;
> > +	return r->nr_accesses;
> > +}
> > +
> >  /*
> >   * Merge adjacent regions having similar access frequencies
> >   *
> > @@ -3256,24 +3271,38 @@ 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_diff(r->nr_accesses, r->last_nr_accesses) > thres)
> > +		unsigned int score, last_score, diff;
> > +
> > +		score = damon_merge_score(r, false, ctx, use_probe_hits);
> > +		last_score = damon_merge_score(r, true, ctx, use_probe_hits);
> > +
> > +		if (abs_diff(score, last_score) > thres)
> [Severity: Medium]
> When probe weights are enabled and use_probe_hits is true, score and
> last_score are scaled by probe->weight via damon_probe_hits_wsum().
> However, kdamond_merge_regions() calculates the initial threshold using
> unweighted access metrics. Will comparing this unscaled threshold against
> the newly scaled weighted scores prevent region merges and force the retry
> loop to inappropriately scale the threshold?

No issue.  damon_has_probe_weights() always return false.  A later patch in
this series will change that before damon_has_probe_weights() can return true.

> 
> >  			r->age = 0;
> > -		else if ((r->nr_accesses == 0) != (r->last_nr_accesses == 0))
> > +		else if ((score == 0) != (last_score == 0))
> >  			r->age = 0;
> >  		else
> >  			r->age++;
> [Severity: High]
> This is a pre-existing issue, but does mutating the region age inside this
> retry loop corrupt the age metric?
> 
> In kdamond_merge_regions(), damon_merge_regions_of() is called inside a
> do-while loop that repeats if nr_regions > max_nr_regions. Every time
> damon_merge_regions_of() executes, it unconditionally mutates r->age for
> every region based on the current threshold. Because the outer loop
> doubles the threshold and retries, r->age might be artificially inflated or
> repeatedly reset to 0 multiple times per aggregation interval.

Known issue.  I'm working on this.

> 
> [ ... ]
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index d97280270c27b..6ad73559dd8ea 100644
> > --- a/mm/damon/tests/core-kunit.h
> > +++ b/mm/damon/tests/core-kunit.h
> [ ... ]
> > @@ -258,6 +266,7 @@ static void damon_test_merge_regions_of(struct kunit *test)
> >  		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
> >  	}
> [Severity: Medium]
> This is a pre-existing issue, but could this test cause a NULL pointer
> dereference and leak test resources?
> 
> If the internal damon_merge_regions_of logic unexpectedly merges too many
> regions, damon_nr_regions(t) might fall below 6. The KUNIT_EXPECT_EQ check
> logs a failure but does not halt the test.
> 
> The subsequent loop executes 6 times regardless:
> 
> mm/damon/tests/core-kunit.h:damon_test_merge_regions_of() {
>     ...
>     for (i = 0; i < 6; i++) {
>         r = __nth_region_of(t, i);
>         KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
>     ...
> }
> 
> This would cause __nth_region_of() to return NULL, which is immediately
> dereferenced in the KUNIT_EXPECT_EQ macro without validation.

Good catch.  No blocker, though.  I will separately work on this.

> 
> >  	damon_free_target(t);
> > +	damon_destroy_ctx(ctx);
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260710134651.18084-1-sj@kernel.org?part=8


Thanks,
SJ