arm64: unwind: fix broken exception stack dump

(quoted diff) +0 -8

@@ -203,20 +193,13 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 		ret = unwind_frame(tsk, &frame);
 		if (ret < 0)
 			break;
-		stack = frame.sp;
 		if (in_exception_text(where)) {
-			/*
-			 * If we switched to the irq_stack before calling this
-			 * exception handler, then the pt_regs will be on the
-			 * task stack. The easiest way to tell is if the large
-			 * pt_regs would overlap with the end of the irq_stack.
-			 */
-			if (stack < irq_stack_ptr &&

Hi Ard,

This series looks good. I've tested the whole thing with perf top -g and sysrq-L.

In the process of reviewing this, I spotted some other (existing) bugs in this area.

Quoted 13 lines · Ard Biesheuvel

(quoted diff) +0 -8

@@ -203,20 +193,13 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
              ret = unwind_frame(tsk, &frame);
              if (ret < 0)
                      break;
-             stack = frame.sp;
              if (in_exception_text(where)) {
-                     /*
-                      * If we switched to the irq_stack before calling this
-                      * exception handler, then the pt_regs will be on the
-                      * task stack. The easiest way to tell is if the large
-                      * pt_regs would overlap with the end of the irq_stack.
-                      */
-                     if (stack < irq_stack_ptr &&

Review void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) · line 202

Quoted 10 lines

(quoted diff) +5 -4

-                         (stack + sizeof(struct pt_regs)) > irq_stack_ptr)
-                             stack = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+                     stack = frame.fp - offsetof(struct pt_regs, stackframe);
 
-                     dump_mem("", "Exception stack", stack,
-                              stack + sizeof(struct pt_regs));
+                     if (on_task_stack(tsk, stack) ||
+                         (tsk == current && !preemptible() && on_irq_stack(stack)))
+                             dump_mem("", "Exception stack", stack,
+                                      stack + sizeof(struct pt_regs));

So here, we're using information from three records (where the last might have been faked from the current FP + LR). We look at the previous frame's LR value to determine whether the current frame's fp points to a next frame that's associated with some regs:

         +----+----------------+
         | fp | interrupted PC | /* embedded in pt_regs */
         +----+----------------+
           /\
           ||
         +----+----------------+
where => | fp | < entry PC >   |
         +----+----------------+
           /\
           ||
         +----+----------------+
         | fp | where          | /* "where" is in exception text */
         +----+----------------+

Which (IIUC) has three problems, inherited from the existing approach.

1) Several C functions called from exception entry assembly are missing __exception annotations. e.g. bad_mode.

So we can miss exception regs in some cases.

2) Several functions may be called either from the exception entry assembly, or from other C code. e.g. do_undefinstr, and any cascaded irqchip handlers.

So we can spuriously decide part of the stack is a pt_regs.

3) The entry assembly can enable exceptions before calling C code, so if an exception is taken at the right time, these will eb in the backtrace without a surrounding frame in the exception text.

So we can miss exception regs in some cases (distinct from (1)).

... given that, I don't think we can rely on in_exception_text(). I think we have to look at whether the current frame's PC/LR is in .entry.text.

Fixup to that effect below, tested with sysrq-L and perf top -g. Note that cpu_switch_to and ret_from_fork have to be moved into .text to avoid false positives. I also killed 'where' since it's no longer necessary to remember the previous frame.

There's another bug -- we always dump the exception regs, even when skipping frames. The sqsrq-L handler tries to skip frames, so you get the regs for irq_entry, but without an irq_entry backtrace, which is confusing. That's simple enough to fix up separately, I guess.

---->8----

arch/arm64/include/asm/traps.h +5 -0

index 02e9035..4136168 100644
@@ -60,4 +60,9 @@ static inline int in_exception_text(unsigned long ptr)
        return in ? : __in_irqentry_text(ptr);
 }
 
+static inline int in_entry_text(unsigned long ptr)
+{
+       return ptr >= (unsigned long)&__entry_text_start &&
+              ptr < (unsigned long)&__entry_text_end;
+}
 #endif

arch/arm64/kernel/entry.S +44 -44

