[PATCH] tracing/x86/xen: Remove zero data size trace events trace_xen_mmu_flush_tlb{_all}

[PATCH] tracing/x86/xen: Remove zero data size trace events trace_xen_mmu_flush_tlb{_all}

[Steven Rostedt]

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

Doing an audit of trace events, I discovered two trace events in the xen
subsystem that use a hack to create zero data size trace events. This is not
what trace events are for. Trace events add memory footprint overhead, and
if all you need to do is see if a function is hit or not, simply make that
function noinline and use function tracer filtering.

Worse yet, the hack used was:

 __array(char, x, 0)

Which creates a static string of zero in length. There's assumptions about
such constructs in ftrace that this is a dynamic string that is nul
terminated. This is not the case with these tracepoints and can cause
problems in various parts of ftrace.

Nuke the trace events!

Cc: stable@vger.kernel.org
Fixes: 95a7d76897c1e ("xen/mmu: Use Xen specific TLB flush instead of the generic one.")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/xen/mmu.c         |  4 +---
 arch/x86/xen/mmu_pv.c      |  4 +---
 include/trace/events/xen.h | 16 ----------------
 3 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index d33e7dbe3129..2d76106788a3 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -42,13 +42,11 @@ xmaddr_t arbitrary_virt_to_machine(void *vaddr)
 }
 EXPORT_SYMBOL_GPL(arbitrary_virt_to_machine);
 
-static void xen_flush_tlb_all(void)
+static noinline void xen_flush_tlb_all(void)
 {
 	struct mmuext_op *op;
 	struct multicall_space mcs;
 
-	trace_xen_mmu_flush_tlb_all(0);
-
 	preempt_disable();
 
 	mcs = xen_mc_entry(sizeof(*op));
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 486c0a34d00b..2c30cabfda90 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1310,13 +1310,11 @@ unsigned long xen_read_cr2_direct(void)
 	return this_cpu_read(xen_vcpu_info.arch.cr2);
 }
 
