Re: [PATCH v5 13/18] tracing/remotes: Add dump_on_panic tracefs file
From: sashiko-bot@kernel.org
Date: 2026-08-12 17:08:27
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Deadlock in panic handler due to unconditionally acquiring raw spinlocks that may be held by stopped CPUs. - [Low] Mixing goto-based error handling with scope-based `__free()` cleanup. --
--- Patch [13]: [PATCH v5 13/18] tracing/remotes: Add dump_on_panic tracefs file ---commit 0e10ff19e3851c2a033a99ee96a39ed51f4786e7
Author: Vincent Donnefort [off-list ref]
tracing/remotes: Add dump_on_panic tracefs file
When enabled, dump_on_panic will dump the content of the trace remote
buffer 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); int ret; lockdep_assert_held(&remote->lock); - if (type == TRI_NONCONSUMING && !trace_remote_loaded(remote)) - return NULL; + if (!iter) + return ERR_PTR(-ENOMEM);
[ ... ]
if (!trace_remote_has_cpu(remote, cpu)) {
ret = -ENODEV;
goto err;^^^^^^^^ [Severity: Low] Does this mix goto-based error handling with scope-based __free() cleanup? The iter variable was declared with __free(kfree) at the top of the function, but this goto jumps to an error label. Mixing goto labels and cleanup scope decorators in the same function can create confusing ownership semantics and violates cleanup guidelines. Could this be converted entirely to scope-based cleanup, or avoid using __free() here? [ ... ]
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 these functions deadlock the panic handler?
During a panic, smp_send_stop() halts all other CPUs. If a remote CPU was
reading the trace buffer and holding the reader_lock when it was halted,
won't this spin forever?
ring_buffer_read_remote_meta_page() and ring_buffer_iter_reset() both call
rb_reader_lock(), which unconditionally takes a raw_spin_lock() when not in
NMI context:
kernel/trace/ring_buffer.c:rb_reader_lock() {
...
if (likely(!in_nmi())) {
raw_spin_lock(&cpu_buffer->reader_lock);
return true;
}
...
}
Because standard panics do not execute in NMI context, in_nmi() returns false
here, leading to a hang that prevents kdump or reboot.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812161340.2417322-1-vdonnefort@google.com?part=13