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 <[email protected]> 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/[email protected]?part=1
