linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] ftrace: Fix stack tracing issues
@ 2014-11-19  3:33 Steven Rostedt
  2014-11-19  3:33 ` [PATCH 1/2] ftrace/x86: Add frames pointers to trampoline as necessary Steven Rostedt
  2014-11-19  3:33 ` [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function Steven Rostedt
  0 siblings, 2 replies; 11+ messages in thread
From: Steven Rostedt @ 2014-11-19  3:33 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin,
	williams, Masami Hiramatsu, Namhyung Kim

I ran my ftrace tests on a PREEMPT_RT kernel and one of the tests failed.
It triggered a race that was in mainline and was fixed by another patch.
The bug was with the traceoff function trigger.

I stated testing the other triggers and discovered two other bugs.
One was caused by my latest changes, but the other one has been in
mainline for some time. It's been there since 3.16, and I haven't
tested it further. It's not that big of a bug so I'm not labeling
it with stable.

The bug that's been there happens when CONFIG_FRAME_POINTERS is set.
The ftrace trampoline doesn't set up a frame pointer, and the stack
trace code will miss the called function. That is if you do:

 echo __kmalloc:stacktrace > set_ftrace_filter

It will not show __kmalloc in the trace. This isn't that bad, but if
fentry is used (compiled with gcc 4.6 and newer on x86), then not only
is __kmalloc missed, but also the function that called __kmalloc.
This is a bit more serious, as that is useful information. The reason
for the difference with fentry, is that the fentry is called before
the stack frame is set up, so the missing bp frame pointer goes back
pass the parent.

The second bug is with the new code and dynamic ftrace trampolines.
There's a check in the stack trace recording to see if the address
on the stack is kernel code or not. This checks core kernel text as
well as module address. But it doesn't check if it is a dynamically
allocated ftrace trampoline. This is much worse than the other bug
because if FRAME_POINTERS is set, the pointer to the trampoline is
skipped and the bp frame pointer is never updated. That means, no
functions will be traced. Makes the stack trace from function tracing
rather pointless. Luckily, that code is not in mainline yet and this
fix will make sure mainline doesn't get the bug (except for bisects).

Enjoy,

-- Steve


Steven Rostedt (Red Hat) (2):
      ftrace/x86: Add frames pointers to trampoline as necessary
      ftrace/x86/extable: Add is_ftrace_trampoline() function

----
 arch/x86/kernel/ftrace.c    |  9 +++++++--
 arch/x86/kernel/mcount_64.S | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/ftrace.h      |  8 ++++++++
 kernel/extable.c            |  3 +++
 kernel/trace/ftrace.c       | 26 ++++++++++++++++++++++++++
 5 files changed, 85 insertions(+), 2 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] ftrace/x86: Add frames pointers to trampoline as necessary
  2014-11-19  3:33 [PATCH 0/2] ftrace: Fix stack tracing issues Steven Rostedt
