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

On Sun, 26 Jul 2026 17:57:00 +0000 [email protected] wrote:

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

Nice suggestion, I will do, like below.

'''
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -714,7 +714,8 @@ enum damon_ops_id {
  * 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.
+ * damon_probe applications to each region.  It should also set
+ * &damon_region->sampling_addr of each region if ``set_samples`` is true.
  * @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.
'''

> 
> >   * @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(...).

Good finding!  I will move do_prep setup to be done after
kdamond_wait_activation().
> 
> [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.

That's the intended behavior.

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


Thanks,
SJ