On Mon, Aug 17, 2026 at 02:10:19PM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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.
>
> > 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
>
> [ ... ]
>
> > @@ -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?
>
> [ ... ]
>
> > @@ -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()?
Makes sense. And actually ftrace_dump_one does that too!
>
> > +
> > + 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/[email protected]?part=13
--
Vincent
