Re: [RFC PATCH 12/16] mm/damon/core: implement damon_has_probe_weight()

[SJ Park <sj@kernel.org>]

On Sun, 05 Jul 2026 21:15:52 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] `damon_has_probe_weights()` returns `true` even for DAMON contexts that do not implement the `apply_probes` operation callback (e.g., `vaddr` and `fvaddr`), causing DAMON to silently bypass access checks and completely stop monitoring memory.
> 
> Pre-existing issues:
> - [High] Integer overflow in `abs()` macro usage with large unsigned probe scores breaks DAMON region merging.
> - [High] Multiplication overflow in `damon_merge_two_regions` when calculating merged probe hits and access counts on 32-bit systems.
> --
> 
> commit 4b421534e3283ef89ac2afef3f02956b0510da1e
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: implement damon_has_probe_weight()
>     
> DAMON monitors only data attributes when probe weights are set. However,
> the function for detecting it is intentionally returning false always.
> Now all the building blocks are made. Complete the mode detection function
> so that the mode can really start working.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 7a070cfb7cd1b..c7de55865b9c9 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -211,6 +211,12 @@ static struct damon_probe *damon_nth_probe(int n, struct damon_ctx *ctx)
> >  
> >  static bool damon_has_probe_weights(struct damon_ctx *c)
> >  {
> > +	struct damon_probe *p;
> > +
> > +	damon_for_each_probe(p, c) {
> > +		if (p->weight)
> > +			return true;
> > +	}
> 
> [Severity: High]
> Does this code need to check if the underlying ops struct supports probes?
> 
> If a context like vaddr or fvaddr has a probe weight configured via sysfs,
> this will return true. This causes access_check_disabled to be set to true in
> kdamond_fn(), which skips check_accesses():
> 
> mm/damon/core.c:kdamond_fn() {
>     ...
>     bool access_check_disabled = damon_has_probe_weights(ctx);
>     ...
> }
> 
> Because ctx->ops.apply_probes is NULL for vaddr, the apply block is then
> skipped and max_merge_score remains 0, which causes kdamond_merge_regions()
> to aggressively merge everything with a threshold of 0.

Yes, and no problem.  Users wrongly configure DAMON will get poor quality of
outputs.  No real bad consequences like kernel crash or memory leak happen.

> 
> >  	return false;
> >  }
> 
> I also noticed a couple of pre-existing issues in this file:
> 
> [Severity: High]
> This is a pre-existing issue, but does this code overflow the 32-bit
> boundary on 32-bit systems?
> 
> mm/damon/core.c:damon_merge_two_regions() {
> [ ... ]
> 	l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
> 			(sz_l + sz_r);
> [ ... ]
> 	for (i = 0; i < DAMON_MAX_PROBES; i++)
> 		l->probe_hits[i] = (l->probe_hits[i] * sz_l + r->probe_hits[i]
> 				* sz_r) / (sz_l + sz_r);
> [ ... ]
> }
> 
> If a region size reaches around 42MB on a 32-bit architecture and
> nr_accesses is around 100, the multiplication nr_accesses * sz_l could
> exceed 4.2 billion and silently overflow the 32-bit unsigned long,
> corrupting the memory access statistics for merged regions.

Known low priority issue.  On 32-bit architectures users could use the
addr_unit parameter to avoid this.

> 
> [Severity: High]
> This is a pre-existing issue, but can the absolute difference of two large
> unsigned integers be evaluated incorrectly here?
> 
> mm/damon/core.c:damon_merge_regions_of() {
> [ ... ]
> 		score = damon_merge_score(r, false, ctx, use_probe_hits);
> 		last_score = damon_merge_score(r, true, ctx, use_probe_hits);
> 
> 		if (abs(score - last_score) > thres)
> 			r->age = 0;
> [ ... ]
> }
> 
> If a user configures a large probe weight via sysfs such that the weighted
> sum exceeds INT_MAX, score - last_score produces a large unsigned int. The
> abs() macro explicitly casts unsigned types to signed types, which would
> wrap into a negative signed integer and then be negated into a much smaller
> positive value. This could bypass the greater than threshold check and
> prevent regions from resetting their age.

Pre-existing issue.  I will separately work on this.

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


Thanks,
SJ