Re: [RFC PATCH v2 07/17] mm/damon/core: introduce damon_operations->prep_probes()

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] TOCTOU race in `kdamond_fn` across blocking wait leads to NULL pointer dereference of `ctx->ops.prep_probes`.
- [Medium] Silent bypass of preparation actions when unsupported by operations set.
- [Low] API Contract Omission: `prep_probes` documentation fails to specify the requirement to set `sampling_addr`.
--

--- Patch [7]: [RFC PATCH v2 07/17] mm/damon/core: introduce damon_operations->prep_probes() ---
commit f3f60f13684ba2645da0754240dfe010fa41b859
Author: SJ Park <[email protected]>

mm/damon/core: introduce damon_operations->prep_probes()

damon_prep needs to be executed by the underlying DAMON operation set.
Extend the operation set callback list for the execution of damon_prep
actions. If the underlying operation set implements the callback, DAMON
core executes it in the monitoring preparation time.

> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 24b362fcf53b1..fe2c6b56d1d53 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
[ ... ]
> @@ -654,6 +655,8 @@ enum damon_ops_id {
>   * last preparation and update the number of observed accesses of each region.
>   * It should also return max number of observed accesses that made as a result
>   * of its update.  The value will be used for regions adjustment threshold.
> + * @prep_probes should execute required &struct damon_prep for next &struct
> + * damon_probe applications to each region.

[Severity: Low]
Does this documentation need to specify that @prep_probes is responsible
for setting the sampling_addr?

Looking at kdamond_fn(), when do_prep and access_check_disabled are both
true, apply_probes() receives false for its set_samples parameter:

kdamond_fn() {
...
        if (do_prep)
                ctx->ops.prep_probes(ctx, access_check_disabled);
...
                max_wsum = ctx->ops.apply_probes(ctx,
                                access_check_disabled && !do_prep,
                                get_max_wsum);
...
}

This seems to leave prep_probes as the only callback receiving true for
set_samples, making it solely responsible for setting sampling_addr.

>   * @apply_probes should apply the data attribute probes to each region and
>   * accordingly update the probe hits counter of the region.  It should also
>   * set &damon_region->sampling_addr of each region if ``set_samples`` is true.
[ ... ]
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 127d3a7c27280..7ea42983d1276 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -3878,6 +3891,8 @@ static int kdamond_fn(void *data)
>  
>  		if (!access_check_disabled && ctx->ops.prepare_access_checks)
>  			ctx->ops.prepare_access_checks(ctx);

[Severity: High]
Could the context's operations set change during kdamond_wait_activation()
(called just before this), resulting in a NULL pointer dereference here?

The do_prep flag is cached before calling kdamond_wait_activation().

If the wait blocks, a user could update the context's operations to one that
does not implement prep_probes (via kdamond_call() from a sysfs commit),
making ctx->ops.prep_probes NULL.

When execution resumes, it would use the stale do_prep = true value and
attempt to call the now-NULL ctx->ops.prep_probes(...).

[Severity: Medium]
Also, does this silently ignore user-configured preparation actions?

If a user configures a probe with preparation actions, but the current
operations set doesn't implement prep_probes, do_prep evaluates to false.

It appears the preparation actions are silently bypassed during execution
without returning an error during context commit, such as in
__damon_commit_ctx().

This could leave the user unaware that their configured actions are not
being applied.

> +		if (do_prep)
> +			ctx->ops.prep_probes(ctx, access_check_disabled);
>  
>  		kdamond_usleep(sample_interval);
>  		ctx->passed_sample_intervals++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7