[PATCH] tracing/x86/xen: Remove zero data size trace events trace_xen_mmu_flush_tlb{_all}
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),
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),
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
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), \
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
linux-stable-mirror@lists.linaro.org
-
Juergen Gross -
Steven Rostedt