[RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly

[RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly

[Steven Rostedt]

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 */

Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly

[Josh Poimboeuf]

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

Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly

[Steven Rostedt]

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

Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly

[Josh Poimboeuf]

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

Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly

[Steven Rostedt]

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

[PATCH] x86/ftrace: Fix ORC unwinding from ftrace handlers

[Josh Poimboeuf]

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