Re: [RFC 2/3] rv: add per-monitor edge-stat facility and stats file

[sashiko-bot@kernel.org]

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 <tobias.schaffner@siemens.com>
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/20260827072400.45734-1-tobias.schaffner@siemens.com?part=2