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

[Steven Rostedt <rostedt@goodmis.org>]

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.


> 
> 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