index 32f3b24..3d25ec5 100644
@@ -707,38 +707,6 @@ el0_irq_naked:
 ENDPROC(el0_irq)
 
 /*
- * Register switch for AArch64. The callee-saved registers need to be saved
- * and restored. On entry:
- *   x0 = previous task_struct (must be preserved across the switch)
- *   x1 = next task_struct
- * Previous and next are guaranteed not to be the same.
- *
- */
-ENTRY(cpu_switch_to)
-       mov     x10, #THREAD_CPU_CONTEXT
-       add     x8, x0, x10
-       mov     x9, sp
-       stp     x19, x20, [x8], #16             // store callee-saved registers
-       stp     x21, x22, [x8], #16
-       stp     x23, x24, [x8], #16
-       stp     x25, x26, [x8], #16
-       stp     x27, x28, [x8], #16
-       stp     x29, x9, [x8], #16
-       str     lr, [x8]
-       add     x8, x1, x10
-       ldp     x19, x20, [x8], #16             // restore callee-saved registers
-       ldp     x21, x22, [x8], #16
-       ldp     x23, x24, [x8], #16
-       ldp     x25, x26, [x8], #16
-       ldp     x27, x28, [x8], #16
-       ldp     x29, x9, [x8], #16
-       ldr     lr, [x8]
-       mov     sp, x9
-       msr     sp_el0, x1
-       ret
-ENDPROC(cpu_switch_to)
-
-/*
  * This is the fast syscall return path.  We do as little as possible here,
  * and this includes saving x0 back into the kernel stack.
  */

@@ -781,18 +749,6 @@ finish_ret_to_user:
 ENDPROC(ret_to_user)
 
 /*
- * This is how we return from a fork.
- */
-ENTRY(ret_from_fork)
-       bl      schedule_tail
-       cbz     x19, 1f                         // not a kernel thread
-       mov     x0, x20
-       blr     x19
-1:     get_thread_info tsk
-       b       ret_to_user
-ENDPROC(ret_from_fork)
-
-/*
  * SVC handler.
  */
        .align  6

@@ -865,3 +821,47 @@ ENTRY(sys_rt_sigreturn_wrapper)
        mov     x0, sp
        b       sys_rt_sigreturn
 ENDPROC(sys_rt_sigreturn_wrapper)
+
+/*
+ * Register switch for AArch64. The callee-saved registers need to be saved
+ * and restored. On entry:
+ *   x0 = previous task_struct (must be preserved across the switch)
+ *   x1 = next task_struct
+ * Previous and next are guaranteed not to be the same.
+ *
+ */
+ENTRY(cpu_switch_to)
+       mov     x10, #THREAD_CPU_CONTEXT
+       add     x8, x0, x10
+       mov     x9, sp
+       stp     x19, x20, [x8], #16             // store callee-saved registers
+       stp     x21, x22, [x8], #16
+       stp     x23, x24, [x8], #16
+       stp     x25, x26, [x8], #16
+       stp     x27, x28, [x8], #16
+       stp     x29, x9, [x8], #16
+       str     lr, [x8]
+       add     x8, x1, x10
+       ldp     x19, x20, [x8], #16             // restore callee-saved registers
+       ldp     x21, x22, [x8], #16
+       ldp     x23, x24, [x8], #16
+       ldp     x25, x26, [x8], #16
+       ldp     x27, x28, [x8], #16
+       ldp     x29, x9, [x8], #16
+       ldr     lr, [x8]
+       mov     sp, x9
+       msr     sp_el0, x1
+       ret
+ENDPROC(cpu_switch_to)
+
+/*
+ * This is how we return from a fork.
+ */
+ENTRY(ret_from_fork)
+       bl      schedule_tail
+       cbz     x19, 1f                         // not a kernel thread
+       mov     x0, x20
+       blr     x19
+1:     get_thread_info tsk
+       b       ret_to_user
+ENDPROC(ret_from_fork)

arch/arm64/kernel/traps.c +2 -3

