x86,ftrace: Fix ftrace_regs_caller() unwind

The ftrace_regs_caller() trampoline does something 'funny' when there
is a direct-caller present. In that case it stuffs the 'direct-caller'
address on the return stack and then exits the function. This then
results in 'returning' to the direct-caller with the exact registers
we came in with -- an indirect tail-call without using a register.

This however (rightfully) confuses objtool because the function shares
a few instruction in order to have a single exit path, but the stack
layout is different for them, depending through which path we came
there.

This is currently cludged by forcing the stack state to the non-direct
case, but this generates actively wrong (ORC) unwind information for
the direct case, leading to potential broken unwinds.

Fix this issue by fully separating the exit paths. This results in
having to poke a second RET into the trampoline copy, see
ftrace_regs_caller_ret.

This brings us to a second objtool problem, in order for it to
perceive the 'jmp ftrace_epilogue' as a function exit, it needs to be
recognised as a tail call. In order to make that happen,
ftrace_epilogue needs to be the start of an STT_FUNC, so re-arrange
code to make this so.

Finally, a third issue is that objtool requires functions to exit with
the same stack layout they started with, which is obviously violated
in the direct case, employ the new HINT_RET_OFFSET to tell objtool
this is an expected exception.

Together, this results in generating correct ORC unwind information
for the ftrace_regs_caller() function and it's trampoline copies.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lkml.kernel.org/r/20200416115118.749606694@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 37a0aeaf89e771b63bccbeab92979469cbd96ecb..867c126ddabe39abb1c8eea0f318fcf3435c8f54 100644 (file)
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -282,7 +282,8 @@ static inline void tramp_free(void *tramp) { }
 
 /* Defined as markers to the end of the ftrace default trampolines */
 extern void ftrace_regs_caller_end(void);
-extern void ftrace_epilogue(void);
+extern void ftrace_regs_caller_ret(void);
+extern void ftrace_caller_end(void);
 extern void ftrace_caller_op_ptr(void);
 extern void ftrace_regs_caller_op_ptr(void);
 
@@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
                call_offset = (unsigned long)ftrace_regs_call;
        } else {
                start_offset = (unsigned long)ftrace_caller;
-               end_offset = (unsigned long)ftrace_epilogue;
+               end_offset = (unsigned long)ftrace_caller_end;
                op_offset = (unsigned long)ftrace_caller_op_ptr;
                call_offset = (unsigned long)ftrace_call;
        }
@@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
        if (WARN_ON(ret < 0))
                goto fail;
 
+       if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
+               ip = trampoline + (ftrace_regs_caller_ret - ftrace_regs_caller);
+               ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
+               if (WARN_ON(ret < 0))
+                       goto fail;
+       }
+
        /*
         * The address of the ftrace_ops that is used for this trampoline
         * is stored at the end of the trampoline. This will be used to

diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe39843a060e359d2c81c8ce7f27bf..7657dc782828818e5802ff98297ba215546cf9b4 100644 (file)
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -157,8 +157,12 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
         * think twice before adding any new code or changing the
         * layout here.
         */
-SYM_INNER_LABEL(ftrace_epilogue, SYM_L_GLOBAL)
+SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
 
+       jmp ftrace_epilogue
+SYM_FUNC_END(ftrace_caller);
+
+SYM_FUNC_START(ftrace_epilogue)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
        jmp ftrace_stub
@@ -170,14 +174,12 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
  */
 SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK)
        retq
-SYM_FUNC_END(ftrace_caller)
+SYM_FUNC_END(ftrace_epilogue)
 
 SYM_FUNC_START(ftrace_regs_caller)
        /* Save the current flags before any operations that can change them */
        pushfq
 
-       UNWIND_HINT_SAVE
-
        /* added 8 bytes to save flags */
        save_mcount_regs 8
        /* save_mcount_regs fills in first two parameters */
@@ -233,7 +235,10 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
        movq ORIG_RAX(%rsp), %rax
        movq %rax, MCOUNT_REG_SIZE-8(%rsp)
 
-       /* If ORIG_RAX is anything but zero, make this a call to that */
+       /*
+        * If ORIG_RAX is anything but zero, make this a call to that.
+        * See arch_ftrace_set_direct_caller().
+        */
        movq ORIG_RAX(%rsp), %rax
        cmpq    $0, %rax
        je      1f
@@ -244,20 +249,14 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
        movq %rax, MCOUNT_REG_SIZE(%rsp)
 
        restore_mcount_regs 8
+       /* Restore flags */
+       popfq
 
-       jmp     2f
+SYM_INNER_LABEL(ftrace_regs_caller_ret, SYM_L_GLOBAL);
+       UNWIND_HINT_RET_OFFSET
+       jmp     ftrace_epilogue
 
 1:     restore_mcount_regs
-
-
-2:
-       /*
-        * The stack layout is nondetermistic here, depending on which path was
-        * taken.  This confuses objtool and ORC, rightfully so.  For now,
-        * pretend the stack always looks like the non-direct case.
-        */
-       UNWIND_HINT_RESTORE
-
        /* Restore flags */
        popfq
 
@@ -268,7 +267,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
         * to the return.
         */
 SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
-
        jmp ftrace_epilogue
 
 SYM_FUNC_END(ftrace_regs_caller)