Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] destroy_monitor_dir() frees mon->edge_pcpu before VFS synchronization, allowing concurrent readers to access freed memory via the tracefs stats file. - [Medium] Non-atomic memset corrupts local64_t counters, racing with concurrent NMI tracepoint updates and lockless tracefs reads. - [Medium] rv_edge_reset() only resets statistics on currently online CPUs, violating the guarantee that counters are fully cleared on enablement. --
commit 5d961f88b7dec86f152d8e652fbd68ebc65c3317 Author: Tobias Schaffner <[email protected]> rv: add per-monitor edge-stat facility and stats file This commit introduces CONFIG_RV_EDGE_STAT, an optional feature that records how long a monitor's automaton dwells in a state. It allocates a per-CPU buffer on first enable and exposes the counters via a per-monitor stats file. > diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c > index ee4e68102f175..88a0bbaec4d0a 100644 > --- a/kernel/trace/rv/rv.c > +++ b/kernel/trace/rv/rv.c [ ... ] > @@ -289,9 +298,15 @@ static int rv_enable_single(struct rv_monitor *mon) > > retval = mon->enable(); > > - if (!retval) > + if (!retval) { > mon->enabled = 1; > > + if (rv_edge_setup(mon)) > + pr_warn("rv: %s: edge statistics unavailable (out of > memory)\n", > + mon->name); > + rv_edge_reset(mon); [Severity: Medium] Does resetting the statistics with memset() after the monitor is already enabled risk data corruption? Since mon->enable() is called before rv_edge_reset(), NMI tracepoint updates can fire concurrently. Using memset() in rv_edge_reset_ipi() to clear the array of local64_t counters could race with these read-modify-write updates, potentially resulting in torn reads or corrupted atomic statistics when rv_edge_stats_show() accesses them. > + } > + > return retval; > } [ ... ] > +static void rv_edge_reset_ipi(void *info) > +{ > + struct rv_monitor *mon = info; > + > + memset(this_cpu_ptr(mon->edge_pcpu), 0, rv_edge_blob_size(mon)); > +} > + > +/* rv_edge_reset - zero the statistics; call from a monitor reset/enable. */ > +static void rv_edge_reset(struct rv_monitor *mon) > +{ > + if (mon->edge_pcpu) > + on_each_cpu(rv_edge_reset_ipi, mon, 1); > +} [Severity: Medium] Will this skip clearing the statistics on offline CPUs? Because on_each_cpu() only targets online CPUs, dynamically allocated per-CPU edge statistics buffers for offline CPUs remain uncleared. If an offline CPU is later brought online, reading the tracefs stats file will iterate over it via for_each_online_cpu() and report stale, un-reset values from previous runs. [ ... ] > @@ -747,6 +857,10 @@ static const struct file_operations monitoring_on_fops = > { > > static void destroy_monitor_dir(struct rv_monitor *mon) > { > +#ifdef CONFIG_RV_EDGE_STAT > + free_percpu(mon->edge_pcpu); > + mon->edge_pcpu = NULL; > +#endif > rv_remove(mon->root_d); > } [Severity: High] Could this lead to a use-after-free if the tracefs stats file is read while the monitor is being unregistered? The per-cpu buffer is explicitly freed here before the rv_remove() synchronization point removes the tracefs file. A concurrent lockless reader using rv_edge_stats_show() could access the freed mon->edge_pcpu memory: kernel/trace/rv/rv.c:rv_edge_stats_show() { ... if (!mon->edge_pcpu) return 0; for_each_online_cpu(cpu) { struct rv_edge_stat *s = per_cpu_ptr(mon->edge_pcpu, cpu); ... } Should rv_remove() be called before freeing the per-CPU buffer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
