* [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly @ 2017-12-22 18:18 Steven Rostedt 2017-12-22 18:30 ` Josh Poimboeuf 0 siblings, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2017-12-22 18:18 UTC (permalink / raw) To: LKML Cc: Josh Poimboeuf, Ingo Molnar, Thomas Gleixner, Andrew Morton, x86, Linus Torvalds From: Steven Rostedt (VMware) <rostedt@goodmis.org> While doing some debugging, I kicked off the stacktrace trigger, to see what was doing stacktraces in my code (it was causing a lot of noise in my function traces). I ran: # trace-cmd record -p function -l save_stack_trace -l save_stack_trace:stacktrace And took a look at the trace, but all I found was this: trace-cm-1326 0d..2 107.791049: kernel_stack: <stack trace> trace-cm-1326 0d..2 107.791077: kernel_stack: <stack trace> trace-cm-1326 0...2 107.791084: kernel_stack: <stack trace> trace-cm-1326 0d..2 107.791091: kernel_stack: <stack trace> trace-cm-1326 0d..2 107.791097: kernel_stack: <stack trace> trace-cm-1326 0...2 107.791102: kernel_stack: <stack trace> trace-cm-1326 0d..4 107.791109: kernel_stack: <stack trace> trace-cm-1326 0d..2 107.791116: kernel_stack: <stack trace> trace-cm-1326 0d..2 107.791120: kernel_stack: <stack trace> trace-cm-1326 0...2 107.791125: kernel_stack: <stack trace> trace-cm-1326 0d..2 107.791133: kernel_stack: <stack trace> trace-cm-1326 0d..2 107.791140: kernel_stack: <stack trace> trace-cm-1326 0...2 107.791145: kernel_stack: <stack trace> trace-cm-1326 0d..4 107.791157: kernel_stack: <stack trace> [...] This was not very useful. I kicked off a bisect to see where it broke, because it was working fine in 4.14, and this is a new regression in the 4.15 rc cycle. The culprit ended up being: fc72ae40e30 "x86/unwind: Make CONFIG_UNWINDER_ORC=y the default in kconfig for 64-bit" As I find this to be a useful option, I don't want to revert it, so I looked at how to make ftrace work with this new unwinder. I came up with this solution. I'm not sure it is correct, but now my traces look like this (and objtool check doesn't complain): splice-t-1301 0d..1 203.199356: function: save_stack_trace splice-t-1301 0d..2 203.199365: kernel_stack: <stack trace> => ___slab_alloc (ffffffff81221c5d) => __slab_alloc.isra.81 (ffffffff81221d08) => kmem_cache_alloc (ffffffff812227ce) => getname_flags (ffffffff8124d49a) => SyS_execve (ffffffff812459f2) => do_syscall_64 (ffffffff8100394c) => return_from_SYSCALL_64 (ffffffff817c2dd8) splice-t-1301 0d..1 203.199379: function: save_stack_trace splice-t-1301 0d..2 203.199382: kernel_stack: <stack trace> => ___slab_alloc (ffffffff81221c5d) => __slab_alloc.isra.81 (ffffffff81221d08) => kmem_cache_alloc (ffffffff812227ce) => create_object (ffffffff81237593) => kmem_cache_alloc (ffffffff81222795) => getname_flags (ffffffff8124d49a) => SyS_execve (ffffffff812459f2) => do_syscall_64 (ffffffff8100394c) => return_from_SYSCALL_64 (ffffffff817c2dd8) splice-t-1301 0...1 203.199385: function: save_stack_trace splice-t-1301 0...2 203.199387: kernel_stack: <stack trace> => getname_flags (ffffffff8124d49a) => SyS_execve (ffffffff812459f2) => do_syscall_64 (ffffffff8100394c) => return_from_SYSCALL_64 (ffffffff817c2dd8) splice-t-1301 0d..1 203.199392: function: save_stack_trace splice-t-1301 0d..2 203.199395: kernel_stack: <stack trace> => ___slab_alloc (ffffffff81221c5d) => __slab_alloc.isra.81 (ffffffff81221d08) => kmem_cache_alloc_trace (ffffffff81222568) => do_execveat_common.isra.34 (ffffffff81245171) => SyS_execve (ffffffff81245a0c) => do_syscall_64 (ffffffff8100394c) => return_from_SYSCALL_64 (ffffffff817c2dd8) P.S. the culprit was SLAB_DEBUG that was doing the stack traces Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 81bb565f4497..5d9eb722a04e 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -29,7 +29,6 @@ KASAN_SANITIZE_stacktrace.o := n KASAN_SANITIZE_paravirt.o := n OBJECT_FILES_NON_STANDARD_relocate_kernel_$(BITS).o := y -OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o := y OBJECT_FILES_NON_STANDARD_test_nx.o := y OBJECT_FILES_NON_STANDARD_paravirt_patch_$(BITS).o := y diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index c832291d948a..6ab3308d632a 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -4,6 +4,7 @@ */ #include <linux/linkage.h> +#include <asm/unwind_hints.h> #include <asm/ptrace.h> #include <asm/ftrace.h> #include <asm/export.h> @@ -148,9 +149,10 @@ EXPORT_SYMBOL(mcount) ENTRY(function_hook) retq -END(function_hook) +ENDPROC(function_hook) ENTRY(ftrace_caller) + UNWIND_HINT_FUNC /* save_mcount_regs fills in first two parameters */ save_mcount_regs @@ -165,6 +167,7 @@ GLOBAL(ftrace_call) call ftrace_stub restore_mcount_regs + UNWIND_HINT_FUNC /* * The copied trampoline must call ftrace_epilogue as it @@ -187,11 +190,13 @@ WEAK(ftrace_stub) END(ftrace_caller) ENTRY(ftrace_regs_caller) + UNWIND_HINT_FUNC /* Save the current flags before any operations that can change them */ pushfq /* added 8 bytes to save flags */ save_mcount_regs 8 + UNWIND_HINT_REGS /* save_mcount_regs fills in first two parameters */ GLOBAL(ftrace_regs_caller_op_ptr) @@ -241,6 +246,7 @@ GLOBAL(ftrace_regs_call) movq RBX(%rsp), %rbx restore_mcount_regs + UNWIND_HINT_FUNC /* Restore flags */ popfq @@ -296,6 +302,7 @@ END(function_hook) #ifdef CONFIG_FUNCTION_GRAPH_TRACER ENTRY(ftrace_graph_caller) + UNWIND_HINT_FUNC /* Saves rbp into %rdx and fills first parameter */ save_mcount_regs @@ -311,11 +318,13 @@ ENTRY(ftrace_graph_caller) call prepare_ftrace_return restore_mcount_regs + UNWIND_HINT_FUNC retq END(ftrace_graph_caller) GLOBAL(return_to_handler) + UNWIND_HINT_EMPTY subq $24, %rsp /* Save the return values */ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly 2017-12-22 18:18 [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly Steven Rostedt @ 2017-12-22 18:30 ` Josh Poimboeuf 2017-12-22 18:38 ` Steven Rostedt 0 siblings, 1 reply; 6+ messages in thread From: Josh Poimboeuf @ 2017-12-22 18:30 UTC (permalink / raw) To: Steven Rostedt Cc: LKML, Ingo Molnar, Thomas Gleixner, Andrew Morton, x86, Linus Torvalds On Fri, Dec 22, 2017 at 01:18:41PM -0500, Steven Rostedt wrote: > From: Steven Rostedt (VMware) <rostedt@goodmis.org> > > While doing some debugging, I kicked off the stacktrace trigger, to see > what was doing stacktraces in my code (it was causing a lot of noise in > my function traces). > > I ran: > > # trace-cmd record -p function -l save_stack_trace -l save_stack_trace:stacktrace > > And took a look at the trace, but all I found was this: > > trace-cm-1326 0d..2 107.791049: kernel_stack: <stack trace> > trace-cm-1326 0d..2 107.791077: kernel_stack: <stack trace> > trace-cm-1326 0...2 107.791084: kernel_stack: <stack trace> > trace-cm-1326 0d..2 107.791091: kernel_stack: <stack trace> > trace-cm-1326 0d..2 107.791097: kernel_stack: <stack trace> > trace-cm-1326 0...2 107.791102: kernel_stack: <stack trace> > trace-cm-1326 0d..4 107.791109: kernel_stack: <stack trace> > trace-cm-1326 0d..2 107.791116: kernel_stack: <stack trace> > trace-cm-1326 0d..2 107.791120: kernel_stack: <stack trace> > trace-cm-1326 0...2 107.791125: kernel_stack: <stack trace> > trace-cm-1326 0d..2 107.791133: kernel_stack: <stack trace> > trace-cm-1326 0d..2 107.791140: kernel_stack: <stack trace> > trace-cm-1326 0...2 107.791145: kernel_stack: <stack trace> > trace-cm-1326 0d..4 107.791157: kernel_stack: <stack trace> > [...] > > This was not very useful. I kicked off a bisect to see where it broke, > because it was working fine in 4.14, and this is a new regression in > the 4.15 rc cycle. > > The culprit ended up being: > > fc72ae40e30 "x86/unwind: Make CONFIG_UNWINDER_ORC=y the default in > kconfig for 64-bit" > > As I find this to be a useful option, I don't want to revert it, so I > looked at how to make ftrace work with this new unwinder. I came up > with this solution. I'm not sure it is correct, but now my traces look > like this (and objtool check doesn't complain): > > > splice-t-1301 0d..1 203.199356: function: save_stack_trace > splice-t-1301 0d..2 203.199365: kernel_stack: <stack trace> > => ___slab_alloc (ffffffff81221c5d) > => __slab_alloc.isra.81 (ffffffff81221d08) > => kmem_cache_alloc (ffffffff812227ce) > => getname_flags (ffffffff8124d49a) > => SyS_execve (ffffffff812459f2) > => do_syscall_64 (ffffffff8100394c) > => return_from_SYSCALL_64 (ffffffff817c2dd8) > splice-t-1301 0d..1 203.199379: function: save_stack_trace > splice-t-1301 0d..2 203.199382: kernel_stack: <stack trace> > => ___slab_alloc (ffffffff81221c5d) > => __slab_alloc.isra.81 (ffffffff81221d08) > => kmem_cache_alloc (ffffffff812227ce) > => create_object (ffffffff81237593) > => kmem_cache_alloc (ffffffff81222795) > => getname_flags (ffffffff8124d49a) > => SyS_execve (ffffffff812459f2) > => do_syscall_64 (ffffffff8100394c) > => return_from_SYSCALL_64 (ffffffff817c2dd8) > splice-t-1301 0...1 203.199385: function: save_stack_trace > splice-t-1301 0...2 203.199387: kernel_stack: <stack trace> > => getname_flags (ffffffff8124d49a) > => SyS_execve (ffffffff812459f2) > => do_syscall_64 (ffffffff8100394c) > => return_from_SYSCALL_64 (ffffffff817c2dd8) > splice-t-1301 0d..1 203.199392: function: save_stack_trace > splice-t-1301 0d..2 203.199395: kernel_stack: <stack trace> > => ___slab_alloc (ffffffff81221c5d) > => __slab_alloc.isra.81 (ffffffff81221d08) > => kmem_cache_alloc_trace (ffffffff81222568) > => do_execveat_common.isra.34 (ffffffff81245171) > => SyS_execve (ffffffff81245a0c) > => do_syscall_64 (ffffffff8100394c) > => return_from_SYSCALL_64 (ffffffff817c2dd8) > > P.S. the culprit was SLAB_DEBUG that was doing the stack traces > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index 81bb565f4497..5d9eb722a04e 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -29,7 +29,6 @@ KASAN_SANITIZE_stacktrace.o := n > KASAN_SANITIZE_paravirt.o := n > > OBJECT_FILES_NON_STANDARD_relocate_kernel_$(BITS).o := y > -OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o := y > OBJECT_FILES_NON_STANDARD_test_nx.o := y > OBJECT_FILES_NON_STANDARD_paravirt_patch_$(BITS).o := y > > diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S > index c832291d948a..6ab3308d632a 100644 > --- a/arch/x86/kernel/ftrace_64.S > +++ b/arch/x86/kernel/ftrace_64.S > @@ -4,6 +4,7 @@ > */ > > #include <linux/linkage.h> > +#include <asm/unwind_hints.h> > #include <asm/ptrace.h> > #include <asm/ftrace.h> > #include <asm/export.h> > @@ -148,9 +149,10 @@ EXPORT_SYMBOL(mcount) > > ENTRY(function_hook) > retq > -END(function_hook) > +ENDPROC(function_hook) > > ENTRY(ftrace_caller) > + UNWIND_HINT_FUNC > /* save_mcount_regs fills in first two parameters */ > save_mcount_regs > > @@ -165,6 +167,7 @@ GLOBAL(ftrace_call) > call ftrace_stub > > restore_mcount_regs > + UNWIND_HINT_FUNC > > /* > * The copied trampoline must call ftrace_epilogue as it > @@ -187,11 +190,13 @@ WEAK(ftrace_stub) > END(ftrace_caller) > > ENTRY(ftrace_regs_caller) > + UNWIND_HINT_FUNC > /* Save the current flags before any operations that can change them */ > pushfq > > /* added 8 bytes to save flags */ > save_mcount_regs 8 > + UNWIND_HINT_REGS > /* save_mcount_regs fills in first two parameters */ > > GLOBAL(ftrace_regs_caller_op_ptr) > @@ -241,6 +246,7 @@ GLOBAL(ftrace_regs_call) > movq RBX(%rsp), %rbx > > restore_mcount_regs > + UNWIND_HINT_FUNC > > /* Restore flags */ > popfq > @@ -296,6 +302,7 @@ END(function_hook) > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > ENTRY(ftrace_graph_caller) > + UNWIND_HINT_FUNC > /* Saves rbp into %rdx and fills first parameter */ > save_mcount_regs > > @@ -311,11 +318,13 @@ ENTRY(ftrace_graph_caller) > call prepare_ftrace_return > > restore_mcount_regs > + UNWIND_HINT_FUNC > > retq > END(ftrace_graph_caller) > > GLOBAL(return_to_handler) > + UNWIND_HINT_EMPTY > subq $24, %rsp > > /* Save the return values */ I'm officially on vacation but I got a chance to look at this myself a few minutes ago. This looks mostly right. In theory, you should able to mark those as functions by changing END to ENDPROC. Then you won't need all the UNWIND_HINT_FUNCs. I tried making that change, but objtool thinks the stack frame is still modified when the functions return. I didn't see anything obvious in save_mcount_regs or restore_mcount_regs that should cause it to think that. I'll need to look into it a little more. -- Josh ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly 2017-12-22 18:30 ` Josh Poimboeuf @ 2017-12-22 18:38 ` Steven Rostedt 2017-12-23 2:43 ` Josh Poimboeuf 0 siblings, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2017-12-22 18:38 UTC (permalink / raw) To: Josh Poimboeuf Cc: LKML, Ingo Molnar, Thomas Gleixner, Andrew Morton, x86, Linus Torvalds On Fri, 22 Dec 2017 12:30:24 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > I'm officially on vacation but I got a chance to look at this myself a > few minutes ago. This looks mostly right. In theory, you should able > to mark those as functions by changing END to ENDPROC. Then you won't > need all the UNWIND_HINT_FUNCs. Yep, that was my first solution, but as you found, objtool complained about it. > > I tried making that change, but objtool thinks the stack frame is still > modified when the functions return. I didn't see anything obvious in > save_mcount_regs or restore_mcount_regs that should cause it to think > that. I'll need to look into it a little more. Thanks! Worse comes to worse, we can keep this change, and you can enjoy your holidays. I just need this fixed before 4.15 is released. I'll be jumping into objtool shortly, to see if I can merge the code from recordmcount.c with it too. I'm going to need to learn ORC and DWARF to start on extending the function tracer to act more like tracepoints (like I discussed with Linus in Prague). -- Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly 2017-12-22 18:38 ` Steven Rostedt @ 2017-12-23 2:43 ` Josh Poimboeuf 2017-12-23 5:51 ` Steven Rostedt 2018-01-23 4:07 ` [PATCH] x86/ftrace: Fix ORC unwinding from ftrace handlers Josh Poimboeuf 0 siblings, 2 replies; 6+ messages in thread From: Josh Poimboeuf @ 2017-12-23 2:43 UTC (permalink / raw) To: Steven Rostedt Cc: LKML, Ingo Molnar, Thomas Gleixner, Andrew Morton, x86, Linus Torvalds On Fri, Dec 22, 2017 at 01:38:01PM -0500, Steven Rostedt wrote: > On Fri, 22 Dec 2017 12:30:24 -0600 > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > I'm officially on vacation but I got a chance to look at this myself a > > few minutes ago. This looks mostly right. In theory, you should able > > to mark those as functions by changing END to ENDPROC. Then you won't > > need all the UNWIND_HINT_FUNCs. > > Yep, that was my first solution, but as you found, objtool complained > about it. > > > > > I tried making that change, but objtool thinks the stack frame is still > > modified when the functions return. I didn't see anything obvious in > > save_mcount_regs or restore_mcount_regs that should cause it to think > > that. I'll need to look into it a little more. > > Thanks! > > Worse comes to worse, we can keep this change, and you can enjoy your > holidays. I just need this fixed before 4.15 is released. > > I'll be jumping into objtool shortly, to see if I can merge the code > from recordmcount.c with it too. I'm going to need to learn ORC and > DWARF to start on extending the function tracer to act more like > tracepoints (like I discussed with Linus in Prague). Objtool doesn't like the fact that save_mcount_regs pushes rbp at the beginning, but it's never restored (directly, at least). How about something like this instead, where it skips the original rbp push? I didn't test it, but I think it should work. It at least gets rid of the warnings and doesn't need any unwind hints other than the UNWIND_HINT_EMPTY in return_to_handler. diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 81bb565f4497..7e2baf7304ae 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -29,10 +29,13 @@ KASAN_SANITIZE_stacktrace.o := n KASAN_SANITIZE_paravirt.o := n OBJECT_FILES_NON_STANDARD_relocate_kernel_$(BITS).o := y -OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o := y OBJECT_FILES_NON_STANDARD_test_nx.o := y OBJECT_FILES_NON_STANDARD_paravirt_patch_$(BITS).o := y +ifdef CONFIG_FRAME_POINTER +OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o := y +endif + # If instrumentation of this dir is enabled, boot hangs during first second. # Probably could be more selective here, but note that files related to irqs, # boot, dumpstack/stacktrace, etc are either non-interesting or can lead to diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index c832291d948a..38b079626018 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -7,6 +7,7 @@ #include <asm/ptrace.h> #include <asm/ftrace.h> #include <asm/export.h> +#include <asm/unwind_hints.h> .code64 @@ -20,7 +21,6 @@ EXPORT_SYMBOL(__fentry__) EXPORT_SYMBOL(mcount) #endif -/* All cases save the original rbp (8 bytes) */ #ifdef CONFIG_FRAME_POINTER # ifdef CC_USING_FENTRY /* Save parent and function stack frames (rip and rbp) */ @@ -31,7 +31,7 @@ EXPORT_SYMBOL(mcount) # endif #else /* No need to save a stack frame */ -# define MCOUNT_FRAME_SIZE 8 +# define MCOUNT_FRAME_SIZE 0 #endif /* CONFIG_FRAME_POINTER */ /* Size of stack used to save mcount regs in save_mcount_regs */ @@ -64,10 +64,10 @@ EXPORT_SYMBOL(mcount) */ .macro save_mcount_regs added=0 - /* Always save the original rbp */ +#ifdef CONFIG_FRAME_POINTER + /* Save the original rbp */ pushq %rbp -#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 @@ -105,7 +105,11 @@ EXPORT_SYMBOL(mcount) * Save the original RBP. Even though the mcount ABI does not * require this, it helps out callers. */ +#ifdef CONFIG_FRAME_POINTER movq MCOUNT_REG_SIZE-8(%rsp), %rdx +#else + movq %rbp, %rdx +#endif movq %rdx, RBP(%rsp) /* Copy the parent address into %rsi (second parameter) */ @@ -148,7 +152,7 @@ EXPORT_SYMBOL(mcount) ENTRY(function_hook) retq -END(function_hook) +ENDPROC(function_hook) ENTRY(ftrace_caller) /* save_mcount_regs fills in first two parameters */ @@ -184,7 +188,7 @@ GLOBAL(ftrace_graph_call) /* This is weak to keep gas from relaxing the jumps */ WEAK(ftrace_stub) retq -END(ftrace_caller) +ENDPROC(ftrace_caller) ENTRY(ftrace_regs_caller) /* Save the current flags before any operations that can change them */ @@ -255,7 +259,7 @@ GLOBAL(ftrace_regs_caller_end) jmp ftrace_epilogue -END(ftrace_regs_caller) +ENDPROC(ftrace_regs_caller) #else /* ! CONFIG_DYNAMIC_FTRACE */ @@ -313,9 +317,10 @@ ENTRY(ftrace_graph_caller) restore_mcount_regs retq -END(ftrace_graph_caller) +ENDPROC(ftrace_graph_caller) -GLOBAL(return_to_handler) +ENTRY(return_to_handler) + UNWIND_HINT_EMPTY subq $24, %rsp /* Save the return values */ @@ -330,4 +335,5 @@ GLOBAL(return_to_handler) movq (%rsp), %rax addq $24, %rsp jmp *%rdi +END(return_to_handler) #endif ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly 2017-12-23 2:43 ` Josh Poimboeuf @ 2017-12-23 5:51 ` Steven Rostedt 2018-01-23 4:07 ` [PATCH] x86/ftrace: Fix ORC unwinding from ftrace handlers Josh Poimboeuf 1 sibling, 0 replies; 6+ messages in thread From: Steven Rostedt @ 2017-12-23 5:51 UTC (permalink / raw) To: Josh Poimboeuf Cc: LKML, Ingo Molnar, Thomas Gleixner, Andrew Morton, x86, Linus Torvalds On Fri, 22 Dec 2017 20:43:37 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > Objtool doesn't like the fact that save_mcount_regs pushes rbp at the > beginning, but it's never restored (directly, at least). > > How about something like this instead, where it skips the original rbp > push? I didn't test it, but I think it should work. It at least gets > rid of the warnings and doesn't need any unwind hints other than the > UNWIND_HINT_EMPTY in return_to_handler. There's a reason I did it that way, but don't remember exactly why. I think we discussed this a while ago when we had issues with ftrace.S during your first iterations of objtool. I'll have to test all different compile options and different gcc compiler versions. -- Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] x86/ftrace: Fix ORC unwinding from ftrace handlers 2017-12-23 2:43 ` Josh Poimboeuf 2017-12-23 5:51 ` Steven Rostedt @ 2018-01-23 4:07 ` Josh Poimboeuf 1 sibling, 0 replies; 6+ messages in thread From: Josh Poimboeuf @ 2018-01-23 4:07 UTC (permalink / raw) To: Steven Rostedt Cc: LKML, Ingo Molnar, Thomas Gleixner, Andrew Morton, x86, Linus Torvalds Steven Rostedt discovered that the ftrace stack tracer is broken when it's used with the ORC unwinder. The problem is that objtool is instructed by the Makefile to ignore the ftrace_64.S code, so it doesn't generate any ORC data for it. Fix it by making the asm code objtool-friendly: - Objtool doesn't like the fact that save_mcount_regs pushes RBP at the beginning, but it's never restored (directly, at least). So just skip the original RBP push, which is only needed for frame pointers anyway. - Annotate some functions as normal callable functions with ENTRY/ENDPROC. - Add an empty unwind hint to return_to_handler(). The return address isn't on the stack, so there's nothing ORC can do there. It will just punt in the unlikely case it tries to unwind from that code. With all that fixed, remove the OBJECT_FILES_NON_STANDARD Makefile annotation so objtool can read the file. Reported-by: Steven Rostedt <rostedt@goodmis.org> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- arch/x86/kernel/Makefile | 5 ++++- arch/x86/kernel/ftrace_64.S | 24 +++++++++++++++--------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index aed9296dccd3..29786c87e864 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -29,10 +29,13 @@ KASAN_SANITIZE_stacktrace.o := n KASAN_SANITIZE_paravirt.o := n OBJECT_FILES_NON_STANDARD_relocate_kernel_$(BITS).o := y -OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o := y OBJECT_FILES_NON_STANDARD_test_nx.o := y OBJECT_FILES_NON_STANDARD_paravirt_patch_$(BITS).o := y +ifdef CONFIG_FRAME_POINTER +OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o := y +endif + # If instrumentation of this dir is enabled, boot hangs during first second. # Probably could be more selective here, but note that files related to irqs, # boot, dumpstack/stacktrace, etc are either non-interesting or can lead to diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index 7cb8ba08beb9..ef61f540cf0a 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -8,6 +8,7 @@ #include <asm/ftrace.h> #include <asm/export.h> #include <asm/nospec-branch.h> +#include <asm/unwind_hints.h> .code64 .section .entry.text, "ax" @@ -20,7 +21,6 @@ EXPORT_SYMBOL(__fentry__) EXPORT_SYMBOL(mcount) #endif -/* All cases save the original rbp (8 bytes) */ #ifdef CONFIG_FRAME_POINTER # ifdef CC_USING_FENTRY /* Save parent and function stack frames (rip and rbp) */ @@ -31,7 +31,7 @@ EXPORT_SYMBOL(mcount) # endif #else /* No need to save a stack frame */ -# define MCOUNT_FRAME_SIZE 8 +# define MCOUNT_FRAME_SIZE 0 #endif /* CONFIG_FRAME_POINTER */ /* Size of stack used to save mcount regs in save_mcount_regs */ @@ -64,10 +64,10 @@ EXPORT_SYMBOL(mcount) */ .macro save_mcount_regs added=0 - /* Always save the original rbp */ +#ifdef CONFIG_FRAME_POINTER + /* Save the original rbp */ pushq %rbp -#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 @@ -105,7 +105,11 @@ EXPORT_SYMBOL(mcount) * Save the original RBP. Even though the mcount ABI does not * require this, it helps out callers. */ +#ifdef CONFIG_FRAME_POINTER movq MCOUNT_REG_SIZE-8(%rsp), %rdx +#else + movq %rbp, %rdx +#endif movq %rdx, RBP(%rsp) /* Copy the parent address into %rsi (second parameter) */ @@ -148,7 +152,7 @@ EXPORT_SYMBOL(mcount) ENTRY(function_hook) retq -END(function_hook) +ENDPROC(function_hook) ENTRY(ftrace_caller) /* save_mcount_regs fills in first two parameters */ @@ -184,7 +188,7 @@ GLOBAL(ftrace_graph_call) /* This is weak to keep gas from relaxing the jumps */ WEAK(ftrace_stub) retq -END(ftrace_caller) +ENDPROC(ftrace_caller) ENTRY(ftrace_regs_caller) /* Save the current flags before any operations that can change them */ @@ -255,7 +259,7 @@ GLOBAL(ftrace_regs_caller_end) jmp ftrace_epilogue -END(ftrace_regs_caller) +ENDPROC(ftrace_regs_caller) #else /* ! CONFIG_DYNAMIC_FTRACE */ @@ -313,9 +317,10 @@ ENTRY(ftrace_graph_caller) restore_mcount_regs retq -END(ftrace_graph_caller) +ENDPROC(ftrace_graph_caller) -GLOBAL(return_to_handler) +ENTRY(return_to_handler) + UNWIND_HINT_EMPTY subq $24, %rsp /* Save the return values */ @@ -330,4 +335,5 @@ GLOBAL(return_to_handler) movq (%rsp), %rax addq $24, %rsp JMP_NOSPEC %rdi +END(return_to_handler) #endif -- 2.14.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-23 4:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-22 18:18 [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly Steven Rostedt 2017-12-22 18:30 ` Josh Poimboeuf 2017-12-22 18:38 ` Steven Rostedt 2017-12-23 2:43 ` Josh Poimboeuf 2017-12-23 5:51 ` Steven Rostedt 2018-01-23 4:07 ` [PATCH] x86/ftrace: Fix ORC unwinding from ftrace handlers Josh Poimboeuf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.