Re: [RFC PATCH 08/16] mm/damon/core: disable access monitoring when probe weights are set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] kdamond_merge_regions loop terminates prematurely because max_thres is capped by access count limit, breaking the scaling of threshold for probe hits.
- [High] Silent breakage of vaddr/fvaddr monitoring when probe weights are configured via sysfs.

Pre-existing issues:
- [High] Unbounded integer overflow of probe_hits array elements in struct damon_region.
--

commit 6a6c230b9f6562af956ffa250fd34b615f031ce5
Author: SJ Park <sj@kernel.org>

mm/damon/core: disable access monitoring when probe weights are set
    
When users set probe weights, they are interested in the data
attributes.  Not necessarily they are interested in data access
patterns.  Disable the data access monitoring in the case.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index f6e03746b170e..db4e404f95bd1 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3670,17 +3670,18 @@ static int kdamond_fn(void *data)
>  		unsigned long next_aggregation_sis = ctx->next_aggregation_sis;
>  		unsigned long next_ops_update_sis = ctx->next_ops_update_sis;
>  		unsigned long sample_interval = ctx->attrs.sample_interval;
> +		bool access_check_disabled = damon_has_probe_weights(ctx);
>  
>  		if (kdamond_wait_activation(ctx))
>  			break;
>  
> -		if (ctx->ops.prepare_access_checks)
> +		if (!access_check_disabled && ctx->ops.prepare_access_checks)
>  			ctx->ops.prepare_access_checks(ctx);
>  
>  		kdamond_usleep(sample_interval);
>  		ctx->passed_sample_intervals++;
>  
> -		if (ctx->ops.check_accesses)
> +		if (!access_check_disabled && ctx->ops.check_accesses)
>  			max_nr_accesses = ctx->ops.check_accesses(ctx);

[Severity: High]
If a user configures a DAMON context with vaddr or fvaddr operations,
and creates a probe via sysfs with a weight, will this bypass check_accesses
without a fallback?

It appears that damon_has_probe_weights() will return true and
access_check_disabled will become true.

This unconditionally bypasses ctx->ops.check_accesses(ctx).

However, if vaddr and fvaddr operations do not currently implement
apply_probes (leaving it NULL), it seems no sampling will occur at all.

Does this silently leave the context without any functional monitoring for
virtual address spaces?

>  		if (ctx->ops.apply_probes)
>  			ctx->ops.apply_probes(ctx, false, false);

[Severity: High]
Could this change cause the loop in kdamond_merge_regions() to exit
prematurely when using probe weights?

When probe weights are enabled, kdamond_merge_regions() starts its
region merging loop with an initial threshold scaled by the maximum
probe hit weighted sum.

However, the loop's upper bound cap, max_thres, remains capped by the
maximum possible access counts:

mm/damon/core.c:kdamond_merge_regions() {
    ...
    } while (nr_regions > c->attrs.max_nr_regions &&
             threshold / 2 < max_thres);
}

Because a probe hit weighted sum can easily exceed the access-based max_thres
by orders of magnitude, the loop condition can fail on the very first
iteration.

Will this force the loop to exit prematurely without adequately
doubling the threshold, leaving nr_regions exceeding max_nr_regions?


[Severity: High]
This isn't a bug introduced by this patch, but there is an unbounded
integer overflow of the probe_hits array elements in struct damon_region.

Running DAMON with a large aggregation interval relative to the sample
interval can cause the unsigned char probe_hits counter to wrap around.

In damon_pa_apply_probes(), the counter is incremented without bounds
checking:

mm/damon/paddr.c:damon_pa_apply_probes() {
    ...
    if (damon_pa_filter_pass(pa, folio, p))
        r->probe_hits[i]++;
    ...
}

Since probe_hits is defined as an unsigned char (max 255) in
struct damon_region:

include/linux/damon.h {
    ...
    unsigned char probe_hits[DAMON_MAX_PROBES];
    ...
}

If a region satisfies a probe for more than 255 sampling intervals within
a single aggregation window, the counter will wrap around to 0 multiple
times.

Might this feed arbitrary garbage values to damon_probe_hits_wsum(),
corrupting the metric that region merging and scheme application rely on?

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