@ 2014-11-19  3:33 ` Steven Rostedt
  2014-11-19 18:26   ` Thomas Gleixner
  2014-11-19  3:33 ` [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function Steven Rostedt
  1 sibling, 1 reply; 11+ messages in thread
From: Steven Rostedt @ 2014-11-19  3:33 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin,
	williams, Masami Hiramatsu, Namhyung Kim, Ingo Molnar, x86

[-- Attachment #1: 0001-ftrace-x86-Add-frames-pointers-to-trampoline-as-nece.patch --]
[-- Type: text/plain, Size: 2772 bytes --]

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

When CONFIG_FRAME_POINTERS are enabled, it is required that the
ftrace_caller and ftrace_regs_caller trampolines set up frame pointers
otherwise a stack trace from a function call wont print the functions
that called the trampoline. This is due to a check in
__save_stack_address():

 #ifdef CONFIG_FRAME_POINTER
	if (!reliable)
		return;
 #endif

The "reliable" variable is only set if the function address is equal to
contents of the address before the address the frame pointer register
points to. If the frame pointer is not set up for the ftrace caller
then this will fail the reliable test. It will miss the function that
called the trampoline. Worse yet, if fentry is used (gcc 4.6 and
beyond), it will also miss the parent, as the fentry is called before
the stack frame is set up. That means the bp frame pointer points
to the stack of just before the parent function was called.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/mcount_64.S | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index 42f0cdd20baf..35a793fa4bba 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -47,14 +47,51 @@ GLOBAL(\trace_label)
 #endif
 .endm
 
+#ifdef CONFIG_FRAME_POINTER
+/*
+ * Stack traces will stop at the ftrace trampoline if the frame pointer
+ * is not set up properly. If fentry is used, we need to save a frame
+ * pointer for the parent as well as the function traced, because the
+ * fentry is called before the stack frame is set up, where as mcount
+ * is called afterward.
+ */
+.macro create_frame parent rip
+#ifdef CC_USING_FENTRY
+	pushq \parent
+	pushq %rbp
+	movq %rsp, %rbp
+#endif
+	pushq \rip
+	pushq %rbp
+	movq %rsp, %rbp
+.endm
+
+.macro restore_frame
+#ifdef CC_USING_FENTRY
+	addq $16, %rsp
+#endif
+	popq %rbp
+	addq $8, %rsp
+.endm
+#else
+.macro create_frame parent rip
+.endm
+.macro restore_frame
+.endm
+#endif /* CONFIG_FRAME_POINTER */
+
 ENTRY(ftrace_caller)
 	ftrace_caller_setup ftrace_caller_op_ptr
 	/* regs go into 4th parameter (but make it NULL) */
 	movq $0, %rcx
 
+	create_frame %rsi, %rdi
+
 GLOBAL(ftrace_call)
 	call ftrace_stub
 
+	restore_frame
+
 	MCOUNT_RESTORE_FRAME
 
 	/*
@@ -105,9 +142,13 @@ ENTRY(ftrace_regs_caller)
 	/* regs go into 4th parameter */
 	leaq (%rsp), %rcx
 
+	create_frame %rsi, %rdi
+
 GLOBAL(ftrace_regs_call)
 	call ftrace_stub
 
+	restore_frame
+
 	/* Copy flags back to SS, to restore them */
 	movq EFLAGS(%rsp), %rax
 	movq %rax, SS(%rsp)
-- 
2.1.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function
  2014-11-19  3:33 [PATCH 0/2] ftrace: Fix stack tracing issues Steven Rostedt
  2014-11-19  3:33 ` [PATCH 1/2] ftrace/x86: Add frames pointers to trampoline as necessary Steven Rostedt
