Re: [PATCH v6 13/18] tracing/remotes: Add dump_on_panic tracefs file
From: sashiko-bot@kernel.org
Date: 2026-08-17 14:10:20
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The panic handler unconditionally attempts to acquire a per-CPU reader spinlock that may be held by a CPU halted by smp_send_stop(), leading to a permanent deadlock during kernel panic. - [High] Missing touch_nmi_watchdog() in the panic dump loop can cause a secondary panic or hard reset. - [Medium] Mixing goto-based error handling with __free() scope-based cleanup in trace_remote_iter(). - [Low] Missing READ_ONCE() when reading remote->panic_on in dump_on_panic_show(). --
--- Patch [13]: [PATCH v6 13/18] tracing/remotes: Add dump_on_panic tracefs file ---commit 33a5774bde5e05c9ff209cc5026a9b962c22df16 Author: Vincent Donnefort [off-list ref] tracing/remotes: Add dump_on_panic tracefs file This commit adds a dump_on_panic tracefs file. When enabled, it dumps the content of the trace remote buffer to the console if the system panics.
quoted hunk ↗ jump to hunk
diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c index 9e78267d71f8b..5536e0df43795 100644 --- a/kernel/trace/trace_remote.c +++ b/kernel/trace/trace_remote.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -434,58 +455,68 @@ static void trace_remote_dec_poll(struct trace_remote *remote) static struct trace_remote_iterator *trace_remote_iter(struct trace_remote *remote, int cpu, enum tri_type type) { - struct trace_remote_iterator *iter = NULL; + struct trace_remote_iterator *iter __free(kfree) = kzalloc_obj(*iter);
[Severity: Medium] This isn't a bug, but this function mixes scope-based cleanup (__free) with a goto-based error handling pattern. According to the cleanup subsystem guidelines, mixing goto-based cleanup labels and __free() declarations in the same function is forbidden because it creates confusing ownership semantics. Should this function either use goto-based cleanup exclusively, or be fully converted to scope-based cleanup? [ ... ]
quoted hunk ↗ jump to hunk
@@ -989,6 +1024,117 @@ static int dmesg_show(struct seq_file *s, void *unused) } DEFINE_TRACE_REMOTE_ATTRIBUTE(dmesg); +static int trace_remote_panic_handler(struct notifier_block *self, unsigned long ev, void *v) +{ + struct trace_remote *remote = container_of(self, struct trace_remote, panic_notifier); + struct trace_remote_iterator *iter = smp_load_acquire(&remote->panic_iter); + int cpu; + + if (!iter) { + pr_warn("Unexpected error: no panic iterator for the trace remote\n"); + return NOTIFY_DONE; + } + + for_each_possible_cpu(cpu) { + if (iter->rb_iters[cpu]) { + /* No RING_BUFFER_ALL_CPUS to avoid taking cpu_read_lock() */ + ring_buffer_read_remote_meta_page(remote->trace_buffer, cpu); + ring_buffer_iter_reset(iter->rb_iters[cpu]); + } + }
[Severity: High] Can this loop deadlock indefinitely during a panic? If another CPU is actively reading the remote trace buffer and holds its reader_lock, the panic() function will stop that CPU via smp_send_stop(). When the panicking CPU executes this loop, ring_buffer_read_remote_meta_page() calls rb_reader_lock(). If in_nmi() is false (which happens on a normal oops or SysRq-C), rb_reader_lock() will unconditionally attempt to acquire the lock held by the stopped CPU rather than using a trylock. Will this cause the panic recovery process to hang forever?
+
+ while (trace_remote_iter_read_event(iter)) {
+ trace_seq_init(&iter->seq);
+
+ trace_remote_iter_print_event(iter);
+ trace_seq_putc(&iter->seq, 0);
+ pr_emerg("%s", seq_buf_str(&iter->seq.seq));
+
+ trace_remote_iter_move(iter);
+ }[Severity: High] Since serial consoles can be slow and remote trace buffers can contain many events, could the time spent in this printing loop trigger the NMI watchdog and cause a hard reset before the panic dump completes? Does this loop need a call to touch_nmi_watchdog()?
+ + return NOTIFY_DONE; +}
[ ... ]
+static int dump_on_panic_show(struct seq_file *s, void *unused)
+{
+ struct trace_remote *remote = s->private;
+
+ seq_printf(s, "%d\n", remote->panic_on);[Severity: Low] Does this unprotected read of remote->panic_on need a READ_ONCE()? Since dump_on_panic_write() modifies this flag concurrently under remote->lock, this is formally a data race under the Linux kernel memory model and might be flagged by KCSAN.
+ + return 0; +}
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260817135517.3919534-1-vdonnefort@google.com?part=13