index 9ba060b..63cbfb0 100644
@@ -170,13 +170,12 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
        skip = !!regs;
        printk("Call trace:\n");
        while (1) {
-               unsigned long where = frame.pc;
                unsigned long stack;
                int ret;
 
                /* skip until specified stack frame */
                if (!skip) {
-                       dump_backtrace_entry(where);
+                       dump_backtrace_entry(frame.pc);
                } else if (frame.fp == regs->regs[29]) {
                        skip = 0;
                        /*

@@ -191,7 +190,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
                ret = unwind_frame(tsk, &frame);
                if (ret < 0)
                        break;
-               if (in_exception_text(where)) {
+               if (in_entry_text(frame.pc)) {
                        stack = frame.fp - offsetof(struct pt_regs, stackframe);
 
                        if (on_task_stack(tsk, stack) ||

Quoted 69 lines · Mark Rutland

Hi Ard,

This series looks good. I've tested the whole thing with perf top -g and sysrq-L.

In the process of reviewing this, I spotted some other (existing) bugs in this area.

Quoted 13 lines · Ard Biesheuvel

(quoted diff) +0 -8

@@ -203,20 +193,13 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
              ret = unwind_frame(tsk, &frame);
              if (ret < 0)
                      break;
-             stack = frame.sp;
              if (in_exception_text(where)) {
-                     /*
-                      * If we switched to the irq_stack before calling this
-                      * exception handler, then the pt_regs will be on the
-                      * task stack. The easiest way to tell is if the large
-                      * pt_regs would overlap with the end of the irq_stack.
-                      */
-                     if (stack < irq_stack_ptr &&

Review void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) · line 202

Quoted 10 lines

(quoted diff) +5 -4

-                         (stack + sizeof(struct pt_regs)) > irq_stack_ptr)
-                             stack = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+                     stack = frame.fp - offsetof(struct pt_regs, stackframe);
 
-                     dump_mem("", "Exception stack", stack,
-                              stack + sizeof(struct pt_regs));
+                     if (on_task_stack(tsk, stack) ||
+                         (tsk == current && !preemptible() && on_irq_stack(stack)))
+                             dump_mem("", "Exception stack", stack,
+                                      stack + sizeof(struct pt_regs));

So here, we're using information from three records (where the last might have been faked from the current FP + LR). We look at the previous frame's LR value to determine whether the current frame's fp points to a next frame that's associated with some regs:

         +----+----------------+
         | fp | interrupted PC | /* embedded in pt_regs */
         +----+----------------+
           /\
           ||
         +----+----------------+
where => | fp | < entry PC >   |
         +----+----------------+
           /\
           ||
         +----+----------------+
         | fp | where          | /* "where" is in exception text */
         +----+----------------+

Which (IIUC) has three problems, inherited from the existing approach.

1) Several C functions called from exception entry assembly are missing __exception annotations. e.g. bad_mode.

So we can miss exception regs in some cases.

2) Several functions may be called either from the exception entry assembly, or from other C code. e.g. do_undefinstr, and any cascaded irqchip handlers.

So we can spuriously decide part of the stack is a pt_regs.

3) The entry assembly can enable exceptions before calling C code, so if an exception is taken at the right time, these will eb in the backtrace without a surrounding frame in the exception text.

Quoted 6 lines

So we can miss exception regs in some cases (distinct from (1)).

... given that, I don't think we can rely on in_exception_text(). I think we have to look at whether the current frame's PC/LR is in .entry.text.

I guess that's an improvement, yes. At least we won't have false positives where a stack frame is misidentified as being on embedded in pt_regs. But it does rely on the bl instruction being used even in cases where we know we won't be returning, i.e., jumps to bad_mode, do_sp_pc_abort and do_undef_instr from .entry.text

Quoted 25 lines · Ard Biesheuvel

Quoted 24 lines · Mark Rutland

So here, we're using information from three records (where the last might have been faked from the current FP + LR). We look at the previous frame's LR value to determine whether the current frame's fp points to a next frame that's associated with some regs:

         +----+----------------+
         | fp | interrupted PC | /* embedded in pt_regs */
         +----+----------------+
           /\
           ||
         +----+----------------+
where => | fp | < entry PC >   |
         +----+----------------+
           /\
           ||
         +----+----------------+
         | fp | where          | /* "where" is in exception text */
         +----+----------------+

Which (IIUC) has three problems, inherited from the existing approach.

3) The entry assembly can enable exceptions before calling C code, so if an exception is taken at the right time, these will eb in the backtrace without a surrounding frame in the exception text.

Quoted 5 lines

I guess that's an improvement, yes. At least we won't have false positives where a stack frame is misidentified as being on embedded in pt_regs. But it does rely on the bl instruction being used even in cases where we know we won't be returning, i.e., jumps to bad_mode, do_sp_pc_abort and do_undef_instr from .entry.text

I guess we could do something along the lines of

"""

arch/arm64/kernel/entry.S +19 -3

index 45947b36f1ff..37f67c2080dc 100644
@@ -60,6 +60,22 @@
 #endif
        .endm
 
+       .macro          b_e, target, b=b
+       .pushsection    ".rodata.entrycalls", "a", @progbits
+       .align          2
+       .long           6767f - .
+       .popsection
+6767:  \b              \target
+       .endm
+
+       .macro          bl_e, target
+       b_e             \target, bl
+       .endm
+
+       .macro          blr_e, target
+       b_e             \target, blr
+       .endm
+
 /*
  * Bad Abort numbers
  *-----------------

@@ -352,7 +368,7 @@ tsk .req    x28             // current thread_info
        ldr_l   x1, handle_arch_irq
        mov     x0, sp
        irq_stack_entry
-       blr     x1
+       blr_e   x1
        irq_stack_exit
        .endm
 

@@ -381,7 +397,7 @@ __bad_stack:
        mov     x0, sp
 
        /* Time to die */
-       bl      handle_bad_stack
+       bl_e    handle_bad_stack
        nop                             // ensure lr points somewhere sane
 ENDPROC(__bad_stack)
 #endif /* CONFIG_VMAP_STACK */

@@ -429,7 +445,7 @@ END(vectors)
        mov     x0, sp
        mov     x1, #\reason
        mrs     x2, esr_el1
-       b       bad_mode
+       b_e     bad_mode
        .endm
 
 el0_sync_invalid:

"""