@ 2014-11-19  3:33 ` Steven Rostedt
  2014-11-19  4:15   ` Steven Rostedt
  2014-11-19  8:16   ` Namhyung Kim
  1 sibling, 2 replies; 11+ messages in thread
From: Steven Rostedt @ 2014-11-19  3:33 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin,
	williams, Masami Hiramatsu, Namhyung Kim, Ingo Molnar

[-- Attachment #1: 0002-ftrace-x86-extable-Add-is_ftrace_trampoline-function.patch --]
[-- Type: text/plain, Size: 5955 bytes --]

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

Stack traces that happen from function tracing check if the address
on the stack is a __kernel_text_address(). That is, is the address
kernel code. This calls core_kernel_text() which returns true
if the address is part of the builtin kernel code. It also calls
is_module_text_address() which returns true if the address belongs
to module code.

But what is missing is ftrace dynamically allocated trampolines.
These trampolines are allocated for individual ftrace_ops that
call the ftrace_ops callback functions directly. But if they do a
stack trace, the code checking the stack wont detect them as they
are neither core kernel code nor module address space.

Adding another field to ftrace_ops that also stores the size of
the trampoline assigned to it we can create a new function called
is_ftrace_trampoline() that returns true if the address is a
dynamically allocate ftrace trampoline. Note, it ignores trampolines
that are not dynamically allocated as they will return true with
the core_kernel_text() function.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c |  9 +++++++--
 include/linux/ftrace.h   |  8 ++++++++
 kernel/extable.c         |  3 +++
 kernel/trace/ftrace.c    | 26 ++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1aea94d336c7..60881d919432 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -712,7 +712,8 @@ union ftrace_op_code_union {
 	} __attribute__((packed));
 };
 
-static unsigned long create_trampoline(struct ftrace_ops *ops)
+static unsigned long
+create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 {
 	unsigned const char *jmp;
 	unsigned long start_offset;
@@ -749,6 +750,8 @@ static unsigned long create_trampoline(struct ftrace_ops *ops)
 	if (!trampoline)
 		return 0;
 
+	*tramp_size = size + MCOUNT_INSN_SIZE + sizeof(void *);
+
 	/* Copy ftrace_caller onto the trampoline memory */
 	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
 	if (WARN_ON(ret < 0)) {
@@ -819,6 +822,7 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 	unsigned char *new;
 	unsigned long offset;
 	unsigned long ip;
+	unsigned int size;
 	int ret;
 
 	if (ops->trampoline) {
@@ -829,9 +833,10 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 		if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
 			return;
 	} else {
-		ops->trampoline = create_trampoline(ops);
+		ops->trampoline = create_trampoline(ops, &size);
 		if (!ops->trampoline)
 			return;
+		ops->trampoline_size = size;
 	}
 
 	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 619e37cc17fd..7b2616fa2472 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -150,6 +150,7 @@ struct ftrace_ops {
 	struct ftrace_ops_hash		*func_hash;
 	struct ftrace_ops_hash		old_hash;
 	unsigned long			trampoline;
+	unsigned long			trampoline_size;
 #endif
 };
 
@@ -297,6 +298,8 @@ extern int ftrace_text_reserved(const void *start, const void *end);
 
 extern int ftrace_nr_registered_ops(void);
 
+bool is_ftrace_trampoline(unsigned long addr);
+
 /*
  * The dyn_ftrace record's flags field is split into two parts.
  * the first part which is '0-FTRACE_REF_MAX' is a counter of
@@ -596,6 +599,11 @@ static inline ssize_t ftrace_notrace_write(struct file *file, const char __user
 			     size_t cnt, loff_t *ppos) { return -ENODEV; }
 static inline int
 ftrace_regex_release(struct inode *inode, struct file *file) { return -ENODEV; }
+
+static inline bool is_ftrace_trampoline(unsigned long addr)
+{
+	return false;
+}
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 /* totally disable ftrace - can not re-enable after this */
diff --git a/kernel/extable.c b/kernel/extable.c
index d8a6446adbcb..f3313ee4e201 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -18,6 +18,7 @@
 #include <linux/ftrace.h>
 #include <linux/memory.h>
 #include <linux/module.h>
+#include <linux/ftrace.h>
 #include <linux/mutex.h>
 #include <linux/init.h>
 
@@ -102,6 +103,8 @@ int __kernel_text_address(unsigned long addr)
 		return 1;
 	if (is_module_text_address(addr))
 		return 1;
+	if (is_ftrace_trampoline(addr))
+		return 1;
 	/*
 	 * There might be init symbols in saved stacktraces.
 	 * Give those symbols a chance to be printed in
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 6233f9102179..e1b364df3c7f 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1117,6 +1117,31 @@ static struct ftrace_ops global_ops = {
 					  FTRACE_OPS_FL_INITIALIZED,
 };
 
+/*
+ * This is used by __kernel_text_address() to return true if the
+ * the address is on a dynamically allocated trampoline that would
+ * not return true for either core_kernel_text() or
+ * is_module_text_address().
+ */
+bool is_ftrace_trampoline(unsigned long addr)
+{
+	struct ftrace_ops *op;
+
+	do_for_each_ftrace_op(op, ftrace_ops_list) {
+		/*
+		 * This is to check for dynamically allocated trampolines.
+		 * Trampolines that are in kernel text will have
+		 * core_kernel_text() return true.
+		 */
+		if (op->trampoline && op->trampoline_size)
+			if (addr >= op->trampoline &&
+			    addr < op->trampoline + op->trampoline_size)
+				return true;
+	} while_for_each_ftrace_op(op);
+
+	return false;
+}
+
 struct ftrace_page {
 	struct ftrace_page	*next;
 	struct dyn_ftrace	*records;
@@ -5373,6 +5398,7 @@ static struct ftrace_ops graph_ops = {
 				   FTRACE_OPS_FL_STUB,
 #ifdef FTRACE_GRAPH_TRAMP_ADDR
 	.trampoline		= FTRACE_GRAPH_TRAMP_ADDR,
+	/* trampoline_size is only needed for dynamically allocated tramps */
 #endif
 	ASSIGN_OPS_HASH(graph_ops, &global_ops.local_hash)
 };
-- 
2.1.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function
  2014-11-19  3:33 ` [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function Steven Rostedt
@ 2014-11-19  4:15   ` Steven Rostedt
  2014-11-19  8:16   ` Namhyung Kim
  1 sibling, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2014-11-19  4:15 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin,
	williams, Masami Hiramatsu, Namhyung Kim, Ingo Molnar

On Tue, 18 Nov 2014 22:33:33 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:


> +/*
> + * This is used by __kernel_text_address() to return true if the
> + * the address is on a dynamically allocated trampoline that would
> + * not return true for either core_kernel_text() or
> + * is_module_text_address().
> + */
> +bool is_ftrace_trampoline(unsigned long addr)
> +{
> +	struct ftrace_ops *op;
> +
> +	do_for_each_ftrace_op(op, ftrace_ops_list) {
> +		/*
> +		 * This is to check for dynamically allocated trampolines.
> +		 * Trampolines that are in kernel text will have
> +		 * core_kernel_text() return true.
> +		 */
> +		if (op->trampoline && op->trampoline_size)
> +			if (addr >= op->trampoline &&
> +			    addr < op->trampoline + op->trampoline_size)
> +				return true;
> +	} while_for_each_ftrace_op(op);
> +
> +	return false;
> +}
> +

Hmm, preemption should be disabled here. We can't guarantee that the
caller will have that. Will update.

-- Steve

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function
  2014-11-19  3:33 ` [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function Steven Rostedt
  2014-11-19  4:15   ` Steven Rostedt
@ 2014-11-19  8:16   ` Namhyung Kim
  2014-11-19 13:36     ` Steven Rostedt
  2014-11-19 15:37     ` Steven Rostedt
  1 sibling, 2 replies; 11+ messages in thread
From: Namhyung Kim @ 2014-11-19  8:16 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, Ingo Molnar, Andrew Morton, Thomas Gleixner,
	H. Peter Anvin, williams, Masami Hiramatsu, Ingo Molnar

Hi Steve,

On Tue, 18 Nov 2014 22:33:33 -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>
> Stack traces that happen from function tracing check if the address
> on the stack is a __kernel_text_address(). That is, is the address
> kernel code. This calls core_kernel_text() which returns true
> if the address is part of the builtin kernel code. It also calls
> is_module_text_address() which returns true if the address belongs
> to module code.
>
> But what is missing is ftrace dynamically allocated trampolines.
> These trampolines are allocated for individual ftrace_ops that
> call the ftrace_ops callback functions directly. But if they do a
> stack trace, the code checking the stack wont detect them as they
> are neither core kernel code nor module address space.
>
> Adding another field to ftrace_ops that also stores the size of
> the trampoline assigned to it we can create a new function called
> is_ftrace_trampoline() that returns true if the address is a
> dynamically allocate ftrace trampoline. Note, it ignores trampolines
> that are not dynamically allocated as they will return true with
> the core_kernel_text() function.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


[SNIP]
> @@ -102,6 +103,8 @@ int __kernel_text_address(unsigned long addr)
>  		return 1;
>  	if (is_module_text_address(addr))
>  		return 1;
> +	if (is_ftrace_trampoline(addr))
> +		return 1;

What about kernel_text_address()?  It seems some archs like ARM use it
instead of __kernel_text_address() although trampoline is only enabled
on x86 for now.

Thanks,
Namhyung


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function
  2014-11-19  8:16   ` Namhyung Kim
@ 2014-11-19 13:36     ` Steven Rostedt
  2014-11-19 15:37     ` Steven Rostedt
  1 sibling, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2014-11-19 13:36 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: linux-kernel, Ingo Molnar, Andrew Morton, Thomas Gleixner,
	H. Peter Anvin, williams, Masami Hiramatsu, Ingo Molnar


> 
> [SNIP]
> > @@ -102,6 +103,8 @@ int __kernel_text_address(unsigned long addr)
> >  		return 1;
> >  	if (is_module_text_address(addr))
> >  		return 1;
> > +	if (is_ftrace_trampoline(addr))
> > +		return 1;
> 
> What about kernel_text_address()?  It seems some archs like ARM use it
> instead of __kernel_text_address() although trampoline is only enabled
> on x86 for now.
> 

Good question. I just did this to get x86 working. But as we add
trampolines to other archs, we probably should add it to that too.

-- Steve

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function
  2014-11-19  8:16   ` Namhyung Kim
  2014-11-19 13:36     ` Steven Rostedt
@ 2014-11-19 15:37     ` Steven Rostedt
  2014-11-19 18:29       ` Thomas Gleixner
  1 sibling, 1 reply; 11+ messages in thread
From: Steven Rostedt @ 2014-11-19 15:37 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: linux-kernel, Ingo Molnar, Andrew Morton, Thomas Gleixner,
	H. Peter Anvin, williams, Masami Hiramatsu, Ingo Molnar

On Wed, 19 Nov 2014 17:16:55 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> 
> [SNIP]
> > @@ -102,6 +103,8 @@ int __kernel_text_address(unsigned long addr)
> >  		return 1;
> >  	if (is_module_text_address(addr))
> >  		return 1;
> > +	if (is_ftrace_trampoline(addr))
> > +		return 1;
> 
> What about kernel_text_address()?  It seems some archs like ARM use it
> instead of __kernel_text_address() although trampoline is only enabled
> on x86 for now.

Here's the new patch:

>From 418f77ed7636c01e40627ee6609f4bd859c62c75 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Date: Tue, 18 Nov 2014 21:14:11 -0500
Subject: [PATCH] ftrace/x86/extable: Add is_ftrace_trampoline() function

Stack traces that happen from function tracing check if the address
on the stack is a __kernel_text_address(). That is, is the address
kernel code. This calls core_kernel_text() which returns true
if the address is part of the builtin kernel code. It also calls
is_module_text_address() which returns true if the address belongs
to module code.

But what is missing is ftrace dynamically allocated trampolines.
These trampolines are allocated for individual ftrace_ops that
call the ftrace_ops callback functions directly. But if they do a
stack trace, the code checking the stack wont detect them as they
are neither core kernel code nor module address space.

Adding another field to ftrace_ops that also stores the size of
the trampoline assigned to it we can create a new function called
is_ftrace_trampoline() that returns true if the address is a
dynamically allocate ftrace trampoline. Note, it ignores trampolines
that are not dynamically allocated as they will return true with
the core_kernel_text() function.

Link: http://lkml.kernel.org/r/20141119034829.497125839@goodmis.org

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c |  9 +++++++--
 include/linux/ftrace.h   |  8 ++++++++
 kernel/extable.c         |  7 ++++++-
 kernel/trace/ftrace.c    | 38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1aea94d336c7..60881d919432 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -712,7 +712,8 @@ union ftrace_op_code_union {
 	} __attribute__((packed));
 };
 
