| Message ID | 20240304174341.2a561d9f@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5efd3e2aef91d2d812290dcb25b2058e6f3f532c |
| Headers | show |
| Series | tracing: Remove precision vsnprintf() check from print event | expand |
On 2024-03-04 17:43, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > This reverts 60be76eeabb3d ("tracing: Add size check when printing > trace_marker output"). The only reason the precision check was added > was because of a bug that miscalculated the write size of the string into > the ring buffer and it truncated it removing the terminating nul byte. On > reading the trace it crashed the kernel. But this was due to the bug in > the code that happened during development and should never happen in > practice. If anything, the precision can hide bugs where the string in the > ring buffer isn't nul terminated and it will not be checked. > > Link: https://lore.kernel.org/all/C7E7AF1A-D30F-4D18-B8E5-AF1EF58004F5@linux.ibm.com/ > Link: https://lore.kernel.org/linux-trace-kernel/20240227125706.04279ac2@gandalf.local.home > Link: https://lore.kernel.org/all/20240302111244.3a1674be@gandalf.local.home/ > > Reported-by: Sachin Sant <sachinp@linux.ibm.com> > Fixes: 60be76eeabb3d ("tracing: Add size check when printing trace_marker output") > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> This is a step in the right direction IMHO. Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Just out of curiosity, is there anything to prevent trace_marker from writing a huge string into the ring buffer in the first place ? Is this limit implicit and based on the page size of the architecture or is it a known fixed limit ? (e.g. 4kB strings). It appears to currently be limited by #define TRACE_SEQ_BUFFER_SIZE (PAGE_SIZE * 2 - \ (sizeof(struct seq_buf) + sizeof(size_t) + sizeof(int))) checked within tracing_mark_write(). I would have hoped for a simpler limit (e.g. 4kB) consistent across architectures. But that would belong to a separate change. Thanks, Mathieu
On Mon, 4 Mar 2024 18:23:41 -0500 Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > It appears to currently be limited by > > #define TRACE_SEQ_BUFFER_SIZE (PAGE_SIZE * 2 - \ > (sizeof(struct seq_buf) + sizeof(size_t) + sizeof(int))) > > checked within tracing_mark_write(). Yeah, I can hard code this to 8K as it handles output of complete events, that can dump a lot of data, and then limit the trace_marker writes to be 4K. -- Steve
On Mon, 4 Mar 2024 18:55:00 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 4 Mar 2024 18:23:41 -0500 > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > It appears to currently be limited by > > > > #define TRACE_SEQ_BUFFER_SIZE (PAGE_SIZE * 2 - \ > > (sizeof(struct seq_buf) + sizeof(size_t) + sizeof(int))) > > > > checked within tracing_mark_write(). > > Yeah, I can hard code this to 8K as it handles output of complete events, > that can dump a lot of data, and then limit the trace_marker writes to be 4K. Actually, the trace_marker writes is already limited by TRACE_SEQ_BUFFER_SIZE, and by making this hard coded to 8K, it limits the size of the trace_marker writes. I may make the writes even smaller. -- Steve
> On 05-Mar-2024, at 4:13 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > This reverts 60be76eeabb3d ("tracing: Add size check when printing > trace_marker output"). The only reason the precision check was added > was because of a bug that miscalculated the write size of the string into > the ring buffer and it truncated it removing the terminating nul byte. On > reading the trace it crashed the kernel. But this was due to the bug in > the code that happened during development and should never happen in > practice. If anything, the precision can hide bugs where the string in the > ring buffer isn't nul terminated and it will not be checked. > > Link: https://lore.kernel.org/all/C7E7AF1A-D30F-4D18-B8E5-AF1EF58004F5@linux.ibm.com/ > Link: https://lore.kernel.org/linux-trace-kernel/20240227125706.04279ac2@gandalf.local.home > Link: https://lore.kernel.org/all/20240302111244.3a1674be@gandalf.local.home/ > > Reported-by: Sachin Sant <sachinp@linux.ibm.com> > Fixes: 60be76eeabb3d ("tracing: Add size check when printing trace_marker output") > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- This fixes the reported problem for me. All the ftrace selftests complete without any fails. # of passed: 121 # of failed: 0 # of unresolved: 6 # of untested: 0 # of unsupported: 7 # of xfailed: 1 # of undefined(test bug): 0 Tested-by: Sachin Sant <sachinp@linux.ibm.com> — Sachin
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c index 3e7fa44dc2b2..d8b302d01083 100644 --- a/kernel/trace/trace_output.c +++ b/kernel/trace/trace_output.c @@ -1587,12 +1587,11 @@ static enum print_line_t trace_print_print(struct trace_iterator *iter, { struct print_entry *field; struct trace_seq *s = &iter->seq; - int max = iter->ent_size - offsetof(struct print_entry, buf); trace_assign_type(field, iter->ent); seq_print_ip_sym(s, field->ip, flags); - trace_seq_printf(s, ": %.*s", max, field->buf); + trace_seq_printf(s, ": %s", field->buf); return trace_handle_return(s); } @@ -1601,11 +1600,10 @@ static enum print_line_t trace_print_raw(struct trace_iterator *iter, int flags, struct trace_event *event) { struct print_entry *field; - int max = iter->ent_size - offsetof(struct print_entry, buf); trace_assign_type(field, iter->ent); - trace_seq_printf(&iter->seq, "# %lx %.*s", field->ip, max, field->buf); + trace_seq_printf(&iter->seq, "# %lx %s", field->ip, field->buf); return trace_handle_return(&iter->seq); }