Re: [RFC PATCH 04/13] mm/damon/core: use damon_nr_accesses_mvsum() for damos region tracing

[SeongJae Park <sj@kernel.org>]

On Fri, 19 Jun 2026 19:51:40 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Unconditional evaluation of an expensive function (`damon_nr_accesses_mvsum`) as an argument to a tracepoint, causing performance overhead when tracing is disabled.
> 
> Pre-existing issues:
> - [Critical] Pre-existing vulnerability where setting aggr_interval to 0 triggers a divide-by-zero kernel panic in damon_nr_accesses_mvsum().
> --
> 
> commit a32ac0de542ee6f63cc5519f32be214de8a6347f
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: use damon_nr_accesses_mvsum() for damos region tracing
> 
> damon_nr_accesses_mvsum() returns a value same to nr_accesses_bp.  Also
> the function is more simple and therefore more tolerant to errors.
> Execution of the function would be more expensive than the simple read
> of the field, but because the function is quite simple, the overhead
> should be negligible.  Use it in the DAMON region exporting trace points
> instead of the nr_accesses_bp.
> 
> Signed-off-by: SeongJae Park <sj@kernel.org>
> 
> > diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> > index 78388538acf44..8851727ae1627 100644
> > --- a/include/trace/events/damon.h
> > +++ b/include/trace/events/damon.h
> > @@ -78,9 +78,11 @@ TRACE_EVENT_CONDITION(damos_before_apply,
> >  
> >  	TP_PROTO(unsigned int context_idx, unsigned int scheme_idx,
> >  		unsigned int target_idx, struct damon_region *r,
> > -		unsigned int nr_regions, bool do_trace),
> > +		unsigned int nr_accesses, unsigned int nr_regions,
> > +		bool do_trace),
> 
> [ ... ]
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index f45f030a72d82..a7b45b3cad508 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2463,6 +2463,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> >  			return;
> >  		ktime_get_coarse_ts64(&begin);
> >  		trace_damos_before_apply(cidx, sidx, tidx, r,
> > +				damon_nr_accesses_mvsum(r, c),
> >  				damon_nr_regions(t), do_trace);
> 
> [Severity: Medium]
> Will this unconditionally evaluate damon_nr_accesses_mvsum() even when
> tracing is disabled?
> 
> Because trace_damos_before_apply() is an inline function wrapper, its
> arguments are evaluated before the function is called. This means
> damon_nr_accesses_mvsum() will execute on the hot path for every valid
> region, regardless of whether the tracepoint is active. It also causes
> the function to be evaluated a second time per region, since it was
> already called in __damos_valid_target().
> 
> Would it be better to pass the DAMON context pointer c into the tracepoint
> prototype and defer the execution of damon_nr_accesses_mvsum() entirely to
> TP_fast_assign() to avoid this overhead?

Nice catch.  I will execute the function in trace_damos_before_apply_enabled()
scope of the function, like below.

'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2711,7 +2711,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
        struct damos *siter;            /* schemes iterator */
        unsigned int sidx = 0;
        struct damon_target *titer;     /* targets iterator */
-       unsigned int tidx = 0;
+       unsigned int tidx = 0, nr_accesses = 0;
        bool do_trace = false;

        /* get indices for trace_damos_before_apply() */
@@ -2726,6 +2726,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
                                break;
                        tidx++;
                }
+               nr_accesses = damon_nr_accesses_mvsum(r, c);
                do_trace = true;
        }

@@ -2741,8 +2742,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
                if (damos_core_filter_out(c, t, r, s))
                        return;
                ktime_get_coarse_ts64(&begin);
-               trace_damos_before_apply(cidx, sidx, tidx, r,
-                               damon_nr_accesses_mvsum(r, c),
+               trace_damos_before_apply(cidx, sidx, tidx, r, nr_accesses,
                                damon_nr_regions(t), do_trace);
                sz_applied = c->ops.apply_scheme(c, t, r, s,
                                &sz_ops_filter_passed);
'''

> 
> 
> [Severity: Critical]
> This is a pre-existing issue, but does damon_nr_accesses_mvsum() have a
> divide-by-zero vulnerability when userspace configures both sample_interval
> and aggr_interval to 0?
> 
> Userspace can configure DAMON via sysfs. In damon_set_attrs(), the validation
> logic accepts configurations where both intervals are 0:
> 
> mm/damon/core.c:damon_set_attrs() {
>     ...
> 	if (attrs->sample_interval > attrs->aggr_interval)
> 		return -EINVAL;
>     ...
> }
> 
> Since 0 > 0 evaluates to false, this configuration is accepted.
> 
> When damon_nr_accesses_mvsum() is subsequently called, it computes
> window_len = ctx->attrs.aggr_interval / sample_interval, which evaluates to 0.
> It then calls mult_frac(left_window, 10000, window_len), which divides
> by window_len and will cause an immediate kernel panic.

I will fix this in the next revision.


Thanks,
SJ

[...]