[PATCH v5] trace: acquire buffer from temparary trace sequence

[PATCH v5] trace: acquire buffer from temparary trace sequence

[Linyu Yuan]

there is one dwc3 trace event declare as below,
DECLARE_EVENT_CLASS(dwc3_log_event,
	TP_PROTO(u32 event, struct dwc3 *dwc),
	TP_ARGS(event, dwc),
	TP_STRUCT__entry(
		__field(u32, event)
		__field(u32, ep0state)
		__dynamic_array(char, str, DWC3_MSG_MAX)
	),
	TP_fast_assign(
		__entry->event = event;
		__entry->ep0state = dwc->ep0state;
	),
	TP_printk("event (%08x): %s", __entry->event,
			dwc3_decode_event(__get_str(str), DWC3_MSG_MAX,
				__entry->event, __entry->ep0state))
);
the problem is when trace function called, it will allocate up to
DWC3_MSG_MAX bytes from trace event buffer, but never fill the buffer
during fast assignment, it only fill the buffer when output function are
called, so this means if output function are not called, the buffer will
never used.

add __get_buf(len) which acquiree buffer from iter->tmp_seq when trace
output function called, it allow user write string to acquired buffer.

the mentioned dwc3 trace event will changed as below,
DECLARE_EVENT_CLASS(dwc3_log_event,
	TP_PROTO(u32 event, struct dwc3 *dwc),
	TP_ARGS(event, dwc),
	TP_STRUCT__entry(
		__field(u32, event)
		__field(u32, ep0state)
	),
	TP_fast_assign(
		__entry->event = event;
		__entry->ep0state = dwc->ep0state;
	),
	TP_printk("event (%08x): %s", __entry->event,
		dwc3_decode_event(__get_buf(DWC3_MSG_MAX), DWC3_MSG_MAX,
				__entry->event, __entry->ep0state))
);.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---

v5: move WARN_ON_ONCE into function
v4: no change
v3: fix comment from maintainer in v2

 include/linux/trace_seq.h                  |  5 +++++
 include/trace/stages/stage3_trace_output.h |  3 +++
 include/trace/stages/stage7_class_define.h |  1 +
 kernel/trace/trace_seq.c                   | 23 +++++++++++++++++++++++
 4 files changed, 32 insertions(+)

diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
index 0c4c758..6be92bf 100644
--- a/include/linux/trace_seq.h
+++ b/include/linux/trace_seq.h
@@ -95,6 +95,7 @@ extern void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 extern int trace_seq_hex_dump(struct trace_seq *s, const char *prefix_str,
 			      int prefix_type, int rowsize, int groupsize,
 			      const void *buf, size_t len, bool ascii);
+char *trace_seq_acquire(struct trace_seq *s, unsigned int len);
 
 #else /* CONFIG_TRACING */
 static inline __printf(2, 3)
@@ -139,6 +140,10 @@ static inline int trace_seq_path(struct trace_seq *s, const struct path *path)
 {
 	return 0;
 }
+static inline char *trace_seq_acquire(struct trace_seq *s, unsigned int len)
+{
+	return NULL;
+}
 #endif /* CONFIG_TRACING */
 
 #endif /* _LINUX_TRACE_SEQ_H */
diff --git a/include/trace/stages/stage3_trace_output.h b/include/trace/stages/stage3_trace_output.h
index 66374df..c1fb135 100644
--- a/include/trace/stages/stage3_trace_output.h
+++ b/include/trace/stages/stage3_trace_output.h
@@ -139,3 +139,6 @@
 		u64 ____val = (u64)(value);		\
 		(u32) do_div(____val, NSEC_PER_SEC);	\
 	})
+
+#undef __get_buf
+#define __get_buf(len)		trace_seq_acquire(p, (len))
diff --git a/include/trace/stages/stage7_class_define.h b/include/trace/stages/stage7_class_define.h
index 8795429..bcb960d 100644
--- a/include/trace/stages/stage7_class_define.h
+++ b/include/trace/stages/stage7_class_define.h
@@ -23,6 +23,7 @@
 #undef __get_rel_sockaddr
 #undef __print_array
 #undef __print_hex_dump
