Re: [PATCH v4 06/18] tracing/remotes: Add dmesg tracefs file

[Vincent Donnefort <vdonnefort@google.com>]

On Mon, Aug 10, 2026 at 06:42:16PM -0400, Steven Rostedt wrote:
> Adding Sashiko review: https://sashiko.dev/#/patchset/20260804140333.797210-2-vdonnefort%40google.com
> 
> > --- Patch [6]: [PATCH v4 06/18] tracing/remotes: Add dmesg tracefs file ---
> > commit b6a4c19257d91807c612baa19691b2d602bb0670
> > Author: Vincent Donnefort <vdonnefort@google.com>
> > 
> > tracing/remotes: Add dmesg tracefs file
> > 
> > When enabled, the dmesg tracefs file enables the redirection of all
> > events to dmesg. This is similar to tp_printk.
> > 
> > > diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c
> > > index 1fbe2f3f20e99..58a38f3d49024 100644
> > > --- a/kernel/trace/trace_remote.c
> > > +++ b/kernel/trace/trace_remote.c
> > 
> > > [ ... ]
> > 
> > > @@ -882,6 +908,87 @@ static const struct file_operations trace_fops = {
> > >  	.release	= trace_release,
> > >  };
> > >  
> > > +static bool trace_remote_do_dmesg(struct trace_remote *remote)
> > > +{
> > > +	struct trace_remote_iterator *iter = smp_load_acquire(&remote->dmesg);
> > > +	unsigned int max_events = 1000;
> > > +
> > > +	if (!iter)
> > > +		return false;
> > > +
> > > +	trace_remote_iter_read_start(iter);
> > > +
> > > +	while (trace_remote_iter_read_event(iter)) {
> > > +		trace_seq_init(&iter->seq);
> > > +
> > > +		trace_remote_iter_print_event(iter);
> > > +		trace_seq_putc(&iter->seq, 0);
> > > +		if (!pr_info("%s", iter->seq.buffer))
> > 
> > If the remote trace event exceeds the PAGE_SIZE sequence buffer, does
> > trace_seq_putc() in trace_remote_do_dmesg() fail to append the null byte?
> > 
> > Since iter->seq.buffer might not be null-terminated in this case, could this
> > lead to an out-of-bounds read when pr_info() prints the string?
> 
> Yeah, we may want to have it be:
> 
> 		if (!pr_info("%s", seq_buf_str(&iter->seq))
> 
> Which will make sure the buffer is nul terminated.

ack, and same for trace_remote_panic_handler()

> 
> 
> > 
> > This isn't a bug introduced by this patch, but the same pattern appears
> > to exist in the pre-existing trace_remote_panic_handler() with pr_emerg().
> > 
> > If the kernel is built with CONFIG_PRINTK disabled, pr_info() evaluates to 0.
> > 
> > Will this cause the loop in trace_remote_do_dmesg() to break without calling
> > trace_remote_iter_move(), leaving the event unconsumed and permanently
> > stalling the workqueue?
> > 
> > > +			break;
> > > +
> > > +		trace_remote_iter_move(iter);
> > > +
> > > +		if (!(--max_events))
> > > +			break;
> > > +	}
> > > +
> > > +	trace_remote_iter_read_finished(iter);
> > > +
> > > +	return !max_events;
> > > +}
> > 
> > While reviewing this, I also noticed a pre-existing issue in trace_pipe_read()
> > when handling oversized events:
> > 
> > kernel/trace/trace_remote.c:trace_pipe_read() {
> >         ...
> >         if (trace_remote_iter_print_event(iter)) {
> >                 iter->seq.seq.len = prev_len;
> >                 break;
> >         }
> >         ...
> > }
> > 
> > This is a pre-existing issue, but if an oversized event causes
> > trace_remote_iter_print_event() to return -EOVERFLOW, will this break
> > the loop without calling trace_remote_iter_move()?
> > 
> > Since the event is left unconsumed in the buffer, could this lead to an
> > uninterruptible infinite loop (hard lockup) when reading the trace pipe?
> 
> I'll let you answer the above.
> 
> -- Steve

I don't believe this can realistically happen, trace_seq buffer is 8K and remote
events are really small. 

-- 
Vincent