-static void xen_flush_tlb(void)
+static noinline void xen_flush_tlb(void)
 {
 	struct mmuext_op *op;
 	struct multicall_space mcs;
 
-	trace_xen_mmu_flush_tlb(0);
-
 	preempt_disable();
 
 	mcs = xen_mc_entry(sizeof(*op));
diff --git a/include/trace/events/xen.h b/include/trace/events/xen.h
index 7dd8f34c37df..fdcf88bcf0ea 100644
--- a/include/trace/events/xen.h
+++ b/include/trace/events/xen.h
@@ -352,22 +352,6 @@ DECLARE_EVENT_CLASS(xen_mmu_pgd,
 DEFINE_XEN_MMU_PGD_EVENT(xen_mmu_pgd_pin);
 DEFINE_XEN_MMU_PGD_EVENT(xen_mmu_pgd_unpin);
 
-TRACE_EVENT(xen_mmu_flush_tlb_all,
-	    TP_PROTO(int x),
-	    TP_ARGS(x),
-	    TP_STRUCT__entry(__array(char, x, 0)),
-	    TP_fast_assign((void)x),
-	    TP_printk("%s", "")
-	);
-
-TRACE_EVENT(xen_mmu_flush_tlb,
-	    TP_PROTO(int x),
-	    TP_ARGS(x),
-	    TP_STRUCT__entry(__array(char, x, 0)),
-	    TP_fast_assign((void)x),
-	    TP_printk("%s", "")
-	);
-
 TRACE_EVENT(xen_mmu_flush_tlb_one_user,
 	    TP_PROTO(unsigned long addr),
 	    TP_ARGS(addr),
-- 
2.13.6

Re: [PATCH] tracing/x86/xen: Remove zero data size trace events trace_xen_mmu_flush_tlb{_all}

[Steven Rostedt]

Thomas, Ingo or Peter.

Any of you willing to take this patch, or send me an ack and I'll take
it and push it to Linus?

-- Steve


On Wed, 9 May 2018 14:46:05 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Doing an audit of trace events, I discovered two trace events in the xen
> subsystem that use a hack to create zero data size trace events. This is not
> what trace events are for. Trace events add memory footprint overhead, and
> if all you need to do is see if a function is hit or not, simply make that
> function noinline and use function tracer filtering.
> 
> Worse yet, the hack used was:
> 
>  __array(char, x, 0)
> 
> Which creates a static string of zero in length. There's assumptions about
> such constructs in ftrace that this is a dynamic string that is nul
> terminated. This is not the case with these tracepoints and can cause
> problems in various parts of ftrace.
> 
> Nuke the trace events!
> 
> Cc: stable@vger.kernel.org
> Fixes: 95a7d76897c1e ("xen/mmu: Use Xen specific TLB flush instead of the generic one.")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/x86/xen/mmu.c         |  4 +---
>  arch/x86/xen/mmu_pv.c      |  4 +---
>  include/trace/events/xen.h | 16 ----------------
>  3 files changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index d33e7dbe3129..2d76106788a3 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -42,13 +42,11 @@ xmaddr_t arbitrary_virt_to_machine(void *vaddr)
>  }
>  EXPORT_SYMBOL_GPL(arbitrary_virt_to_machine);
>  
> -static void xen_flush_tlb_all(void)
> +static noinline void xen_flush_tlb_all(void)
>  {
>  	struct mmuext_op *op;
>  	struct multicall_space mcs;
>  
> -	trace_xen_mmu_flush_tlb_all(0);
> -
>  	preempt_disable();
>  
>  	mcs = xen_mc_entry(sizeof(*op));
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 486c0a34d00b..2c30cabfda90 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -1310,13 +1310,11 @@ unsigned long xen_read_cr2_direct(void)
>  	return this_cpu_read(xen_vcpu_info.arch.cr2);
>  }
>  
> -static void xen_flush_tlb(void)
> +static noinline void xen_flush_tlb(void)
>  {
>  	struct mmuext_op *op;
>  	struct multicall_space mcs;
>  
> -	trace_xen_mmu_flush_tlb(0);
> -
>  	preempt_disable();
>  
>  	mcs = xen_mc_entry(sizeof(*op));
> diff --git a/include/trace/events/xen.h b/include/trace/events/xen.h
> index 7dd8f34c37df..fdcf88bcf0ea 100644
> --- a/include/trace/events/xen.h
> +++ b/include/trace/events/xen.h
> @@ -352,22 +352,6 @@ DECLARE_EVENT_CLASS(xen_mmu_pgd,
>  DEFINE_XEN_MMU_PGD_EVENT(xen_mmu_pgd_pin);
>  DEFINE_XEN_MMU_PGD_EVENT(xen_mmu_pgd_unpin);
>  
> -TRACE_EVENT(xen_mmu_flush_tlb_all,
> -	    TP_PROTO(int x),
> -	    TP_ARGS(x),
> -	    TP_STRUCT__entry(__array(char, x, 0)),
> -	    TP_fast_assign((void)x),
> -	    TP_printk("%s", "")
> -	);
> -
> -TRACE_EVENT(xen_mmu_flush_tlb,
> -	    TP_PROTO(int x),
> -	    TP_ARGS(x),
> -	    TP_STRUCT__entry(__array(char, x, 0)),
> -	    TP_fast_assign((void)x),
> -	    TP_printk("%s", "")
> -	);
> -
>  TRACE_EVENT(xen_mmu_flush_tlb_one_user,
>  	    TP_PROTO(unsigned long addr),
>  	    TP_ARGS(addr),

Re: [PATCH] tracing/x86/xen: Remove zero data size trace events trace_xen_mmu_flush_tlb{_all}

[Juergen Gross]

On 09/05/18 20:46, Steven Rostedt wrote:
> 
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Doing an audit of trace events, I discovered two trace events in the xen
> subsystem that use a hack to create zero data size trace events. This is not
> what trace events are for. Trace events add memory footprint overhead, and
> if all you need to do is see if a function is hit or not, simply make that
> function noinline and use function tracer filtering.
> 
> Worse yet, the hack used was:
> 
>  __array(char, x, 0)
> 
> Which creates a static string of zero in length. There's assumptions about
> such constructs in ftrace that this is a dynamic string that is nul
> terminated. This is not the case with these tracepoints and can cause
> problems in various parts of ftrace.
> 
> Nuke the trace events!
> 
> Cc: stable@vger.kernel.org
> Fixes: 95a7d76897c1e ("xen/mmu: Use Xen specific TLB flush instead of the generic one.")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Any reason not sending this patch to the Xen maintainers?

I can take it through the Xen tree. :-)

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH] tracing/x86/xen: Remove zero data size trace events trace_xen_mmu_flush_tlb{_all}

[Steven Rostedt]

On Fri, 11 May 2018 06:14:07 +0200
Juergen Gross <jgross@suse.com> wrote:

> Any reason not sending this patch to the Xen maintainers?

Nope, I guess I should have run the patch through "get_maintainer.pl".

> 
> I can take it through the Xen tree. :-)

Thanks!


> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Note, I'm going to be adding the below patch for 4.18 (or 5.0) which
will cause the build to break in case there's any more instances of
this.

-- Steve

>From 8f8125877582345dd68f865f76442dda9204c0c1 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Thu, 10 May 2018 12:40:21 -0400
Subject: [PATCH] tracing: Prevent further users of zero size static arrays in
 trace events

A zero size static array has special meaning in the ftrace infrastructure.
Trace events are for recording data in the trace buffers that is normally
difficult to obtain via probes or function tracing. There is no reason for
any trace event to declare a zero size static array.

If one does, BUILD_BUG_ON() will trigger and prevent the kernel from
compiling.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/trace/trace_events.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index bfda803b0a09..4ecdfe2e3580 100644
--- a/include/trace/trace_events.h
+++ b/include/trace/trace_events.h
@@ -422,6 +422,7 @@ static struct trace_event_functions trace_event_type_funcs_##call = {	\
 	do {								\
 		char *type_str = #type"["__stringify(len)"]";		\
 		BUILD_BUG_ON(len > MAX_FILTER_STR_VAL);			\
+		BUILD_BUG_ON(len <= 0);					\
 		ret = trace_define_field(event_call, type_str, #item,	\
 				 offsetof(typeof(field), item),		\
 				 sizeof(field.item),			\
-- 
2.13.6

Re: [PATCH] tracing/x86/xen: Remove zero data size trace events trace_xen_mmu_flush_tlb{_all}

[Steven Rostedt]

On Fri, 11 May 2018 06:14:07 +0200
Juergen Gross <jgross@suse.com> wrote:

> > Cc: stable@vger.kernel.org
> > Fixes: 95a7d76897c1e ("xen/mmu: Use Xen specific TLB flush instead of the generic one.")
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> 
> Any reason not sending this patch to the Xen maintainers?
> 
> I can take it through the Xen tree. :-)
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Are you going to take it?

Hmm, I guess I probably should as my patch depends on it. I'll add your
reviewed-by tag and take it.

Thanks!

-- Steve