+#undef __get_buf
 
 /*
  * The below is not executed in the kernel. It is only what is
diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c
index 9c90b3a..e5e2992 100644
--- a/kernel/trace/trace_seq.c
+++ b/kernel/trace/trace_seq.c
@@ -403,3 +403,26 @@ int trace_seq_hex_dump(struct trace_seq *s, const char *prefix_str,
 	return 1;
 }
 EXPORT_SYMBOL(trace_seq_hex_dump);
+
+/*
+ * trace_seq_acquire - acquire seq buffer with size len
+ * @s: trace sequence descriptor
+ * @len: size of buffer to be acquired
+ *
+ * acquire buffer with size of @len from trace_seq for output usage,
+ * user can fill string into that buffer.
+ *
+ * Returns start address of acquired buffer.
+ *
+ * it allow multiple usage in one trace output function call.
+ */
+char *trace_seq_acquire(struct trace_seq *s, unsigned int len)
+{
+	char *ret = trace_seq_buffer_ptr(s);
+
+	if (!WARN_ON_ONCE(seq_buf_buffer_left(&s->seq) < len))
+		seq_buf_commit(&s->seq, len);
+
+	return ret;
+}
+EXPORT_SYMBOL(trace_seq_acquire);
-- 
2.7.4

Re: [PATCH v5] trace: acquire buffer from temparary trace sequence

[Linyu Yuan]

@Steven current design is not safe, as user still can write to buffer 
which have no enough space.

do you think it is better to improve dwc3 trace in fast assign path ?