-static unsigned long create_trampoline(struct ftrace_ops *ops)
+static unsigned long
+create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 {
 	unsigned const char *jmp;
 	unsigned long start_offset;
@@ -749,6 +750,8 @@ static unsigned long create_trampoline(struct ftrace_ops *ops)
 	if (!trampoline)
 		return 0;
 
+	*tramp_size = size + MCOUNT_INSN_SIZE + sizeof(void *);
+
 	/* Copy ftrace_caller onto the trampoline memory */
 	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
 	if (WARN_ON(ret < 0)) {
@@ -819,6 +822,7 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 	unsigned char *new;
 	unsigned long offset;
 	unsigned long ip;
+	unsigned int size;
 	int ret;
 
 	if (ops->trampoline) {
@@ -829,9 +833,10 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 		if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
 			return;
 	} else {
-		ops->trampoline = create_trampoline(ops);
+		ops->trampoline = create_trampoline(ops, &size);
 		if (!ops->trampoline)
 			return;
+		ops->trampoline_size = size;
 	}
 
 	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 619e37cc17fd..7b2616fa2472 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -150,6 +150,7 @@ struct ftrace_ops {
 	struct ftrace_ops_hash		*func_hash;
 	struct ftrace_ops_hash		old_hash;
 	unsigned long			trampoline;
+	unsigned long			trampoline_size;
 #endif
 };
 
@@ -297,6 +298,8 @@ extern int ftrace_text_reserved(const void *start, const void *end);
 
 extern int ftrace_nr_registered_ops(void);
 
+bool is_ftrace_trampoline(unsigned long addr);
+
 /*
  * The dyn_ftrace record's flags field is split into two parts.
  * the first part which is '0-FTRACE_REF_MAX' is a counter of
@@ -596,6 +599,11 @@ static inline ssize_t ftrace_notrace_write(struct file *file, const char __user
 			     size_t cnt, loff_t *ppos) { return -ENODEV; }
 static inline int
 ftrace_regex_release(struct inode *inode, struct file *file) { return -ENODEV; }
+
+static inline bool is_ftrace_trampoline(unsigned long addr)
+{
+	return false;
+}
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 /* totally disable ftrace - can not re-enable after this */
diff --git a/kernel/extable.c b/kernel/extable.c
index d8a6446adbcb..c98f926277a8 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -18,6 +18,7 @@
 #include <linux/ftrace.h>
 #include <linux/memory.h>
 #include <linux/module.h>
+#include <linux/ftrace.h>
 #include <linux/mutex.h>
 #include <linux/init.h>
 
@@ -102,6 +103,8 @@ int __kernel_text_address(unsigned long addr)
 		return 1;
 	if (is_module_text_address(addr))
 		return 1;
+	if (is_ftrace_trampoline(addr))
+		return 1;
 	/*
 	 * There might be init symbols in saved stacktraces.
 	 * Give those symbols a chance to be printed in
@@ -119,7 +122,9 @@ int kernel_text_address(unsigned long addr)
 {
 	if (core_kernel_text(addr))
 		return 1;
-	return is_module_text_address(addr);
+	if (is_module_text_address(addr))
+		return 1;
+	return is_ftrace_trampoline(addr);
 }
 
 /*
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 6233f9102179..fa0f36bb32e9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1117,6 +1117,43 @@ static struct ftrace_ops global_ops = {
 					  FTRACE_OPS_FL_INITIALIZED,
 };
 
+/*
+ * This is used by __kernel_text_address() to return true if the
+ * the address is on a dynamically allocated trampoline that would
+ * not return true for either core_kernel_text() or
+ * is_module_text_address().
+ */
+bool is_ftrace_trampoline(unsigned long addr)
+{
+	struct ftrace_ops *op;
+	bool ret = false;
+
+	/*
+	 * Some of the ops may be dynamically allocated,
+	 * they are freed after a synchronize_sched().
+	 */
+	preempt_disable_notrace();
+
+	do_for_each_ftrace_op(op, ftrace_ops_list) {
+		/*
+		 * This is to check for dynamically allocated trampolines.
+		 * Trampolines that are in kernel text will have
+		 * core_kernel_text() return true.
+		 */
+		if (op->trampoline && op->trampoline_size)
+			if (addr >= op->trampoline &&
+			    addr < op->trampoline + op->trampoline_size) {
+				ret = true;
+				goto out;
+			}
+	} while_for_each_ftrace_op(op);
+
+ out:
+	preempt_enable_notrace();
+
+	return ret;
+}
+
 struct ftrace_page {
 	struct ftrace_page	*next;
 	struct dyn_ftrace	*records;
@@ -5373,6 +5410,7 @@ static struct ftrace_ops graph_ops = {
 				   FTRACE_OPS_FL_STUB,
 #ifdef FTRACE_GRAPH_TRAMP_ADDR
 	.trampoline		= FTRACE_GRAPH_TRAMP_ADDR,
+	/* trampoline_size is only needed for dynamically allocated tramps */
 #endif
 	ASSIGN_OPS_HASH(graph_ops, &global_ops.local_hash)
 };
-- 
2.1.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ftrace/x86: Add frames pointers to trampoline as necessary
  2014-11-19  3:33 ` [PATCH 1/2] ftrace/x86: Add frames pointers to trampoline as necessary Steven Rostedt
@ 2014-11-19 18:26   ` Thomas Gleixner
  2014-11-19 18:38     ` Steven Rostedt
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2014-11-19 18:26 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, Ingo Molnar, Andrew Morton, H. Peter Anvin,
	williams, Masami Hiramatsu, Namhyung Kim, Ingo Molnar, x86

On Tue, 18 Nov 2014, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> When CONFIG_FRAME_POINTERS are enabled, it is required that the
> ftrace_caller and ftrace_regs_caller trampolines set up frame pointers
> otherwise a stack trace from a function call wont print the functions
> that called the trampoline. This is due to a check in
> __save_stack_address():
> 
>  #ifdef CONFIG_FRAME_POINTER
> 	if (!reliable)
> 		return;
>  #endif
> 
> The "reliable" variable is only set if the function address is equal to
> contents of the address before the address the frame pointer register
> points to. If the frame pointer is not set up for the ftrace caller
> then this will fail the reliable test. It will miss the function that
> called the trampoline. Worse yet, if fentry is used (gcc 4.6 and
> beyond), it will also miss the parent, as the fentry is called before
> the stack frame is set up. That means the bp frame pointer points
> to the stack of just before the parent function was called.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Shouldn't this be tagged stable?

Acked-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  arch/x86/kernel/mcount_64.S | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
> index 42f0cdd20baf..35a793fa4bba 100644
> --- a/arch/x86/kernel/mcount_64.S
> +++ b/arch/x86/kernel/mcount_64.S
> @@ -47,14 +47,51 @@ GLOBAL(\trace_label)
>  #endif
>  .endm
>  
> +#ifdef CONFIG_FRAME_POINTER
> +/*
> + * Stack traces will stop at the ftrace trampoline if the frame pointer
> + * is not set up properly. If fentry is used, we need to save a frame
> + * pointer for the parent as well as the function traced, because the
> + * fentry is called before the stack frame is set up, where as mcount
> + * is called afterward.
> + */
> +.macro create_frame parent rip
> +#ifdef CC_USING_FENTRY
> +	pushq \parent
> +	pushq %rbp
> +	movq %rsp, %rbp
> +#endif
> +	pushq \rip
> +	pushq %rbp
> +	movq %rsp, %rbp
> +.endm
> +
> +.macro restore_frame
> +#ifdef CC_USING_FENTRY
> +	addq $16, %rsp
> +#endif
> +	popq %rbp
> +	addq $8, %rsp
> +.endm
> +#else
> +.macro create_frame parent rip
> +.endm
> +.macro restore_frame
> +.endm
> +#endif /* CONFIG_FRAME_POINTER */
> +
>  ENTRY(ftrace_caller)
>  	ftrace_caller_setup ftrace_caller_op_ptr
>  	/* regs go into 4th parameter (but make it NULL) */
>  	movq $0, %rcx
>  
> +	create_frame %rsi, %rdi
> +
>  GLOBAL(ftrace_call)
>  	call ftrace_stub
>  
> +	restore_frame
> +
>  	MCOUNT_RESTORE_FRAME
>  
>  	/*
> @@ -105,9 +142,13 @@ ENTRY(ftrace_regs_caller)
>  	/* regs go into 4th parameter */
>  	leaq (%rsp), %rcx
>  
> +	create_frame %rsi, %rdi
> +
>  GLOBAL(ftrace_regs_call)
>  	call ftrace_stub
>  
> +	restore_frame
> +
>  	/* Copy flags back to SS, to restore them */
>  	movq EFLAGS(%rsp), %rax
>  	movq %rax, SS(%rsp)
> -- 
> 2.1.1
> 
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function
  2014-11-19 15:37     ` Steven Rostedt
@ 2014-11-19 18:29       ` Thomas Gleixner
  2014-11-19 18:39         ` Steven Rostedt
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2014-11-19 18:29 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Namhyung Kim, linux-kernel, Ingo Molnar, Andrew Morton,
	H. Peter Anvin, williams, Masami Hiramatsu, Ingo Molnar

On Wed, 19 Nov 2014, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> Date: Tue, 18 Nov 2014 21:14:11 -0500
> Subject: [PATCH] ftrace/x86/extable: Add is_ftrace_trampoline() function
> 
> Stack traces that happen from function tracing check if the address
> on the stack is a __kernel_text_address(). That is, is the address
> kernel code. This calls core_kernel_text() which returns true
> if the address is part of the builtin kernel code. It also calls
> is_module_text_address() which returns true if the address belongs
> to module code.
> 
> But what is missing is ftrace dynamically allocated trampolines.
> These trampolines are allocated for individual ftrace_ops that
> call the ftrace_ops callback functions directly. But if they do a
> stack trace, the code checking the stack wont detect them as they
> are neither core kernel code nor module address space.
> 
> Adding another field to ftrace_ops that also stores the size of
> the trampoline assigned to it we can create a new function called
> is_ftrace_trampoline() that returns true if the address is a
> dynamically allocate ftrace trampoline. Note, it ignores trampolines
> that are not dynamically allocated as they will return true with
> the core_kernel_text() function.
> 
> Link: http://lkml.kernel.org/r/20141119034829.497125839@goodmis.org
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ftrace/x86: Add frames pointers to trampoline as necessary
  2014-11-19 18:26   ` Thomas Gleixner
@ 2014-11-19 18:38     ` Steven Rostedt
  0 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2014-11-19 18:38 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, Ingo Molnar, Andrew Morton, H. Peter Anvin,
	williams, Masami Hiramatsu, Namhyung Kim, Ingo Molnar, x86

On Wed, 19 Nov 2014 19:26:48 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 18 Nov 2014, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > When CONFIG_FRAME_POINTERS are enabled, it is required that the
> > ftrace_caller and ftrace_regs_caller trampolines set up frame pointers
> > otherwise a stack trace from a function call wont print the functions
> > that called the trampoline. This is due to a check in
> > __save_stack_address():
> > 
> >  #ifdef CONFIG_FRAME_POINTER
> > 	if (!reliable)
> > 		return;
> >  #endif
> > 
> > The "reliable" variable is only set if the function address is equal to
> > contents of the address before the address the frame pointer register
> > points to. If the frame pointer is not set up for the ftrace caller
> > then this will fail the reliable test. It will miss the function that
> > called the trampoline. Worse yet, if fentry is used (gcc 4.6 and
> > beyond), it will also miss the parent, as the fentry is called before
> > the stack frame is set up. That means the bp frame pointer points
> > to the stack of just before the parent function was called.
> > 
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: x86@kernel.org
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Shouldn't this be tagged stable?

>From the cover letter:

"I stated testing the other triggers and discovered two other bugs.
One was caused by my latest changes, but the other one has been in
mainline for some time. It's been there since 3.16, and I haven't
tested it further. It's not that big of a bug so I'm not labeling
it with stable."

I guess I can tag it. I have to see how far back it goes. My configs
for the kernels I use this with didn't have FRAME_POINTER enabled, so I
never noticed. I noticed it with my test configs.

> 
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> 

Thanks!

-- Steve

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function
  2014-11-19 18:29       ` Thomas Gleixner
@ 2014-11-19 18:39         ` Steven Rostedt
  0 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2014-11-19 18:39 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Namhyung Kim, linux-kernel, Ingo Molnar, Andrew Morton,
	H. Peter Anvin, williams, Masami Hiramatsu, Ingo Molnar

On Wed, 19 Nov 2014 19:29:25 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 19 Nov 2014, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > Date: Tue, 18 Nov 2014 21:14:11 -0500
> > Subject: [PATCH] ftrace/x86/extable: Add is_ftrace_trampoline() function
> > 
> > Stack traces that happen from function tracing check if the address
> > on the stack is a __kernel_text_address(). That is, is the address
> > kernel code. This calls core_kernel_text() which returns true
> > if the address is part of the builtin kernel code. It also calls
> > is_module_text_address() which returns true if the address belongs
> > to module code.
> > 
> > But what is missing is ftrace dynamically allocated trampolines.
> > These trampolines are allocated for individual ftrace_ops that
> > call the ftrace_ops callback functions directly. But if they do a
> > stack trace, the code checking the stack wont detect them as they
> > are neither core kernel code nor module address space.
> > 
> > Adding another field to ftrace_ops that also stores the size of
> > the trampoline assigned to it we can create a new function called
> > is_ftrace_trampoline() that returns true if the address is a
> > dynamically allocate ftrace trampoline. Note, it ignores trampolines
> > that are not dynamically allocated as they will return true with
> > the core_kernel_text() function.
> > 
> > Link: http://lkml.kernel.org/r/20141119034829.497125839@goodmis.org
> > 
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Acked-by: Thomas Gleixner <tglx@linutronix.de>

Thanks!

-- Steve

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-11-19 18:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-19  3:33 [PATCH 0/2] ftrace: Fix stack tracing issues Steven Rostedt
2014-11-19  3:33 ` [PATCH 1/2] ftrace/x86: Add frames pointers to trampoline as necessary Steven Rostedt
2014-11-19 18:26   ` Thomas Gleixner
2014-11-19 18:38     ` Steven Rostedt
2014-11-19  3:33 ` [PATCH 2/2] ftrace/x86/extable: Add is_ftrace_trampoline() function Steven Rostedt
2014-11-19  4:15   ` Steven Rostedt
2014-11-19  8:16   ` Namhyung Kim
2014-11-19 13:36     ` Steven Rostedt
2014-11-19 15:37     ` Steven Rostedt
2014-11-19 18:29       ` Thomas Gleixner
2014-11-19 18:39         ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).