[PATCH] tracing: Remove precision vsnprintf() check from print event

[PATCH] tracing: Remove precision vsnprintf() check from print event

[Steven Rostedt]

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>
---
 kernel/trace/trace_output.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

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);
 }
-- 
2.43.0

Re: [PATCH] tracing: Remove precision vsnprintf() check from print event

[Mathieu Desnoyers]

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

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH] tracing: Remove precision vsnprintf() check from print event

[Steven Rostedt]

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

Re: [PATCH] tracing: Remove precision vsnprintf() check from print event

[Steven Rostedt]

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

Re: [PATCH] tracing: Remove precision vsnprintf() check from print event

[Sachin Sant]

> 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