On 1/30/2023 3:54 PM, Linyu Yuan wrote:
> there is one dwc3 trace event declare as below,
> DECLARE_EVENT_CLASS(dwc3_log_event,
> 	TP_PROTO(u32 event, struct dwc3 *dwc),
> 	TP_ARGS(event, dwc),
> 	TP_STRUCT__entry(
> 		__field(u32, event)
> 		__field(u32, ep0state)
> 		__dynamic_array(char, str, DWC3_MSG_MAX)
> 	),
> 	TP_fast_assign(
> 		__entry->event = event;
> 		__entry->ep0state = dwc->ep0state;
> 	),
> 	TP_printk("event (%08x): %s", __entry->event,
> 			dwc3_decode_event(__get_str(str), DWC3_MSG_MAX,
> 				__entry->event, __entry->ep0state))
> );
> the problem is when trace function called, it will allocate up to
> DWC3_MSG_MAX bytes from trace event buffer, but never fill the buffer
> during fast assignment, it only fill the buffer when output function are
> called, so this means if output function are not called, the buffer will
> never used.
>
> add __get_buf(len) which acquiree buffer from iter->tmp_seq when trace
> output function called, it allow user write string to acquired buffer.
>
> the mentioned dwc3 trace event will changed as below,
> DECLARE_EVENT_CLASS(dwc3_log_event,
> 	TP_PROTO(u32 event, struct dwc3 *dwc),
> 	TP_ARGS(event, dwc),
> 	TP_STRUCT__entry(
> 		__field(u32, event)
> 		__field(u32, ep0state)
> 	),
> 	TP_fast_assign(
> 		__entry->event = event;
> 		__entry->ep0state = dwc->ep0state;
> 	),
> 	TP_printk("event (%08x): %s", __entry->event,
> 		dwc3_decode_event(__get_buf(DWC3_MSG_MAX), DWC3_MSG_MAX,
> 				__entry->event, __entry->ep0state))
> );.
>
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
>
> v5: move WARN_ON_ONCE into function
> v4: no change
> v3: fix comment from maintainer in v2
>
>   include/linux/trace_seq.h                  |  5 +++++
>   include/trace/stages/stage3_trace_output.h |  3 +++
>   include/trace/stages/stage7_class_define.h |  1 +
>   kernel/trace/trace_seq.c                   | 23 +++++++++++++++++++++++
>   4 files changed, 32 insertions(+)
>
> diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
> index 0c4c758..6be92bf 100644
> --- a/include/linux/trace_seq.h
> +++ b/include/linux/trace_seq.h
> @@ -95,6 +95,7 @@ extern void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
>   extern int trace_seq_hex_dump(struct trace_seq *s, const char *prefix_str,
>   			      int prefix_type, int rowsize, int groupsize,
>   			      const void *buf, size_t len, bool ascii);
> +char *trace_seq_acquire(struct trace_seq *s, unsigned int len);
>   
>   #else /* CONFIG_TRACING */
>   static inline __printf(2, 3)
> @@ -139,6 +140,10 @@ static inline int trace_seq_path(struct trace_seq *s, const struct path *path)
>   {
>   	return 0;
>   }
> +static inline char *trace_seq_acquire(struct trace_seq *s, unsigned int len)
> +{
> +	return NULL;
> +}
>   #endif /* CONFIG_TRACING */
>   
>   #endif /* _LINUX_TRACE_SEQ_H */
> diff --git a/include/trace/stages/stage3_trace_output.h b/include/trace/stages/stage3_trace_output.h
> index 66374df..c1fb135 100644
> --- a/include/trace/stages/stage3_trace_output.h
> +++ b/include/trace/stages/stage3_trace_output.h
> @@ -139,3 +139,6 @@
>   		u64 ____val = (u64)(value);		\
>   		(u32) do_div(____val, NSEC_PER_SEC);	\
>   	})
> +
> +#undef __get_buf
> +#define __get_buf(len)		trace_seq_acquire(p, (len))
> diff --git a/include/trace/stages/stage7_class_define.h b/include/trace/stages/stage7_class_define.h
> index 8795429..bcb960d 100644
> --- a/include/trace/stages/stage7_class_define.h
> +++ b/include/trace/stages/stage7_class_define.h
> @@ -23,6 +23,7 @@
>   #undef __get_rel_sockaddr
>   #undef __print_array
>   #undef __print_hex_dump
> +#undef __get_buf
>   
>   /*
>    * The below is not executed in the kernel. It is only what is
> diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c
> index 9c90b3a..e5e2992 100644
> --- a/kernel/trace/trace_seq.c
> +++ b/kernel/trace/trace_seq.c
> @@ -403,3 +403,26 @@ int trace_seq_hex_dump(struct trace_seq *s, const char *prefix_str,
>   	return 1;
>   }
>   EXPORT_SYMBOL(trace_seq_hex_dump);
> +
> +/*
> + * trace_seq_acquire - acquire seq buffer with size len
> + * @s: trace sequence descriptor
> + * @len: size of buffer to be acquired
> + *
> + * acquire buffer with size of @len from trace_seq for output usage,
> + * user can fill string into that buffer.
> + *
> + * Returns start address of acquired buffer.
> + *
> + * it allow multiple usage in one trace output function call.
> + */
> +char *trace_seq_acquire(struct trace_seq *s, unsigned int len)
> +{
> +	char *ret = trace_seq_buffer_ptr(s);
> +
> +	if (!WARN_ON_ONCE(seq_buf_buffer_left(&s->seq) < len))
> +		seq_buf_commit(&s->seq, len);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(trace_seq_acquire);

Re: [PATCH v5] trace: acquire buffer from temparary trace sequence

[Steven Rostedt]

On Mon, 30 Jan 2023 21:14:01 +0800
Linyu Yuan <quic_linyyuan@quicinc.com> wrote:

> @Steven current design is not safe, as user still can write to buffer 
> which have no enough space.

I'm assuming that it would never use more that DWC3_MSG_MAX, and that's set
to 500. The size of the trace_seq() is 4096 (or PAGE_SIZE), I doubt it will
ever be an issue.

How do you think the user can still write more than enough?

-- Steve

> 
> do you think it is better to improve dwc3 trace in fast assign path ?
> 

Re: [PATCH v5] trace: acquire buffer from temparary trace sequence

[Linyu Yuan]

On 1/31/2023 3:30 AM, Steven Rostedt wrote:
> On Mon, 30 Jan 2023 21:14:01 +0800
> Linyu Yuan <quic_linyyuan@quicinc.com> wrote:
>
>> @Steven current design is not safe, as user still can write to buffer
>> which have no enough space.
> I'm assuming that it would never use more that DWC3_MSG_MAX, and that's set
> to 500. The size of the trace_seq() is 4096 (or PAGE_SIZE), I doubt it will
> ever be an issue.
>
> How do you think the user can still write more than enough?


yes, agree it will be safe for dwc3, but i don't know if any possible 
hacker,

as the function always return a valid pointer even when hacker input a 
large size.


>
> -- Steve
>
>> do you think it is better to improve dwc3 trace in fast assign path ?
>>
>

Re: [PATCH v5] trace: acquire buffer from temparary trace sequence

[Steven Rostedt]

On Tue, 31 Jan 2023 10:06:22 +0800
Linyu Yuan <quic_linyyuan@quicinc.com> wrote:

> yes, agree it will be safe for dwc3, but i don't know if any possible 
> hacker,
> 
> as the function always return a valid pointer even when hacker input a 
> large size.

But gives a nice big warning if that's the case. This is not something that
can be modified by user input. We do not need to worry about
kernel implementations that could overflow (and trigger a WARN_ON() when
they do). Especially since the max size is greater than the max size of the
content of an event.

A lot of systems that worry about hackers enable "panic_on_warn" which
means that if the WARN_ON() triggers, the machine will crash, which will at
most cause a DOS, but not something people can use to hack into the machine
with.

-- Steve

Re: [PATCH v5] trace: acquire buffer from temparary trace sequence

[Linyu Yuan]

On 1/31/2023 10:37 AM, Steven Rostedt wrote:
> On Tue, 31 Jan 2023 10:06:22 +0800
> Linyu Yuan <quic_linyyuan@quicinc.com> wrote:
>
>> yes, agree it will be safe for dwc3, but i don't know if any possible
>> hacker,
>>
>> as the function always return a valid pointer even when hacker input a
>> large size.
> But gives a nice big warning if that's the case. This is not something that
> can be modified by user input. We do not need to worry about
> kernel implementations that could overflow (and trigger a WARN_ON() when
> they do). Especially since the max size is greater than the max size of the
> content of an event.
>
> A lot of systems that worry about hackers enable "panic_on_warn" which
> means that if the WARN_ON() triggers, the machine will crash, which will at
> most cause a DOS, but not something people can use to hack into the machine
> with.


thanks, please help review the change.


>
> -- Steve

Re: [PATCH v5] trace: acquire buffer from temparary trace sequence

[Steven Rostedt]

On Tue, 31 Jan 2023 10:49:37 +0800
Linyu Yuan <quic_linyyuan@quicinc.com> wrote:

> > A lot of systems that worry about hackers enable "panic_on_warn" which
> > means that if the WARN_ON() triggers, the machine will crash, which will at
> > most cause a DOS, but not something people can use to hack into the machine
> > with.  
> 
> 
> thanks, please help review the change.

I'm fine with it. I can pull this into my tree (after it goes through all
my testing with the other patches in queue).

Re: [PATCH v5] trace: acquire buffer from temparary trace sequence

[Linyu Yuan]

On 1/31/2023 11:58 AM, Steven Rostedt wrote:
> On Tue, 31 Jan 2023 10:49:37 +0800
> Linyu Yuan <quic_linyyuan@quicinc.com> wrote:
>
>>> A lot of systems that worry about hackers enable "panic_on_warn" which
>>> means that if the WARN_ON() triggers, the machine will crash, which will at
>>> most cause a DOS, but not something people can use to hack into the machine
>>> with.
>>
>> thanks, please help review the change.
> I'm fine with it. I can pull this into my tree (after it goes through all
> my testing with the other patches in queue).


thanks, once it merged into mainline, I will send patch for related modules.