Re: [PATCH v7 15/20] tracing/remotes: Add dump_on_panic tracefs file
From: sashiko-bot@kernel.org
Date: 2026-09-24 15:15:09
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] trace: trace_remote_panic_handler deadlocks on rb_reader_lock during panic - [Low] cleanup: mixing goto-based error handling with scope-based cleanup -- commit 66fa48a367eccf4815a02a5d4904f1fcccdbd1d5 Author: Vincent Donnefort [off-list ref] tracing/remotes: Add dump_on_panic tracefs file This commit adds a dump_on_panic tracefs file for trace remotes. When enabled, it registers a panic notifier that attempts to read and dump the contents of the 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 ea0fc347dff86..0a360690b174e 100644 --- a/kernel/trace/trace_remote.c +++ b/kernel/trace/trace_remote.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -435,58 +457,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: Low] This isn't a functional bug, but does this mix goto-based error handling with scope-based cleanup? The cleanup subsystem guidelines suggest converting all resources in a given function to scope-based cleanup or converting none, as mixing the paradigms can create confusing ownership semantics. [ ... ]
- return iter;
}
- ret = -ENOMEM;
-err:
- kfree(iter);
- trace_remote_put(remote);
+ return no_free_ptr(iter);
+err:
+ switch (type) {
+ case TRI_PANIC:
+ break;
+ default:
+ trace_remote_put(remote);
+ }
return ERR_PTR(ret);
}[ ... ]
quoted hunk ↗ jump to hunk
@@ -990,6 +1026,118 @@ 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]); + } + } + + while (trace_remote_iter_read_event(iter)) { + trace_seq_init(&iter->seq);
[Severity: High]
Could this sequence permanently deadlock the system during a kernel panic?
If a panic originates from a non-NMI context (such as a BUG() or sysrq-c),
the panicking CPU halts all other CPUs via smp_send_stop().
If one of those halted CPUs was interrupted while holding its per-CPU
reader_lock, it will never release it. The panicking CPU executing this
handler iterates over all CPUs and calls ring_buffer_read_remote_meta_page().
Looking at ring_buffer_read_remote_meta_page(), it unconditionally acquires
the reader_lock:
int ring_buffer_read_remote_meta_page(struct trace_buffer *buffer, int cpu)
{
...
cpu_buffer = buffer->buffers[cpu];
local_irq_save(flags);
dolock = rb_reader_lock(cpu_buffer);
rb_read_remote_meta_page(cpu_buffer);
...
}
Furthermore, the loop condition trace_remote_iter_read_event() relies on
ring_buffer_empty_cpu(), which also attempts to acquire reader_lock.
Since rb_reader_lock() acquires a standard spinlock rather than using a
trylock, wouldn't the panicking CPU spin infinitely waiting for the halted
CPU's lock, preventing the crash dump and subsequent reboot?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260924145754.1285146-1-vdonnefort@google.com?part=15