etc to explicitly record the places where we call out of the entry code with a pt_regs struct on the top of the stack.

Then, we can use

static bool is_entry_call(unsigned long pc)

{
    extern s32 __entrycalls_start[], __entrycalls_end[];
    s32 *p;

    for (p = __entrycalls_start; p < __entrycalls_end; p++)
        if ((unsigned long)p + *p == pc - 4)
            return true;
    return false;
}

to identify values of frame.pc that coincide with such calls.

Quoted 26 lines · Ard Biesheuvel

Quoted 25 lines · Ard Biesheuvel

Quoted 24 lines · Mark Rutland

So here, we're using information from three records (where the last might have been faked from the current FP + LR). We look at the previous frame's LR value to determine whether the current frame's fp points to a next frame that's associated with some regs:

         +----+----------------+
         | fp | interrupted PC | /* embedded in pt_regs */
         +----+----------------+
           /\
           ||
         +----+----------------+
where => | fp | < entry PC >   |
         +----+----------------+
           /\
           ||
         +----+----------------+
         | fp | where          | /* "where" is in exception text */
         +----+----------------+

Which (IIUC) has three problems, inherited from the existing approach.

3) The entry assembly can enable exceptions before calling C code, so if an exception is taken at the right time, these will eb in the backtrace without a surrounding frame in the exception text.

[...]

Quoted 5 lines

Quoted 5 lines

I guess that's an improvement, yes. At least we won't have false positives where a stack frame is misidentified as being on embedded in pt_regs. But it does rely on the bl instruction being used even in cases where we know we won't be returning, i.e., jumps to bad_mode, do_sp_pc_abort and do_undef_instr from .entry.text

Good point, yes.

This would be good for backtracing, regardless.

Quoted 75 lines

I guess we could do something along the lines of

"""

arch/arm64/kernel/entry.S +19 -3

index 45947b36f1ff..37f67c2080dc 100644
@@ -60,6 +60,22 @@
 #endif
        .endm
 
+       .macro          b_e, target, b=b
+       .pushsection    ".rodata.entrycalls", "a", @progbits
+       .align          2
+       .long           6767f - .
+       .popsection
+6767:  \b              \target
+       .endm
+
+       .macro          bl_e, target
+       b_e             \target, bl
+       .endm
+
+       .macro          blr_e, target
+       b_e             \target, blr
+       .endm
+
 /*
  * Bad Abort numbers
  *-----------------

@@ -352,7 +368,7 @@ tsk .req    x28             // current thread_info
        ldr_l   x1, handle_arch_irq
        mov     x0, sp
        irq_stack_entry
-       blr     x1
+       blr_e   x1
        irq_stack_exit
        .endm
 

@@ -381,7 +397,7 @@ __bad_stack:
        mov     x0, sp
 
        /* Time to die */
-       bl      handle_bad_stack
+       bl_e    handle_bad_stack
        nop                             // ensure lr points somewhere sane
 ENDPROC(__bad_stack)
 #endif /* CONFIG_VMAP_STACK */

@@ -429,7 +445,7 @@ END(vectors)
        mov     x0, sp
        mov     x1, #\reason
        mrs     x2, esr_el1
-       b       bad_mode
+       b_e     bad_mode
        .endm
 
 el0_sync_invalid:

"""

etc to explicitly record the places where we call out of the entry code with a pt_regs struct on the top of the stack.

Then, we can use

static bool is_entry_call(unsigned long pc)

{
    extern s32 __entrycalls_start[], __entrycalls_end[];
    s32 *p;

    for (p = __entrycalls_start; p < __entrycalls_end; p++)
        if ((unsigned long)p + *p == pc - 4)
            return true;
    return false;
}

Quoted 1 line

to identify values of frame.pc that coincide with such calls.

