[PATCH v2] tracing: Limit trace_marker writes to just 4K

[PATCH v2] tracing: Limit trace_marker writes to just 4K

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Limit the max print event of trace_marker to just 4K string size. This must
also be less than the amount that can be held by a trace_seq along with
the text that is before the output (like the task name, PID, CPU, state,
etc). As trace_seq is made to handle large events (some greater than 4K).
Make the max size of a trace_marker write event be 4K which is guaranteed
to fit in the trace_seq buffer.

Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/20240304192710.4c99677c@gandalf.local.home/

- Just make the max limit 4K and not half of the trace_seq size.
  The trace_seq is already made to handle events greater than 4k.

 kernel/trace/trace.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8198bfc54b58..d16b95ca58a7 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -7293,6 +7293,8 @@ tracing_free_buffer_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+#define TRACE_MARKER_MAX_SIZE		4096
+
 static ssize_t
 tracing_mark_write(struct file *filp, const char __user *ubuf,
 					size_t cnt, loff_t *fpos)
@@ -7320,6 +7322,9 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
 	if ((ssize_t)cnt < 0)
 		return -EINVAL;
 
+	if (cnt > TRACE_MARKER_MAX_SIZE)
+		cnt = TRACE_MARKER_MAX_SIZE;
+
 	meta_size = sizeof(*entry) + 2;  /* add '\0' and possible '\n' */
  again:
 	size = cnt + meta_size;
@@ -7328,11 +7333,6 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
 	if (cnt < FAULTED_SIZE)
 		size += FAULTED_SIZE - cnt;
 
-	if (size > TRACE_SEQ_BUFFER_SIZE) {
-		cnt -= size - TRACE_SEQ_BUFFER_SIZE;
-		goto again;
-	}
-
 	buffer = tr->array_buffer.buffer;
 	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
 					    tracing_gen_ctx());
-- 
2.43.0

Re: [PATCH v2] tracing: Limit trace_marker writes to just 4K

[Mathieu Desnoyers]

On 2024-03-04 22:34, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Limit the max print event of trace_marker to just 4K string size. This must
> also be less than the amount that can be held by a trace_seq along with
> the text that is before the output (like the task name, PID, CPU, state,
> etc). As trace_seq is made to handle large events (some greater than 4K).
> Make the max size of a trace_marker write event be 4K which is guaranteed
> to fit in the trace_seq buffer.
> 
> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

 From my perspective I only attempted to clarify the point Linus made
about limiting the trace_marker input to 4kB. Feel adapt the
Suggested-by tag accordingly.

Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks,

Mathieu

> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/20240304192710.4c99677c@gandalf.local.home/
> 
> - Just make the max limit 4K and not half of the trace_seq size.
>    The trace_seq is already made to handle events greater than 4k.
> 
>   kernel/trace/trace.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 8198bfc54b58..d16b95ca58a7 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -7293,6 +7293,8 @@ tracing_free_buffer_release(struct inode *inode, struct file *filp)
>   	return 0;
>   }
>   
> +#define TRACE_MARKER_MAX_SIZE		4096
> +
>   static ssize_t
>   tracing_mark_write(struct file *filp, const char __user *ubuf,
>   					size_t cnt, loff_t *fpos)
> @@ -7320,6 +7322,9 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>   	if ((ssize_t)cnt < 0)
>   		return -EINVAL;
>   
> +	if (cnt > TRACE_MARKER_MAX_SIZE)
> +		cnt = TRACE_MARKER_MAX_SIZE;
> +
>   	meta_size = sizeof(*entry) + 2;  /* add '\0' and possible '\n' */
>    again:
>   	size = cnt + meta_size;
> @@ -7328,11 +7333,6 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>   	if (cnt < FAULTED_SIZE)
>   		size += FAULTED_SIZE - cnt;
>   
> -	if (size > TRACE_SEQ_BUFFER_SIZE) {
> -		cnt -= size - TRACE_SEQ_BUFFER_SIZE;
> -		goto again;
> -	}
> -
>   	buffer = tr->array_buffer.buffer;
>   	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
>   					    tracing_gen_ctx());

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

Re: [PATCH v2] tracing: Limit trace_marker writes to just 4K

[Masami Hiramatsu]

On Mon, 4 Mar 2024 22:34:33 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Limit the max print event of trace_marker to just 4K string size. This must
> also be less than the amount that can be held by a trace_seq along with
> the text that is before the output (like the task name, PID, CPU, state,
> etc). As trace_seq is made to handle large events (some greater than 4K).
> Make the max size of a trace_marker write event be 4K which is guaranteed
> to fit in the trace_seq buffer.
> 
> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

This looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

> ---
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/20240304192710.4c99677c@gandalf.local.home/
> 
> - Just make the max limit 4K and not half of the trace_seq size.
>   The trace_seq is already made to handle events greater than 4k.
> 
>  kernel/trace/trace.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 8198bfc54b58..d16b95ca58a7 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -7293,6 +7293,8 @@ tracing_free_buffer_release(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> +#define TRACE_MARKER_MAX_SIZE		4096
> +
>  static ssize_t
>  tracing_mark_write(struct file *filp, const char __user *ubuf,
>  					size_t cnt, loff_t *fpos)
> @@ -7320,6 +7322,9 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>  	if ((ssize_t)cnt < 0)
>  		return -EINVAL;
>  
> +	if (cnt > TRACE_MARKER_MAX_SIZE)
> +		cnt = TRACE_MARKER_MAX_SIZE;
> +
>  	meta_size = sizeof(*entry) + 2;  /* add '\0' and possible '\n' */
>   again:
>  	size = cnt + meta_size;
> @@ -7328,11 +7333,6 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>  	if (cnt < FAULTED_SIZE)
>  		size += FAULTED_SIZE - cnt;
>  
> -	if (size > TRACE_SEQ_BUFFER_SIZE) {
> -		cnt -= size - TRACE_SEQ_BUFFER_SIZE;
> -		goto again;
> -	}
> -
>  	buffer = tr->array_buffer.buffer;
>  	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
>  					    tracing_gen_ctx());
> -- 
> 2.43.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>