Re: [RFC PATCH v1.3 11/19] mm/damon/core: disable access monitoring when probe weights are set

[SJ Park <sj@kernel.org>]

On Fri, 10 Jul 2026 00:43:26 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] Skipping `check_accesses()` leaves `max_nr_accesses` stale or zero, permanently breaking the region merging heuristic and causing unbounded region growth.
> - [Medium] Skipping `prepare_access_checks()` leaves `r->sampling_addr` uninitialized, leading to arbitrary physical memory reads.
> - [Medium] Configuring probe weights on operations that do not support them (e.g., VADDR) breaks the monitoring context completely.
> --
> 
> --- Patch [11]: [RFC PATCH v1.3 11/19] mm/damon/core: disable access monitoring when probe weights are set ---
> commit 156f6c878eae64d06c0b181a333eb7acd3bfbbf6
> 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 a0e6a5129fbf3..65f91e3b58d2e 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -3763,17 +3763,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)
> 
> [Severity: Medium]
> Does skipping ctx->ops.prepare_access_checks() leave the sampling_addr
> field uninitialized?
> 
> If access_check_disabled evaluates to true, prepare_access_checks() is
> skipped. The region object allocated by damon_new_region() leaves this
> field uninitialized:
> 
> mm/damon/core.c:damon_new_region() {
>     ...
>     region = kmem_cache_alloc(damon_region_cache, GFP_KERNEL);
>     if (!region)
>         return NULL;
> 
>     region->ar.start = start;
>     region->ar.end = end;
>     region->nr_accesses = 0;
>     ...
> }
> 
> Then apply_probes(ctx, false, false) is called below with set_samples as
> false. Could damon_pa_apply_probes() read the uninitialized sampling_addr
> to generate a physical address, leading to arbitrary physical memory reads?

No issue.  access_check_disabled is always false for now.

> 
> >  			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: Medium]
> Will skipping check_accesses() leave max_nr_accesses stale or zero?
> 
> If access_check_disabled is true, check_accesses() is skipped, and
> apply_probes is called with return_max_wsum as false, so its return value
> is ignored. Could this pass a stale max_nr_accesses into
> kdamond_merge_regions(), preventing adjacent regions evaluated by large
> weighted sums from ever merging and causing runaway region fragmentation?

No issue.  access_check_disabled is always false for now.

> 
> 
> [Severity: Medium]
> What happens if probe weights are configured for operations that do not
> support them?
> 
> If access_check_disabled evaluates to true because probe weights are set,
> this skips check_accesses(). Because ctx->ops.apply_probes would be NULL
> for operations like vaddr, it is also skipped.
> 
> Does damon_valid_probe_params() need to reject configurations where
> ctx->ops.apply_probes is NULL to prevent the context from silently
> skipping all data gathering?

No issue.  access_check_disabled is always false for now.  Even if it can be
true, that is the intended behavior.

> 
> >  		if (ctx->ops.apply_probes)
> >  			ctx->ops.apply_probes(ctx, false, false);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260710002349.111414-1-sj@kernel.org?part=11


Thanks,
SJ