Re: [RFC PATCH v5 1/3] trace: add lock-free stackmap for stack trace deduplication

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ftrace_stackmap_reset() omits ring buffer clearing and tracing state verification, violating the design claimed in the commit message.
--

commit 5edd2c96008787e0c0b7c542cb960e049ed8c84f
Author: Pengfei Li <lipengfei28@xiaomi.com>

trace: add lock-free stackmap for stack trace deduplication

This commit adds a lock-free hash map (ftrace_stackmap) to deduplicate kernel
stack traces for the ftrace ring buffer. By storing a 4-byte stack_id instead
of full traces, it reduces buffer usage while providing NMI-safe, lock-free
inserts and lookups.

> diff --git a/kernel/trace/trace_stackmap.c b/kernel/trace/trace_stackmap.c
> new file mode 100644
> index 0000000000000..6d938aa414dca
> --- /dev/null
> +++ b/kernel/trace/trace_stackmap.c
[ ... ]
> +int ftrace_stackmap_reset(struct ftrace_stackmap *smap)
> +{
> +	int cpu;
> +
> +	if (!smap)
> +		return 0;
> +
> +	if (atomic_cmpxchg(&smap->resetting, 0, 1) != 0)
> +		return -EBUSY;
> +
> +	/*
> +	 * synchronize_rcu() itself is a full barrier; no extra smp_mb()
> +	 * is needed before it. It drains in-flight ftrace callbacks that
> +	 * may have already passed the resetting check with the old value.
> +	 */
> +	synchronize_rcu();

[Severity: Medium]
The commit message states: "Reset uses atomic_cmpxchg() to claim the resetting
flag, then verifies tracer_tracing_is_on() returns false." 

Is there a missing check here? It looks like ftrace_stackmap_reset() omits
the tracing state verification entirely, meaning a reset could proceed while
tracing is active despite the design claims.

[ ... ]
> +	/*
> +	 * Take the reader_sem in exclusive mode. This serializes the
> +	 * memset against any tracefs reader (seq_file iteration or
> +	 * stack_map_bin snapshot) that may currently hold the rwsem
> +	 * for read. synchronize_rcu() already drained the hot path;
> +	 * this rwsem covers process-context readers that aren't
> +	 * preempt-disabled.
> +	 */
> +	down_write(&smap->reader_sem);
> +
> +	memset(smap->entries, 0, sizeof(*smap->entries) * smap->map_size);

[Severity: Medium]
The commit message explicitly states: "Reset is destructive: under the
reader_sem write lock it clears the owning trace_array's ring buffer (and
snapshot buffer) BEFORE the map, so an external observer never sees 'trace
still has <stack_id N> but the map is already empty'. The buffers are cleared
with tracing_reset_all_cpus()".

Should tracing_reset_all_cpus() be called before this memset? As it is
implemented, the map is cleared without the ring buffer being cleared first.

Could this lead to misleading or corrupted trace data resolution from the
user's perspective, violating the design outlined in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902064242.28606-1-lipengfei28@xiaomi.com?part=1