x86/unwind: Add end-of-stack check for ftrace handlers

Dave Jones and Steven Rostedt reported unwinder warnings like the
following:

  WARNING: kernel stack frame pointer at ffff8800bda0ff30 in sshd:1090 has bad value 000055b32abf1fa8

In both cases, the unwinder was attempting to unwind from an ftrace
handler into entry code.  The callchain was something like:

  syscall entry code
    C function
      ftrace handler
        save_stack_trace()

The problem is that the unwinder's end-of-stack logic gets confused by
the way ftrace lays out the stack frame (with fentry enabled).

I was able to recreate this warning with:

  echo call_usermodehelper_exec_async:stacktrace > /sys/kernel/debug/tracing/set_ftrace_filter
  (exit login session)

I considered fixing this by changing the ftrace code to rewrite the
stack to make the unwinder happy.  But that seemed too intrusive after I
implemented it.  Instead, just add another check to the unwinder's
end-of-stack logic to detect this special case.

Side note: We could probably get rid of these end-of-stack checks by
encoding the frame pointer for syscall entry just like we do for
interrupt entry.  That would be simpler, but it would also be a lot more
intrusive since it would slightly affect the performance of every
syscall.

Reported-by: Dave Jones <davej@codemonkey.org.uk>
Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Petr Mladek <pmladek@suse.com>
Cc: live-patching@vger.kernel.org
Fixes: c32c47c68a0a ("x86/unwind: Warn on bad frame pointer")
Link: http://lkml.kernel.org/r/671ba22fbc0156b8f7e0cfa5ab2a795e08bc37e1.1495553739.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 82c6d7f1fd73e64debd566d0846f3b9a62ab8959..b9389d72b2f784887e14acc89a6346a78c13c1b4 100644 (file)
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -104,6 +104,11 @@ static inline unsigned long *last_frame(struct unwind_state *state)
        return (unsigned long *)task_pt_regs(state->task) - 2;
 }
 
+static bool is_last_frame(struct unwind_state *state)
+{
+       return state->bp == last_frame(state);
+}
+
 #ifdef CONFIG_X86_32
 #define GCC_REALIGN_WORDS 3
 #else
@@ -115,16 +120,15 @@ static inline unsigned long *last_aligned_frame(struct unwind_state *state)
        return last_frame(state) - GCC_REALIGN_WORDS;
 }
 
-static bool is_last_task_frame(struct unwind_state *state)
+static bool is_last_aligned_frame(struct unwind_state *state)
 {
        unsigned long *last_bp = last_frame(state);
        unsigned long *aligned_bp = last_aligned_frame(state);
 
        /*
-        * We have to check for the last task frame at two different locations
-        * because gcc can occasionally decide to realign the stack pointer and
-        * change the offset of the stack frame in the prologue of a function
-        * called by head/entry code.  Examples:
+        * GCC can occasionally decide to realign the stack pointer and change
+        * the offset of the stack frame in the prologue of a function called
+        * by head/entry code.  Examples:
         *
         * <start_secondary>:
         *      push   %edi
@@ -141,11 +145,38 @@ static bool is_last_task_frame(struct unwind_state *state)
         *      push   %rbp
         *      mov    %rsp,%rbp
         *
-        * Note that after aligning the stack, it pushes a duplicate copy of
-        * the return address before pushing the frame pointer.
+        * After aligning the stack, it pushes a duplicate copy of the return
+        * address before pushing the frame pointer.
+        */
+       return (state->bp == aligned_bp && *(aligned_bp + 1) == *(last_bp + 1));
+}
+
+static bool is_last_ftrace_frame(struct unwind_state *state)
+{
+       unsigned long *last_bp = last_frame(state);
+       unsigned long *last_ftrace_bp = last_bp - 3;
+
+       /*
+        * When unwinding from an ftrace handler of a function called by entry
+        * code, the stack layout of the last frame is:
+        *
+        *   bp
+        *   parent ret addr
+        *   bp
+        *   function ret addr
+        *   parent ret addr
+        *   pt_regs
+        *   -----------------
         */
-       return (state->bp == last_bp ||
-               (state->bp == aligned_bp && *(aligned_bp+1) == *(last_bp+1)));
+       return (state->bp == last_ftrace_bp &&
+               *state->bp == *(state->bp + 2) &&
+               *(state->bp + 1) == *(state->bp + 4));
+}
+
+static bool is_last_task_frame(struct unwind_state *state)
+{
+       return is_last_frame(state) || is_last_aligned_frame(state) ||
+              is_last_ftrace_frame(state);
 }
 
 /*