Unfortunately, I believe that fails for case (3), where entry code is interrupted before calling C code. e.g. if an interrupt was taken during data abort handling:

        kernel_entry 1
        ...
el1_da:
        ...
        enable_irq
        < interrupted here >
        ...
        bl      do_mem_abort

... as we'd dump the regs for the interrupt, but not those for the data abort.

Quoted 13 lines · Ard Biesheuvel

(quoted diff) +0 -8

@@ -203,20 +193,13 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 		ret = unwind_frame(tsk, &frame);
 		if (ret < 0)
 			break;
-		stack = frame.sp;
 		if (in_exception_text(where)) {
-			/*
-			 * If we switched to the irq_stack before calling this
-			 * exception handler, then the pt_regs will be on the
-			 * task stack. The easiest way to tell is if the large
-			 * pt_regs would overlap with the end of the irq_stack.
-			 */
-			if (stack < irq_stack_ptr &&

Review void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) · line 202

Quoted 10 lines

(quoted diff) +5 -4

-			    (stack + sizeof(struct pt_regs)) > irq_stack_ptr)
-				stack = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+			stack = frame.fp - offsetof(struct pt_regs, stackframe);
 
-			dump_mem("", "Exception stack", stack,
-				 stack + sizeof(struct pt_regs));
+			if (on_task_stack(tsk, stack) ||
+			    (tsk == current && !preemptible() && on_irq_stack(stack)))
+				dump_mem("", "Exception stack", stack,
+					 stack + sizeof(struct pt_regs));

So here, we're using information from three records (where the last might have been faked from the current FP + LR). We look at the previous frame's LR value to determine whether the current frame's fp points to a next frame that's associated with some regs:

         +----+----------------+
         | fp | interrupted PC | /* embedded in pt_regs */
         +----+----------------+
           /\
           ||
         +----+----------------+
where => | fp | < entry PC >   |
         +----+----------------+
           /\
           ||
         +----+----------------+
         | fp | where          | /* "where" is in exception text */
         +----+----------------+

Which (IIUC) has three problems, inherited from the existing approach.

1) Several C functions called from exception entry assembly are missing __exception annotations. e.g. bad_mode.

So we can miss exception regs in some cases.

2) Several functions may be called either from the exception entry assembly, or from other C code. e.g. do_undefinstr, and any cascaded irqchip handlers.

So we can spuriously decide part of the stack is a pt_regs.

3) The entry assembly can enable exceptions before calling C code, so if an exception is taken at the right time, these will eb in the backtrace without a surrounding frame in the exception text.

So we can miss exception regs in some cases (distinct from (1)).

... given that, I don't think we can rely on in_exception_text(). I think we have to look at whether the current frame's PC/LR is in .entry.text.

Quoted 38 lines · Mark Rutland

On Mon, Jul 24, 2017 at 12:26:22PM +0100, Ard Biesheuvel wrote:

So here, we're using information from three records (where the last might have been faked from the current FP + LR). We look at the previous frame's LR value to determine whether the current frame's fp points to a next frame that's associated with some regs:

         +----+----------------+
         | fp | interrupted PC | /* embedded in pt_regs */
         +----+----------------+
           /\
           ||
         +----+----------------+
where => | fp | < entry PC >   |
         +----+----------------+
           /\
           ||
         +----+----------------+
         | fp | where          | /* "where" is in exception text */
         +----+----------------+

Which (IIUC) has three problems, inherited from the existing approach.

1) Several C functions called from exception entry assembly are missing __exception annotations. e.g. bad_mode.

So we can miss exception regs in some cases.

2) Several functions may be called either from the exception entry assembly, or from other C code. e.g. do_undefinstr, and any cascaded irqchip handlers.

So we can spuriously decide part of the stack is a pt_regs.

3) The entry assembly can enable exceptions before calling C code, so if an exception is taken at the right time, these will eb in the backtrace without a surrounding frame in the exception text.

Quoted 5 lines

So we can miss exception regs in some cases (distinct from (1)).

... given that, I don't think we can rely on in_exception_text(). I think we have to look at whether the current frame's PC/LR is in .entry.text.

Looking around, I think most (all?) users of in_exception_text and the associated linker script labels suffer from similar problems. AFAICT, these are:

kprobes - Used to blacklist probes, but omits entry code
ftrace - For graph tracer backtracing; attempted to hack around the
problem in 9a5ad7d0e3e1 but still excludes .entry.text
kasan - Duplicates ftrace stuff in core code, but also checks
against softirqentry

So it looks like we should either include the entry text in these sections or avoid using these functions altogether (eventually removing them). It needn't hold this series up, but worth considering.

What do you think?