Re: [RFC 2/3] rv: add per-monitor edge-stat facility and stats file
flat view
From: sashiko-bot@kernel.org
Date: 2026-08-27 07:39:31
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+ } + return retval; }
[ ... ]
quoted hunk ↗ jump to hunk
+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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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/20260827072400.45734-1-tobias.schaffner@siemens.com?part=2