* [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
* 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 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
* [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 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 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).