Re: [RFC PATCH v1.3 11/19] 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:
- [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?

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


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

>  		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