Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1461224 > unrolled thread

[PATCH v3 00/51] x86/dumpstack: rewrite x86 stack dump code

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 00/51] x86/dumpstack: rewrite x86 stack dump code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 49/51] x86/dumpstack: warn on stack recursion Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 36/51] perf/x86: convert perf_callchain_kernel() to use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 26/51] ftrace: add ftrace_graph_ret_addr() stack unwinding helpers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 33/51] x86/dumpstack: add get_stack_info() interface Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 48/51] x86/unwind: warn if stack grows up Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
        Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 18:30 +0200
    [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 19:40 +0200
        Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to  usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 21:20 +0200
          Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 22:10 +0200
            Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to  usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 22:40 +0200
              Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 22:50 +0200
    [PATCH v3 44/51] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 44/51] x86/dumpstack: print any pt_regs found on the stack Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
    [PATCH v3 12/51] x86: move _stext marker to before head code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
        Re: [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
    [PATCH v3 14/51] x86/asm/head:  put real return address on idle task stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle  task stack Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
      Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle  task stack Nilay Vaish <nilayvaish@gmail.com> - 2016-08-17 22:40 +0200
        Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle  task stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-17 23:20 +0200
    [PATCH v3 16/51] x86/32: put real return address on stack in entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
        Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:10 +0200
          Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code "H. Peter Anvin" <hpa@zytor.com> - 2016-08-15 20:10 +0200
            Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 20:30 +0200
              Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code "H. Peter Anvin" <hpa@zytor.com> - 2016-08-15 21:30 +0200
                Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 22:10 +0200
    [PATCH v3 18/51] x86/entry/head/32: use local labels Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 30/51] x86/dumpstack/ftrace: don't print unreliable addresses in print_context_stack_bp() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 42/51] x86/unwind: create stack frames for saved syscall registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 42/51] x86/unwind: create stack frames for saved  syscall registers Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
        Re: [PATCH v3 42/51] x86/unwind: create stack frames for saved  syscall registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 19:00 +0200
    [PATCH v3 28/51] ftrace/x86: implement HAVE_FUNCTION_GRAPH_RET_ADDR_PTR Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 31/51] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 31/51] x86/dumpstack: allow preemption in  show_stack_log_lvl() and dump_trace() Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
        Re: [PATCH v3 31/51] x86/dumpstack: allow preemption in  show_stack_log_lvl() and dump_trace() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
    [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved interrupt registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved  interrupt registers Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
        Re: [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved  interrupt registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 18:40 +0200
    [PATCH v3 37/51] x86/stacktrace: convert save_stack_trace_*() to use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 05/51] x86/dumpstack: make printk_stack_address() more generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 43/51] x86/dumpstack: print stack identifier on its own line Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 24/51] ftrace: only allocate the ret_stack 'fp' field when needed Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 29/51] x86/dumpstack/ftrace: mark function graph handler function as unreliable Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 03/51] x86/asm/head: rename 'stack_start' -> 'initial_stack' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
      Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer()  previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
        Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer()  previous stack access Brian Gerst <brgerst@gmail.com> - 2016-08-14 15:00 +0200
          Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer()  previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-14 15:50 +0200
        Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32  kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:10 +0200
          Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32  kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 19:30 +0200
            Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer()  previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-15 22:10 +0200
    [PATCH v3 13/51] x86/asm/head: remove useless zeroed word Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 21/51] oprofile/x86: add regs->ip to oprofile trace Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 10/51] x86/dumpstack: add get_stack_pointer() and get_frame_pointer() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 07/51] x86/dumpstack: remove extra brackets around "<EOE>" Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 17/51] x86/smp: fix initial idle stack location on 32-bit Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 08/51] x86/dumpstack: fix irq stack bounds calculation in show_stack_log_lvl() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 15/51] x86/asm/head: standardize the end of the stack for idle tasks Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
      Re: [PATCH v3 15/51] x86/asm/head: standardize the end of the stack  for idle tasks Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
    [PATCH v3 19/51] x86/entry/32: rename 'error_code' to 'common_exception' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
      Re: [PATCH v3 19/51] x86/entry/32: rename 'error_code' to 'common_exception' Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
        Re: [PATCH v3 19/51] x86/entry/32: rename 'error_code' to  'common_exception' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
    [PATCH v3 23/51] ftrace: remove CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST from config Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 11/51] x86/dumpstack: remove unnecessary stack pointer arguments Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 17:10 +0200

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

#1461244 — [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved interrupt registers

With frame pointers, when a task is interrupted, its stack is no longer
completely reliable because the function could have been interrupted
before it had a chance to save the previous frame pointer on the stack.
So the caller of the interrupted function could get skipped by a stack
trace.

This is problematic for live patching, which needs to know whether a
stack trace of a sleeping task can be relied upon.  There's currently no
way to detect if a sleeping task was interrupted by a page fault
exception or preemption before it went to sleep.

Another issue is that when dumping the stack of an interrupted task, the
unwinder has no way of knowing where the saved pt_regs registers are, so
it can't print them.

This solves those issues by encoding the pt_regs pointer in the frame
pointer on entry from an interrupt or an exception.

This patch also updates the unwinder to be able to decode it, because
otherwise the unwinder would be broken by this change.

Note that this causes a change in the behavior of the unwinder: each
instance of a pt_regs on the stack is now considered a "frame".  So
callers of unwind_get_return_address() will now get an occasional
'regs->ip' address that would have previously been skipped over.

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/entry/calling.h       | 21 +++++++++++
 arch/x86/entry/entry_32.S      | 40 ++++++++++++++++++---
 arch/x86/entry/entry_64.S      | 10 ++++--
 arch/x86/include/asm/unwind.h  | 18 ++++++++--
 arch/x86/kernel/unwind_frame.c | 82 +++++++++++++++++++++++++++++++++++++-----
 5 files changed, 153 insertions(+), 18 deletions(-)

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index 9a9e588..ab799a3 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -201,6 +201,27 @@ For 32-bit we have the following conventions - kernel is built with
 	.byte 0xf1
 	.endm
 
+	/*
+	 * This is a sneaky trick to help the unwinder find pt_regs on the
+	 * stack.  The frame pointer is replaced with an encoded pointer to
+	 * pt_regs.  The encoding is just a clearing of the highest-order bit,
+	 * which makes it an invalid address and is also a signal to the
+	 * unwinder that it's a pt_regs pointer in disguise.
+	 *
+	 * NOTE: This macro must be used *after* SAVE_EXTRA_REGS because it
+	 * corrupts the original rbp.
+	 */
+.macro ENCODE_FRAME_POINTER ptregs_offset=0
+#ifdef CONFIG_FRAME_POINTER
+	.if \ptregs_offset
+		leaq \ptregs_offset(%rsp), %rbp
+	.else
+		mov %rsp, %rbp
+	.endif
+	btr $63, %rbp
+#endif
+.endm
+
 #endif /* CONFIG_X86_64 */
 
 /*
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 4396278..4006fa3 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -174,6 +174,23 @@
 	SET_KERNEL_GS %edx
 .endm
 
+/*
+ * This is a sneaky trick to help the unwinder find pt_regs on the
+ * stack.  The frame pointer is replaced with an encoded pointer to
+ * pt_regs.  The encoding is just a clearing of the highest-order bit,
+ * which makes it an invalid address and is also a signal to the
+ * unwinder that it's a pt_regs pointer in disguise.
+ *
+ * NOTE: This macro must be used *after* SAVE_ALL because it corrupts the
+ * original rbp.
+ */
+.macro ENCODE_FRAME_POINTER
+#ifdef CONFIG_FRAME_POINTER
+	mov %esp, %ebp
+	btr $31, %ebp
+#endif
+.endm
+
 .macro RESTORE_INT_REGS
 	popl	%ebx
 	popl	%ecx
@@ -205,10 +222,16 @@
 .endm
 
 ENTRY(ret_from_fork)
+	call	1f
+1:	push	$0
+	mov	%esp, %ebp
+
 	pushl	%eax
 	call	schedule_tail
 	popl	%eax
 
+	addl	$8, %esp
+
 	/* When we fork, we trace the syscall return in the child, too. */
 	movl    %esp, %eax
 	call    syscall_return_slowpath
@@ -588,6 +611,7 @@ common_interrupt:
 	ASM_CLAC
 	addl	$-0x80, (%esp)			/* Adjust vector into the [-256, -1] range */
 	SAVE_ALL
+	ENCODE_FRAME_POINTER
 	TRACE_IRQS_OFF
 	movl	%esp, %eax
 	call	do_IRQ
@@ -599,6 +623,7 @@ ENTRY(name)				\
 	ASM_CLAC;			\
 	pushl	$~(nr);			\
 	SAVE_ALL;			\
+	ENCODE_FRAME_POINTER;		\
 	TRACE_IRQS_OFF			\
 	movl	%esp, %eax;		\
 	call	fn;			\
@@ -733,6 +758,7 @@ END(spurious_interrupt_bug)
 ENTRY(xen_hypervisor_callback)
 	pushl	$-1				/* orig_ax = -1 => not a system call */
 	SAVE_ALL
+	ENCODE_FRAME_POINTER
 	TRACE_IRQS_OFF
 
 	/*
@@ -787,6 +813,7 @@ ENTRY(xen_failsafe_callback)
 	jmp	iret_exc
 5:	pushl	$-1				/* orig_ax = -1 => not a system call */
 	SAVE_ALL
+	ENCODE_FRAME_POINTER
 	jmp	ret_from_exception
 
 .section .fixup, "ax"
@@ -1013,6 +1040,7 @@ common_exception:
 	pushl	%edx
 	pushl	%ecx
 	pushl	%ebx
+	ENCODE_FRAME_POINTER
 	cld
 	movl	$(__KERNEL_PERCPU), %ecx
 	movl	%ecx, %fs
@@ -1045,6 +1073,7 @@ ENTRY(debug)
 	ASM_CLAC
 	pushl	$-1				# mark this as an int
 	SAVE_ALL
+	ENCODE_FRAME_POINTER
 	xorl	%edx, %edx			# error code 0
 	movl	%esp, %eax			# pt_regs pointer
 
@@ -1060,11 +1089,11 @@ ENTRY(debug)
 
 .Ldebug_from_sysenter_stack:
 	/* We're on the SYSENTER stack.  Switch off. */
-	movl	%esp, %ebp
+	movl	%esp, %ebx
 	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esp
 	TRACE_IRQS_OFF
 	call	do_debug
-	movl	%ebp, %esp
+	movl	%ebx, %esp
 	jmp	ret_from_exception
 END(debug)
 
@@ -1087,6 +1116,7 @@ ENTRY(nmi)
 
 	pushl	%eax				# pt_regs->orig_ax
 	SAVE_ALL
+	ENCODE_FRAME_POINTER
 	xorl	%edx, %edx			# zero error code
 	movl	%esp, %eax			# pt_regs pointer
 
@@ -1105,10 +1135,10 @@ ENTRY(nmi)
 	 * We're on the SYSENTER stack.  Switch off.  No one (not even debug)
 	 * is using the thread stack right now, so it's safe for us to use it.
 	 */
-	movl	%esp, %ebp
+	movl	%esp, %ebx
 	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esp
 	call	do_nmi
-	movl	%ebp, %esp
+	movl	%ebx, %esp
 	jmp	.Lrestore_all_notrace
 
 #ifdef CONFIG_X86_ESPFIX32
@@ -1125,6 +1155,7 @@ ENTRY(nmi)
 	.endr
 	pushl	%eax
 	SAVE_ALL
+	ENCODE_FRAME_POINTER
 	FIXUP_ESPFIX_STACK			# %eax == %esp
 	xorl	%edx, %edx			# zero error code
 	call	do_nmi
@@ -1138,6 +1169,7 @@ ENTRY(int3)
 	ASM_CLAC
 	pushl	$-1				# mark this as an int
 	SAVE_ALL
+	ENCODE_FRAME_POINTER
 	TRACE_IRQS_OFF
 	xorl	%edx, %edx			# zero error code
 	movl	%esp, %eax			# pt_regs pointer
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index f6b40e5..6200318 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -434,6 +434,7 @@ END(irq_entries_start)
 	ALLOC_PT_GPREGS_ON_STACK
 	SAVE_C_REGS
 	SAVE_EXTRA_REGS
+	ENCODE_FRAME_POINTER
 
 	testb	$3, CS(%rsp)
 	jz	1f
@@ -907,6 +908,7 @@ ENTRY(xen_failsafe_callback)
 	ALLOC_PT_GPREGS_ON_STACK
 	SAVE_C_REGS
 	SAVE_EXTRA_REGS
+	ENCODE_FRAME_POINTER
 	jmp	error_exit
 END(xen_failsafe_callback)
 
@@ -950,6 +952,7 @@ ENTRY(paranoid_entry)
 	cld
 	SAVE_C_REGS 8
 	SAVE_EXTRA_REGS 8
+	ENCODE_FRAME_POINTER 8
 	movl	$1, %ebx
 	movl	$MSR_GS_BASE, %ecx
 	rdmsr
@@ -997,6 +1000,7 @@ ENTRY(error_entry)
 	cld
 	SAVE_C_REGS 8
 	SAVE_EXTRA_REGS 8
+	ENCODE_FRAME_POINTER 8
 	xorl	%ebx, %ebx
 	testb	$3, CS+8(%rsp)
 	jz	.Lerror_kernelspace
@@ -1179,6 +1183,7 @@ ENTRY(nmi)
 	pushq	%r13		/* pt_regs->r13 */
 	pushq	%r14		/* pt_regs->r14 */
 	pushq	%r15		/* pt_regs->r15 */
+	ENCODE_FRAME_POINTER
 
 	/*
 	 * At this point we no longer need to worry about stack damage
@@ -1192,11 +1197,10 @@ ENTRY(nmi)
 
 	/*
 	 * Return back to user mode.  We must *not* do the normal exit
-	 * work, because we don't want to enable interrupts.  Fortunately,
-	 * do_nmi doesn't modify pt_regs.
+	 * work, because we don't want to enable interrupts.
 	 */
 	SWAPGS
-	jmp	restore_c_regs_and_iret
+	jmp	restore_regs_and_iret
 
 .Lnmi_from_kernel:
 	/*
diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
index 6dcb44b..295dbd1 100644
--- a/arch/x86/include/asm/unwind.h
+++ b/arch/x86/include/asm/unwind.h
@@ -13,6 +13,7 @@ struct unwind_state {
 	int graph_idx;
 #ifdef CONFIG_FRAME_POINTER
 	unsigned long *bp;
+	struct pt_regs *regs;
 #else
 	unsigned long *sp;
 #endif
@@ -32,7 +33,7 @@ unsigned long *unwind_get_return_address_ptr(struct unwind_state *state)
 	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
 		return NULL;
 
-	return state->bp + 1;
+	return state->regs ? &state->regs->ip : state->bp + 1;
 }
 
 static inline unsigned long *unwind_get_stack_ptr(struct unwind_state *state)
@@ -40,7 +41,15 @@ static inline unsigned long *unwind_get_stack_ptr(struct unwind_state *state)
 	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
 		return NULL;
 
-	return state->bp;
+	return state->regs ? (unsigned long *)state->regs : state->bp;
+}
+
+static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)
+{
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return NULL;
+
+	return state->regs;
 }
 
 unsigned long unwind_get_return_address(struct unwind_state *state);
@@ -61,6 +70,11 @@ static inline unsigned long *unwind_get_stack_ptr(struct unwind_state *state)
 	return state->sp;
 }
 
+static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)
+{
+	return NULL;
+}
+
 static inline
 unsigned long unwind_get_return_address(struct unwind_state *state)
 {
diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 00ad526..e02acec 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -14,6 +14,9 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
 		return 0;
 
+	if (state->regs && user_mode(state->regs))
+		return 0;
+
 	addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
 				     addr_p);
 
@@ -21,6 +24,24 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 }
 EXPORT_SYMBOL_GPL(unwind_get_return_address);
 
+/*
+ * This determines if the frame pointer actually contains an encoded pointer to
+ * pt_regs on the stack.  See ENCODE_FRAME_POINTER.
+ */
+static struct pt_regs *decode_frame_pointer(unsigned long *bp)
+{
+	unsigned long regs = (unsigned long)bp;
+
+	/* if the MSB is set, it's not an encoded pointer */
+	if (regs & (1UL << (BITS_PER_LONG - 1)))
+		return NULL;
+
+	/* decode it by setting the MSB */
+	regs |= 1UL << (BITS_PER_LONG - 1);
+
+	return (struct pt_regs *)regs;
+}
+
 static bool update_stack_state(struct unwind_state *state, void *addr,
 			       size_t len)
 {
@@ -45,26 +66,59 @@ unknown:
 
 bool unwind_next_frame(struct unwind_state *state)
 {
-	unsigned long *next_bp;
+	struct pt_regs *regs;
+	unsigned long *next_bp, *next_sp;
+	size_t next_len;
 
 	if (unwind_done(state))
 		return false;
 
-	next_bp = (unsigned long *)*state->bp;
+	/* have we reached the end? */
+	if (state->regs && user_mode(state->regs))
+		goto the_end;
+
+	/* get the next frame pointer */
+	if (state->regs)
+		next_bp = (unsigned long *)state->regs->bp;
+	else
+		next_bp = (unsigned long *)*state->bp;
+
+	/* is the next frame pointer an encoded pointer to pt_regs? */
+	regs = decode_frame_pointer(next_bp);
+	if (regs) {
+		next_sp = (unsigned long *)regs;
+		next_len = sizeof(*regs);
+	} else {
+		next_sp = next_bp;
+		next_len = FRAME_HEADER_SIZE;
+	}
 
 	/* make sure the next frame's data is accessible */
-	if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
+	if (!update_stack_state(state, next_sp, next_len))
 		return false;
-
 	/* move to the next frame */
-	state->bp = next_bp;
+	if (regs) {
+		state->regs = regs;
+		state->bp = NULL;
+	} else {
+		state->bp = next_bp;
+		state->regs = NULL;
+	}
+
 	return true;
+
+the_end:
+	state->stack_info.type = STACK_TYPE_UNKNOWN;
+	return false;
 }
 EXPORT_SYMBOL_GPL(unwind_next_frame);
 
 void __unwind_start(struct unwind_state *state, struct task_struct *task,
 		    struct pt_regs *regs, unsigned long *first_sp)
 {
+	unsigned long *bp, *sp;
+	size_t len;
+
 	memset(state, 0, sizeof(*state));
 	state->task = task;
 
@@ -73,12 +127,22 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task,
 		return;
 
 	/* set up the first stack frame */
-	state->bp = get_frame_pointer(task, regs);
+	bp = get_frame_pointer(task, regs);
+	regs = decode_frame_pointer(bp);
+	if (regs) {
+		state->regs = regs;
+		sp = (unsigned long *)regs;
+		len = sizeof(*regs);
+	}
+	else {
+		state->bp = bp;
+		sp = bp;
+		len = FRAME_HEADER_SIZE;
+	}
 
 	/* initialize stack info and make sure the frame data is accessible */
-	get_stack_info(state->bp, state->task, &state->stack_info,
-		       &state->stack_mask);
-	update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
+	get_stack_info(sp, state->task, &state->stack_info, &state->stack_mask);
+	update_stack_state(state, sp, len);
 
 	/*
 	 * The caller can optionally provide a stack pointer directly
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461700 — Re: [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved interrupt registers

On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> With frame pointers, when a task is interrupted, its stack is no longer
> completely reliable because the function could have been interrupted
> before it had a chance to save the previous frame pointer on the stack.
> So the caller of the interrupted function could get skipped by a stack
> trace.
>
> This is problematic for live patching, which needs to know whether a
> stack trace of a sleeping task can be relied upon.  There's currently no
> way to detect if a sleeping task was interrupted by a page fault
> exception or preemption before it went to sleep.
>
> Another issue is that when dumping the stack of an interrupted task, the
> unwinder has no way of knowing where the saved pt_regs registers are, so
> it can't print them.
>
> This solves those issues by encoding the pt_regs pointer in the frame
> pointer on entry from an interrupt or an exception.
>
> This patch also updates the unwinder to be able to decode it, because
> otherwise the unwinder would be broken by this change.
>
> Note that this causes a change in the behavior of the unwinder: each
> instance of a pt_regs on the stack is now considered a "frame".  So
> callers of unwind_get_return_address() will now get an occasional
> 'regs->ip' address that would have previously been skipped over.

Acked-by: Andy Lutomirski <luto@kernel.org>

with minor optional nitpicks below.

>
> Suggested-by: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/entry/calling.h       | 21 +++++++++++
>  arch/x86/entry/entry_32.S      | 40 ++++++++++++++++++---
>  arch/x86/entry/entry_64.S      | 10 ++++--
>  arch/x86/include/asm/unwind.h  | 18 ++++++++--
>  arch/x86/kernel/unwind_frame.c | 82 +++++++++++++++++++++++++++++++++++++-----
>  5 files changed, 153 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 9a9e588..ab799a3 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -201,6 +201,27 @@ For 32-bit we have the following conventions - kernel is built with
>         .byte 0xf1
>         .endm
>
> +       /*
> +        * This is a sneaky trick to help the unwinder find pt_regs on the
> +        * stack.  The frame pointer is replaced with an encoded pointer to
> +        * pt_regs.  The encoding is just a clearing of the highest-order bit,
> +        * which makes it an invalid address and is also a signal to the
> +        * unwinder that it's a pt_regs pointer in disguise.
> +        *
> +        * NOTE: This macro must be used *after* SAVE_EXTRA_REGS because it
> +        * corrupts the original rbp.
> +        */
> +.macro ENCODE_FRAME_POINTER ptregs_offset=0
> +#ifdef CONFIG_FRAME_POINTER
> +       .if \ptregs_offset
> +               leaq \ptregs_offset(%rsp), %rbp
> +       .else
> +               mov %rsp, %rbp
> +       .endif
> +       btr $63, %rbp
> +#endif
> +.endm
> +
>  #endif /* CONFIG_X86_64 */
>
>  /*
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 4396278..4006fa3 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -174,6 +174,23 @@
>         SET_KERNEL_GS %edx
>  .endm
>
> +/*
> + * This is a sneaky trick to help the unwinder find pt_regs on the
> + * stack.  The frame pointer is replaced with an encoded pointer to
> + * pt_regs.  The encoding is just a clearing of the highest-order bit,
> + * which makes it an invalid address and is also a signal to the
> + * unwinder that it's a pt_regs pointer in disguise.
> + *
> + * NOTE: This macro must be used *after* SAVE_ALL because it corrupts the
> + * original rbp.
> + */
> +.macro ENCODE_FRAME_POINTER
> +#ifdef CONFIG_FRAME_POINTER
> +       mov %esp, %ebp
> +       btr $31, %ebp
> +#endif
> +.endm
> +
>  .macro RESTORE_INT_REGS
>         popl    %ebx
>         popl    %ecx
> @@ -205,10 +222,16 @@
>  .endm
>
>  ENTRY(ret_from_fork)
> +       call    1f

pushl $ret_from_fork is the same length and slightly less strange.
OTOH it forces a relocation, and this function doesn't return, so
there shouldn't be any performance issue, so this may save a byte or
two in the compressed image.

> +1:     push    $0

This could maybe use a comment.

--Andy

[toc] | [prev] | [next] | [standalone]

#1462985 — Re: [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved interrupt registers

On Sun, Aug 14, 2016 at 01:10:42AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > With frame pointers, when a task is interrupted, its stack is no longer
> > completely reliable because the function could have been interrupted
> > before it had a chance to save the previous frame pointer on the stack.
> > So the caller of the interrupted function could get skipped by a stack
> > trace.
> >
> > This is problematic for live patching, which needs to know whether a
> > stack trace of a sleeping task can be relied upon.  There's currently no
> > way to detect if a sleeping task was interrupted by a page fault
> > exception or preemption before it went to sleep.
> >
> > Another issue is that when dumping the stack of an interrupted task, the
> > unwinder has no way of knowing where the saved pt_regs registers are, so
> > it can't print them.
> >
> > This solves those issues by encoding the pt_regs pointer in the frame
> > pointer on entry from an interrupt or an exception.
> >
> > This patch also updates the unwinder to be able to decode it, because
> > otherwise the unwinder would be broken by this change.
> >
> > Note that this causes a change in the behavior of the unwinder: each
> > instance of a pt_regs on the stack is now considered a "frame".  So
> > callers of unwind_get_return_address() will now get an occasional
> > 'regs->ip' address that would have previously been skipped over.
> 
> Acked-by: Andy Lutomirski <luto@kernel.org>
> 
> with minor optional nitpicks below.
> 
> >
> > Suggested-by: Andy Lutomirski <luto@amacapital.net>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  arch/x86/entry/calling.h       | 21 +++++++++++
> >  arch/x86/entry/entry_32.S      | 40 ++++++++++++++++++---
> >  arch/x86/entry/entry_64.S      | 10 ++++--
> >  arch/x86/include/asm/unwind.h  | 18 ++++++++--
> >  arch/x86/kernel/unwind_frame.c | 82 +++++++++++++++++++++++++++++++++++++-----
> >  5 files changed, 153 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> > index 9a9e588..ab799a3 100644
> > --- a/arch/x86/entry/calling.h
> > +++ b/arch/x86/entry/calling.h
> > @@ -201,6 +201,27 @@ For 32-bit we have the following conventions - kernel is built with
> >         .byte 0xf1
> >         .endm
> >
> > +       /*
> > +        * This is a sneaky trick to help the unwinder find pt_regs on the
> > +        * stack.  The frame pointer is replaced with an encoded pointer to
> > +        * pt_regs.  The encoding is just a clearing of the highest-order bit,
> > +        * which makes it an invalid address and is also a signal to the
> > +        * unwinder that it's a pt_regs pointer in disguise.
> > +        *
> > +        * NOTE: This macro must be used *after* SAVE_EXTRA_REGS because it
> > +        * corrupts the original rbp.
> > +        */
> > +.macro ENCODE_FRAME_POINTER ptregs_offset=0
> > +#ifdef CONFIG_FRAME_POINTER
> > +       .if \ptregs_offset
> > +               leaq \ptregs_offset(%rsp), %rbp
> > +       .else
> > +               mov %rsp, %rbp
> > +       .endif
> > +       btr $63, %rbp
> > +#endif
> > +.endm
> > +
> >  #endif /* CONFIG_X86_64 */
> >
> >  /*
> > diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> > index 4396278..4006fa3 100644
> > --- a/arch/x86/entry/entry_32.S
> > +++ b/arch/x86/entry/entry_32.S
> > @@ -174,6 +174,23 @@
> >         SET_KERNEL_GS %edx
> >  .endm
> >
> > +/*
> > + * This is a sneaky trick to help the unwinder find pt_regs on the
> > + * stack.  The frame pointer is replaced with an encoded pointer to
> > + * pt_regs.  The encoding is just a clearing of the highest-order bit,
> > + * which makes it an invalid address and is also a signal to the
> > + * unwinder that it's a pt_regs pointer in disguise.
> > + *
> > + * NOTE: This macro must be used *after* SAVE_ALL because it corrupts the
> > + * original rbp.
> > + */
> > +.macro ENCODE_FRAME_POINTER
> > +#ifdef CONFIG_FRAME_POINTER
> > +       mov %esp, %ebp
> > +       btr $31, %ebp
> > +#endif
> > +.endm
> > +
> >  .macro RESTORE_INT_REGS
> >         popl    %ebx
> >         popl    %ecx
> > @@ -205,10 +222,16 @@
> >  .endm
> >
> >  ENTRY(ret_from_fork)
> > +       call    1f
> 
> pushl $ret_from_fork is the same length and slightly less strange.
> OTOH it forces a relocation, and this function doesn't return, so
> there shouldn't be any performance issue, so this may save a byte or
> two in the compressed image.
> 
> > +1:     push    $0
> 
> This could maybe use a comment.

Oops.  This ret_from_fork bit was meant for a separate patch.

I think the problem with "pushl $ret_from_fork" is that
ret_from_fork+0x0 is not a valid call return address.
printk_stack_address() will show it as the end of the previous function
in the file.

Anyway, this definitely needs a comment and should be split out to a
separate patch.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1461245 — [PATCH v3 37/51] x86/stacktrace: convert save_stack_trace_*() to use the new unwinder

Convert save_stack_trace_*() to use the new unwinder.  dump_trace() has
been deprecated.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/stacktrace.c | 74 +++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 45 deletions(-)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 785aef1..a168e7e 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -8,80 +8,64 @@
 #include <linux/export.h>
 #include <linux/uaccess.h>
 #include <asm/stacktrace.h>
+#include <asm/unwind.h>
 
-static int save_stack_stack(void *data, const char *name)
+static int save_stack_address(struct stack_trace *trace, unsigned long addr,
+			      bool nosched)
 {
-	return 0;
-}
-
-static int
-__save_stack_address(void *data, unsigned long addr, bool reliable, bool nosched)
-{
-	struct stack_trace *trace = data;
-#ifdef CONFIG_FRAME_POINTER
-	if (!reliable)
-		return 0;
-#endif
 	if (nosched && in_sched_functions(addr))
 		return 0;
+
 	if (trace->skip > 0) {
 		trace->skip--;
 		return 0;
 	}
-	if (trace->nr_entries < trace->max_entries) {
-		trace->entries[trace->nr_entries++] = addr;
-		return 0;
-	} else {
-		return -1; /* no more room, stop walking the stack */
-	}
-}
 
-static int save_stack_address(void *data, unsigned long addr, int reliable)
-{
-	return __save_stack_address(data, addr, reliable, false);
+	if (trace->nr_entries >= trace->max_entries)
+		return -1;
+
+	trace->entries[trace->nr_entries++] = addr;
+	return 0;
 }
 
-static int
-save_stack_address_nosched(void *data, unsigned long addr, int reliable)
+static void __save_stack_trace(struct stack_trace *trace,
+			       struct task_struct *task, struct pt_regs *regs,
+			       bool nosched)
 {
-	return __save_stack_address(data, addr, reliable, true);
-}
+	struct unwind_state state;
+	unsigned long addr;
 
-static const struct stacktrace_ops save_stack_ops = {
-	.stack		= save_stack_stack,
-	.address	= save_stack_address,
-	.walk_stack	= print_context_stack,
-};
+	if (regs)
+		save_stack_address(trace, regs->ip, nosched);
 
-static const struct stacktrace_ops save_stack_ops_nosched = {
-	.stack		= save_stack_stack,
-	.address	= save_stack_address_nosched,
-	.walk_stack	= print_context_stack,
-};
+	for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
+	     unwind_next_frame(&state)) {
+		addr = unwind_get_return_address(&state);
+		if (!addr || save_stack_address(trace, addr, nosched))
+			break;
+	}
+
+	if (trace->nr_entries < trace->max_entries)
+		trace->entries[trace->nr_entries++] = ULONG_MAX;
+}
 
 /*
  * Save stack-backtrace addresses into a stack_trace buffer.
  */
 void save_stack_trace(struct stack_trace *trace)
 {
-	dump_trace(current, NULL, NULL, 0, &save_stack_ops, trace);
-	if (trace->nr_entries < trace->max_entries)
-		trace->entries[trace->nr_entries++] = ULONG_MAX;
+	__save_stack_trace(trace, NULL, NULL, false);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace);
 
 void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
 {
-	dump_trace(current, regs, NULL, 0, &save_stack_ops, trace);
-	if (trace->nr_entries < trace->max_entries)
-		trace->entries[trace->nr_entries++] = ULONG_MAX;
+	__save_stack_trace(trace, NULL, regs, false);
 }
 
 void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 {
-	dump_trace(tsk, NULL, NULL, 0, &save_stack_ops_nosched, trace);
-	if (trace->nr_entries < trace->max_entries)
-		trace->entries[trace->nr_entries++] = ULONG_MAX;
+	__save_stack_trace(trace, tsk, NULL, true);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461246 — [PATCH v3 05/51] x86/dumpstack: make printk_stack_address() more generally useful

Change printk_stack_address() to be useful when called by an unwinder
outside the context of dump_trace().

Specifically:

- printk_stack_address()'s 'data' argument is always used as the log
  level string.  Make that explicit.

- Call touch_nmi_watchdog().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 5f49c04..6b3376d 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -26,10 +26,11 @@ int kstack_depth_to_print = 3 * STACKSLOTS_PER_LINE;
 static int die_counter;
 
 static void printk_stack_address(unsigned long address, int reliable,
-		void *data)
+				 char *log_lvl)
 {
+	touch_nmi_watchdog();
 	printk("%s [<%p>] %s%pB\n",
-		(char *)data, (void *)address, reliable ? "" : "? ",
+		log_lvl, (void *)address, reliable ? "" : "? ",
 		(void *)address);
 }
 
@@ -163,7 +164,6 @@ static int print_trace_stack(void *data, char *name)
  */
 static int print_trace_address(void *data, unsigned long addr, int reliable)
 {
-	touch_nmi_watchdog();
 	printk_stack_address(addr, reliable, data);
 	return 0;
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461247 — [PATCH v3 43/51] x86/dumpstack: print stack identifier on its own line

show_trace_log_lvl() prints the stack id (e.g. "<IRQ>") without a
newline so that any stack address printed after it will appear on the
same line.  That causes the first stack address to be vertically
misaligned with the rest, making it visually cluttered and slightly
confusing:

  Call Trace:
   <IRQ> [<ffffffff814431c3>] dump_stack+0x86/0xc3
   [<ffffffff8100828b>] perf_callchain_kernel+0x14b/0x160
   [<ffffffff811e915f>] get_perf_callchain+0x15f/0x2b0
   ...
   <EOI> [<ffffffff8189c6c3>] ? _raw_spin_unlock_irq+0x33/0x60
   [<ffffffff810e1c84>] finish_task_switch+0xb4/0x250
   [<ffffffff8106f7dc>] do_async_page_fault+0x2c/0xa0

It will look worse once we start printing pt_regs registers found in the
middle of the stack:

  <IRQ> RIP: 0010:[<ffffffff8189c6c3>]  [<ffffffff8189c6c3>] _raw_spin_unlock_irq+0x33/0x60
  RSP: 0018:ffff88007876f720  EFLAGS: 00000206
  RAX: ffff8800786caa40 RBX: ffff88007d5da140 RCX: 0000000000000007
  ...

Improve readability by adding a newline to the stack name:

  Call Trace:
   <IRQ>
   [<ffffffff814431c3>] dump_stack+0x86/0xc3
   [<ffffffff8100828b>] perf_callchain_kernel+0x14b/0x160
   [<ffffffff811e915f>] get_perf_callchain+0x15f/0x2b0
   ...
   <EOI>
   [<ffffffff8189c6c3>] ? _raw_spin_unlock_irq+0x33/0x60
   [<ffffffff810e1c84>] finish_task_switch+0xb4/0x250
   [<ffffffff8106f7dc>] do_async_page_fault+0x2c/0xa0

Now that "continued" lines are no longer needed, we can also remove the
hack of using the empty string (aka KERN_CONT) and replace it with
KERN_DEFAULT.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index dcb718b..92a2f82 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -101,7 +101,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
 
 		stack_type_str(stack_info.type, &str_begin, &str_end);
 		if (str_begin)
-			printk("%s <%s> ", log_lvl, str_begin);
+			printk("%s <%s>\n", log_lvl, str_begin);
 
 		/*
 		 * Scan the stack, printing any text addresses we find.  At the
@@ -153,7 +153,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
 		}
 
 		if (str_end)
-			printk("%s <%s> ", log_lvl, str_end);
+			printk("%s <%s>\n", log_lvl, str_end);
 	}
 }
 
@@ -166,12 +166,12 @@ void show_stack(struct task_struct *task, unsigned long *sp)
 	if (!sp && (!task || task == current))
 		sp = get_stack_pointer(current, NULL);
 
-	show_stack_log_lvl(task, NULL, sp, "");
+	show_stack_log_lvl(task, NULL, sp, KERN_DEFAULT);
 }
 
 void show_stack_regs(struct pt_regs *regs)
 {
-	show_stack_log_lvl(NULL, regs, NULL, "");
+	show_stack_log_lvl(NULL, regs, NULL, KERN_DEFAULT);
 }
 
 static arch_spinlock_t die_lock = __ARCH_SPIN_LOCK_UNLOCKED;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461248 — [PATCH v3 24/51] ftrace: only allocate the ret_stack 'fp' field when needed

This saves some memory when HAVE_FUNCTION_GRAPH_FP_TEST isn't defined.
On x86_64 with newer versions of gcc which have -mfentry, it saves 400
bytes per task.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 include/linux/ftrace.h               | 2 ++
 kernel/trace/trace_functions_graph.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7d565af..4ad9ccc 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -795,7 +795,9 @@ struct ftrace_ret_stack {
 	unsigned long func;
 	unsigned long long calltime;
 	unsigned long long subtime;
+#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
 	unsigned long fp;
+#endif
 };
 
 /*
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index fc173cd..0e03ed0 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -171,7 +171,9 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
 	current->ret_stack[index].func = func;
 	current->ret_stack[index].calltime = calltime;
 	current->ret_stack[index].subtime = 0;
+#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
 	current->ret_stack[index].fp = frame_pointer;
+#endif
 	*depth = current->curr_ret_stack;
 
 	return 0;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461250 — [PATCH v3 29/51] x86/dumpstack/ftrace: mark function graph handler function as unreliable

When function graph tracing is enabled for a function, its return
address on the stack is replaced with the address of an ftrace handler
(return_to_handler).

Currently 'return_to_handler' can be reported as reliable.  That's not
ideal, and can actually be misleading.  When saving or dumping the
stack, you normally only care about what led up to that point (the call
path), rather than what will happen in the future (the return path).

That's especially true in the non-oops stack trace case, which isn't
used for debugging.  For example, in a perf profiling operation,
reporting return_to_handler() in the trace would just be confusing.

And in the oops case, where debugging is important, "unreliable" is also
more appropriate there because it serves as a hint that graph tracing
was involved, instead of trying to imply that return_to_handler() was
the real caller.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index b374d85..33f2899 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -88,12 +88,21 @@ print_context_stack(struct task_struct *task,
 				bp = (unsigned long) frame;
 			}
 
-			ops->address(data, addr, reliable);
-
+			/*
+			 * When function graph tracing is enabled for a
+			 * function, its return address on the stack is
+			 * replaced with the address of an ftrace handler
+			 * (return_to_handler).  In that case, before printing
+			 * the "real" address, we want to print the handler
+			 * address as an "unreliable" hint that function graph
+			 * tracing was involved.
+			 */
 			real_addr = ftrace_graph_ret_addr(task, graph, addr,
 							  stack);
 			if (real_addr != addr)
-				ops->address(data, real_addr, 1);
+				ops->address(data, addr, 0);
+
+			ops->address(data, real_addr, reliable);
 		}
 		stack++;
 	}
@@ -117,12 +126,11 @@ print_context_stack_bp(struct task_struct *task,
 		if (!__kernel_text_address(addr))
 			break;
 
-		if (ops->address(data, addr, 1))
-			break;
-
 		real_addr = ftrace_graph_ret_addr(task, graph, addr, retp);
-		if (real_addr != addr)
-			ops->address(data, real_addr, 1);
+		if (real_addr != addr && ops->address(data, addr, 0))
+			break;
+		if (ops->address(data, real_addr, 1))
+			break;
 
 		frame = frame->next_frame;
 		retp = &frame->return_address;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461253 — [PATCH v3 03/51] x86/asm/head: rename 'stack_start' -> 'initial_stack'

The 'stack_start' variable is similar in usage to 'initial_code' and
'initial_gs': they're all stored in head_64.S and they're all updated by
SMP and ACPI suspend before starting a CPU.

Rename it to 'initial_stack' to be consistent with the others.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/realmode.h |  1 +
 arch/x86/include/asm/smp.h      |  3 ---
 arch/x86/kernel/acpi/sleep.c    |  2 +-
 arch/x86/kernel/head_32.S       |  8 ++++----
 arch/x86/kernel/head_64.S       | 11 +++++------
 arch/x86/kernel/smpboot.c       |  2 +-
 6 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index 3327ffb..230e190 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -46,6 +46,7 @@ extern unsigned char real_mode_blob_end[];
 
 extern unsigned long initial_code;
 extern unsigned long initial_gs;
+extern unsigned long initial_stack;
 
 extern unsigned char real_mode_blob[];
 extern unsigned char real_mode_relocs[];
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ebd0c16..19980b3 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -39,9 +39,6 @@ DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_bios_cpu_apicid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(int, x86_cpu_to_logical_apicid);
 #endif
 
-/* Static state in head.S used to set up a CPU */
-extern unsigned long stack_start; /* Initial stack pointer address */
-
 struct task_struct;
 
 struct smp_ops {
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index adb3eaf..4858733 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -99,7 +99,7 @@ int x86_acpi_suspend_lowlevel(void)
 	saved_magic = 0x12345678;
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
-	stack_start = (unsigned long)temp_stack + sizeof(temp_stack);
+	initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
 	early_gdt_descr.address =
 			(unsigned long)get_cpu_gdt_table(smp_processor_id());
 	initial_gs = per_cpu_offset(smp_processor_id());
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 6f8902b..5f40126 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -94,7 +94,7 @@ RESERVE_BRK(pagetables, INIT_MAP_SIZE)
  */
 __HEAD
 ENTRY(startup_32)
-	movl pa(stack_start),%ecx
+	movl pa(initial_stack),%ecx
 	
 	/* test KEEP_SEGMENTS flag to see if the bootloader is asking
 		us to not reload segments */
@@ -286,7 +286,7 @@ num_subarch_entries = (. - subarch_entries) / 4
  * start_secondary().
  */
 ENTRY(start_cpu0)
-	movl stack_start, %ecx
+	movl initial_stack, %ecx
 	movl %ecx, %esp
 	jmp  *(initial_code)
 ENDPROC(start_cpu0)
@@ -307,7 +307,7 @@ ENTRY(startup_32_smp)
 	movl %eax,%es
 	movl %eax,%fs
 	movl %eax,%gs
-	movl pa(stack_start),%ecx
+	movl pa(initial_stack),%ecx
 	movl %eax,%ss
 	leal -__PAGE_OFFSET(%ecx),%esp
 
@@ -703,7 +703,7 @@ ENTRY(initial_page_table)
 
 .data
 .balign 4
-ENTRY(stack_start)
+ENTRY(initial_stack)
 	.long init_thread_union+THREAD_SIZE
 
 __INITRODATA
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 9f8efc9..e048142 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -66,7 +66,7 @@ startup_64:
 	 */
 
 	/*
-	 * Setup stack for verify_cpu(). "-8" because stack_start is defined
+	 * Setup stack for verify_cpu(). "-8" because initial_stack is defined
 	 * this way, see below. Our best guess is a NULL ptr for stack
 	 * termination heuristics and we don't want to break anything which
 	 * might depend on it (kgdb, ...).
@@ -226,7 +226,7 @@ ENTRY(secondary_startup_64)
 	movq	%rax, %cr0
 
 	/* Setup a boot time stack */
-	movq stack_start(%rip), %rsp
+	movq initial_stack(%rip), %rsp
 
 	/* zero EFLAGS after setting rsp */
 	pushq $0
@@ -310,7 +310,7 @@ ENDPROC(secondary_startup_64)
  * start_secondary().
  */
 ENTRY(start_cpu0)
-	movq stack_start(%rip),%rsp
+	movq initial_stack(%rip),%rsp
 	movq	initial_code(%rip),%rax
 	pushq	$0		# fake return address to stop unwinder
 	pushq	$__KERNEL_CS	# set correct cs
@@ -319,15 +319,14 @@ ENTRY(start_cpu0)
 ENDPROC(start_cpu0)
 #endif
 
-	/* SMP bootup changes these two */
+	/* Both SMP bootup and ACPI suspend change these variables */
 	__REFDATA
 	.balign	8
 	GLOBAL(initial_code)
 	.quad	x86_64_start_kernel
 	GLOBAL(initial_gs)
 	.quad	INIT_PER_CPU_VAR(irq_stack_union)
-
-	GLOBAL(stack_start)
+	GLOBAL(initial_stack)
 	.quad  init_thread_union+THREAD_SIZE-8
 	.word  0
 	__FINITDATA
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 067de61..d9d3d67 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -962,7 +962,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 
 	early_gdt_descr.address = (unsigned long)get_cpu_gdt_table(cpu);
 	initial_code = (unsigned long)start_secondary;
-	stack_start  = idle->thread.sp;
+	initial_stack  = idle->thread.sp;
 
 	/*
 	 * Enable the espfix hack for this CPU
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461254 — [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access

On x86_32, when an interrupt happens from kernel space, SS and SP aren't
pushed and the existing stack is used.  So pt_regs is effectively two
words shorter, and the previous stack pointer is normally the memory
after the shortened pt_regs, aka '&regs->sp'.

But in the rare case where the interrupt hits right after the stack
pointer has been changed to point to an empty stack, like for example
when call_on_stack() is used, the address immediately after the
shortened pt_regs is no longer on the stack.  In that case, instead of
'&regs->sp', the previous stack pointer should be retrieved from the
beginning of the current stack page.

kernel_stack_pointer() wants to do that, but it forgets to dereference
the pointer.  So instead of returning a pointer to the previous stack,
it returns a pointer to the beginning of the current stack.

Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/ptrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 2537cfb..5b88a1b 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -173,8 +173,8 @@ unsigned long kernel_stack_pointer(struct pt_regs *regs)
 		return sp;
 
 	prev_esp = (u32 *)(context);
-	if (prev_esp)
-		return (unsigned long)prev_esp;
+	if (*prev_esp)
+		return (unsigned long)*prev_esp;
 
 	return (unsigned long)regs;
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461686 — Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access

On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On x86_32, when an interrupt happens from kernel space, SS and SP aren't
> pushed and the existing stack is used.  So pt_regs is effectively two
> words shorter, and the previous stack pointer is normally the memory
> after the shortened pt_regs, aka '&regs->sp'.
>
> But in the rare case where the interrupt hits right after the stack
> pointer has been changed to point to an empty stack, like for example
> when call_on_stack() is used, the address immediately after the
> shortened pt_regs is no longer on the stack.  In that case, instead of
> '&regs->sp', the previous stack pointer should be retrieved from the
> beginning of the current stack page.
>
> kernel_stack_pointer() wants to do that, but it forgets to dereference
> the pointer.  So instead of returning a pointer to the previous stack,
> it returns a pointer to the beginning of the current stack.
>
> Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure")
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

This seems like a valid fix, but I'm not sure I agree with the intent
of the code.  &regs->sp really is the previous stack pointer in the
sense that the stack pointer was &regs->sp when the entry happened.
From an unwinder's perspective, how is:

movl [whatever], $esp
<-- interrupt

any different from:

movl [whatever], $esp
pushl [something]
<-- interrupt

Also, does x86_32 do this type of stack switching at all?  AFAICS
32-bit kernels don't use IRQ stacks in the first place.  Do they?  Am
I just missing the code that does it?

--Andy

[toc] | [prev] | [next] | [standalone]

#1462118 — Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access

On Sun, Aug 14, 2016 at 3:26 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> On x86_32, when an interrupt happens from kernel space, SS and SP aren't
>> pushed and the existing stack is used.  So pt_regs is effectively two
>> words shorter, and the previous stack pointer is normally the memory
>> after the shortened pt_regs, aka '&regs->sp'.
>>
>> But in the rare case where the interrupt hits right after the stack
>> pointer has been changed to point to an empty stack, like for example
>> when call_on_stack() is used, the address immediately after the
>> shortened pt_regs is no longer on the stack.  In that case, instead of
>> '&regs->sp', the previous stack pointer should be retrieved from the
>> beginning of the current stack page.
>>
>> kernel_stack_pointer() wants to do that, but it forgets to dereference
>> the pointer.  So instead of returning a pointer to the previous stack,
>> it returns a pointer to the beginning of the current stack.
>>
>> Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure")
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>
> This seems like a valid fix, but I'm not sure I agree with the intent
> of the code.  &regs->sp really is the previous stack pointer in the
> sense that the stack pointer was &regs->sp when the entry happened.
> From an unwinder's perspective, how is:
>
> movl [whatever], $esp
> <-- interrupt
>
> any different from:
>
> movl [whatever], $esp
> pushl [something]
> <-- interrupt
>
> Also, does x86_32 do this type of stack switching at all?  AFAICS
> 32-bit kernels don't use IRQ stacks in the first place.  Do they?  Am
> I just missing the code that does it?

32-bit uses a software-based stack switch to run on the IRQ stack.
See execute_on_irq_stack() in irq_32.c.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1462126 — Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access

On Sun, Aug 14, 2016 at 5:55 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Sun, Aug 14, 2016 at 3:26 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>> On x86_32, when an interrupt happens from kernel space, SS and SP aren't
>>> pushed and the existing stack is used.  So pt_regs is effectively two
>>> words shorter, and the previous stack pointer is normally the memory
>>> after the shortened pt_regs, aka '&regs->sp'.
>>>
>>> But in the rare case where the interrupt hits right after the stack
>>> pointer has been changed to point to an empty stack, like for example
>>> when call_on_stack() is used, the address immediately after the
>>> shortened pt_regs is no longer on the stack.  In that case, instead of
>>> '&regs->sp', the previous stack pointer should be retrieved from the
>>> beginning of the current stack page.
>>>
>>> kernel_stack_pointer() wants to do that, but it forgets to dereference
>>> the pointer.  So instead of returning a pointer to the previous stack,
>>> it returns a pointer to the beginning of the current stack.
>>>
>>> Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure")
>>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>
>> This seems like a valid fix, but I'm not sure I agree with the intent
>> of the code.  &regs->sp really is the previous stack pointer in the
>> sense that the stack pointer was &regs->sp when the entry happened.
>> From an unwinder's perspective, how is:
>>
>> movl [whatever], $esp
>> <-- interrupt
>>
>> any different from:
>>
>> movl [whatever], $esp
>> pushl [something]
>> <-- interrupt
>>
>> Also, does x86_32 do this type of stack switching at all?  AFAICS
>> 32-bit kernels don't use IRQ stacks in the first place.  Do they?  Am
>> I just missing the code that does it?
>
> 32-bit uses a software-based stack switch to run on the IRQ stack.
> See execute_on_irq_stack() in irq_32.c.
>

Indeed, thanks.

I'm still not convinced that kernel_stack_pojnter() needs to handle this.

[toc] | [prev] | [next] | [standalone]

#1462869 — Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access

On Sun, Aug 14, 2016 at 12:26:29AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On x86_32, when an interrupt happens from kernel space, SS and SP aren't
> > pushed and the existing stack is used.  So pt_regs is effectively two
> > words shorter, and the previous stack pointer is normally the memory
> > after the shortened pt_regs, aka '&regs->sp'.
> >
> > But in the rare case where the interrupt hits right after the stack
> > pointer has been changed to point to an empty stack, like for example
> > when call_on_stack() is used, the address immediately after the
> > shortened pt_regs is no longer on the stack.  In that case, instead of
> > '&regs->sp', the previous stack pointer should be retrieved from the
> > beginning of the current stack page.
> >
> > kernel_stack_pointer() wants to do that, but it forgets to dereference
> > the pointer.  So instead of returning a pointer to the previous stack,
> > it returns a pointer to the beginning of the current stack.
> >
> > Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure")
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> This seems like a valid fix, but I'm not sure I agree with the intent
> of the code.  &regs->sp really is the previous stack pointer in the
> sense that the stack pointer was &regs->sp when the entry happened.
> From an unwinder's perspective, how is:
> 
> movl [whatever], $esp
> <-- interrupt
> 
> any different from:
> 
> movl [whatever], $esp
> pushl [something]
> <-- interrupt

In the first case, the stack is empty, so reading the value pointed to
by %esp would result in accessing outside the bounds of the stack.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1463013 — Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access

On Mon, Aug 15, 2016 at 10:05:58AM -0500, Josh Poimboeuf wrote:
> On Sun, Aug 14, 2016 at 12:26:29AM -0700, Andy Lutomirski wrote:
> > On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > On x86_32, when an interrupt happens from kernel space, SS and SP aren't
> > > pushed and the existing stack is used.  So pt_regs is effectively two
> > > words shorter, and the previous stack pointer is normally the memory
> > > after the shortened pt_regs, aka '&regs->sp'.
> > >
> > > But in the rare case where the interrupt hits right after the stack
> > > pointer has been changed to point to an empty stack, like for example
> > > when call_on_stack() is used, the address immediately after the
> > > shortened pt_regs is no longer on the stack.  In that case, instead of
> > > '&regs->sp', the previous stack pointer should be retrieved from the
> > > beginning of the current stack page.
> > >
> > > kernel_stack_pointer() wants to do that, but it forgets to dereference
> > > the pointer.  So instead of returning a pointer to the previous stack,
> > > it returns a pointer to the beginning of the current stack.
> > >
> > > Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure")
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> > This seems like a valid fix, but I'm not sure I agree with the intent
> > of the code.  &regs->sp really is the previous stack pointer in the
> > sense that the stack pointer was &regs->sp when the entry happened.
> > From an unwinder's perspective, how is:
> > 
> > movl [whatever], $esp
> > <-- interrupt
> > 
> > any different from:
> > 
> > movl [whatever], $esp
> > pushl [something]
> > <-- interrupt
> 
> In the first case, the stack is empty, so reading the value pointed to
> by %esp would result in accessing outside the bounds of the stack.

...but maybe your point is that following the previous stack pointer is
outside the scope of kernel_stack_pointer() and should instead be done
by its caller.  Especially considering the fact that the x86_64 version
of this function doesn't have this "feature".  In which case I think I
would agree.

However I think fixing that is outside the scope of this
already-way-too-big patch set.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1463147 — Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access

On Mon, Aug 15, 2016 at 10:22 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, Aug 15, 2016 at 10:05:58AM -0500, Josh Poimboeuf wrote:
>> On Sun, Aug 14, 2016 at 12:26:29AM -0700, Andy Lutomirski wrote:
>> > On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > > On x86_32, when an interrupt happens from kernel space, SS and SP aren't
>> > > pushed and the existing stack is used.  So pt_regs is effectively two
>> > > words shorter, and the previous stack pointer is normally the memory
>> > > after the shortened pt_regs, aka '&regs->sp'.
>> > >
>> > > But in the rare case where the interrupt hits right after the stack
>> > > pointer has been changed to point to an empty stack, like for example
>> > > when call_on_stack() is used, the address immediately after the
>> > > shortened pt_regs is no longer on the stack.  In that case, instead of
>> > > '&regs->sp', the previous stack pointer should be retrieved from the
>> > > beginning of the current stack page.
>> > >
>> > > kernel_stack_pointer() wants to do that, but it forgets to dereference
>> > > the pointer.  So instead of returning a pointer to the previous stack,
>> > > it returns a pointer to the beginning of the current stack.
>> > >
>> > > Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure")
>> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> >
>> > This seems like a valid fix, but I'm not sure I agree with the intent
>> > of the code.  &regs->sp really is the previous stack pointer in the
>> > sense that the stack pointer was &regs->sp when the entry happened.
>> > From an unwinder's perspective, how is:
>> >
>> > movl [whatever], $esp
>> > <-- interrupt
>> >
>> > any different from:
>> >
>> > movl [whatever], $esp
>> > pushl [something]
>> > <-- interrupt
>>
>> In the first case, the stack is empty, so reading the value pointed to
>> by %esp would result in accessing outside the bounds of the stack.
>
> ...but maybe your point is that following the previous stack pointer is
> outside the scope of kernel_stack_pointer() and should instead be done
> by its caller.  Especially considering the fact that the x86_64 version
> of this function doesn't have this "feature".  In which case I think I
> would agree.

Yes, especially since your code seems to know how to find the previous
stack already.

>
> However I think fixing that is outside the scope of this
> already-way-too-big patch set.

Agreed.

>
> --
> Josh



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1461256 — [PATCH v3 13/51] x86/asm/head: remove useless zeroed word

This zeroed word has no apparent purpose, so remove it.

Brian Gerst says:

  "FYI the word used to be the SS segment selector for the LSS
  instruction, which isn't needed in 64-bit mode."

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/head_64.S | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index a212310..3621ad2 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -328,7 +328,6 @@ ENDPROC(start_cpu0)
 	.quad	INIT_PER_CPU_VAR(irq_stack_union)
 	GLOBAL(initial_stack)
 	.quad  init_thread_union+THREAD_SIZE-8
-	.word  0
 	__FINITDATA
 
 bad_address:
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461257 — [PATCH v3 21/51] oprofile/x86: add regs->ip to oprofile trace

dump_trace() doesn't add the interrupted instruction's address to the
trace, so add it manually.

Cc: Robert Richter <rric@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/oprofile/backtrace.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c
index c594768..d950f9e 100644
--- a/arch/x86/oprofile/backtrace.c
+++ b/arch/x86/oprofile/backtrace.c
@@ -113,8 +113,14 @@ x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 	struct stack_frame *head = (struct stack_frame *)frame_pointer(regs);
 
 	if (!user_mode(regs)) {
-		if (depth)
-			dump_trace(NULL, regs, NULL, 0, &backtrace_ops, &depth);
+		if (!depth)
+			return;
+
+		oprofile_add_trace(regs->ip);
+		if (!--depth)
+			return;
+
+		dump_trace(NULL, regs, NULL, 0, &backtrace_ops, &depth);
 		return;
 	}
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461258 — [PATCH v3 10/51] x86/dumpstack: add get_stack_pointer() and get_frame_pointer()

The various functions involved in dumping the stack all do similar
things with regard to getting the stack pointer and the frame pointer
based on the regs and task arguments.  Create helper functions to
do that instead.

Reviewed-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/stacktrace.h | 39 ++++++++++++++++++++++-----------------
 arch/x86/kernel/dumpstack.c       |  5 ++---
 arch/x86/kernel/dumpstack_32.c    | 25 ++++---------------------
 arch/x86/kernel/dumpstack_64.c    | 30 ++++--------------------------
 4 files changed, 32 insertions(+), 67 deletions(-)

diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index 0944218..6f65995 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -49,37 +49,42 @@ void dump_trace(struct task_struct *tsk, struct pt_regs *regs,
 
 #ifdef CONFIG_X86_32
 #define STACKSLOTS_PER_LINE 8
-#define get_bp(bp) asm("movl %%ebp, %0" : "=r" (bp) :)
 #else
 #define STACKSLOTS_PER_LINE 4
-#define get_bp(bp) asm("movq %%rbp, %0" : "=r" (bp) :)
 #endif
 
 #ifdef CONFIG_FRAME_POINTER
-static inline unsigned long
-stack_frame(struct task_struct *task, struct pt_regs *regs)
+static inline unsigned long *
+get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
 {
-	unsigned long bp;
-
 	if (regs)
-		return regs->bp;
+		return (unsigned long *)regs->bp;
 
-	if (task == current) {
-		/* Grab bp right from our regs */
-		get_bp(bp);
-		return bp;
-	}
+	if (!task || task == current)
+		return __builtin_frame_address(0);
 
 	/* bp is the last reg pushed by switch_to */
-	return *(unsigned long *)task->thread.sp;
+	return (unsigned long *)*(unsigned long *)task->thread.sp;
 }
 #else
-static inline unsigned long
-stack_frame(struct task_struct *task, struct pt_regs *regs)
+static inline unsigned long *
+get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
 {
 	return 0;
 }
-#endif
+#endif /* CONFIG_FRAME_POINTER */
+
+static inline unsigned long *
+get_stack_pointer(struct task_struct *task, struct pt_regs *regs)
+{
+	if (regs)
+		return (unsigned long *)kernel_stack_pointer(regs);
+
+	if (!task || task == current)
+		return __builtin_frame_address(0);
+
+	return (unsigned long *)task->thread.sp;
+}
 
 extern void
 show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
@@ -106,7 +111,7 @@ static inline unsigned long caller_frame_pointer(void)
 {
 	struct stack_frame *frame;
 
-	get_bp(frame);
+	frame = __builtin_frame_address(0);
 
 #ifdef CONFIG_FRAME_POINTER
 	frame = frame->next_frame;
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 6b3376d..68f42bb 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -185,15 +185,14 @@ show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
 void show_stack(struct task_struct *task, unsigned long *sp)
 {
 	unsigned long bp = 0;
-	unsigned long stack;
 
 	/*
 	 * Stack frames below this one aren't interesting.  Don't show them
 	 * if we're printing for %current.
 	 */
 	if (!sp && (!task || task == current)) {
-		sp = &stack;
-		bp = stack_frame(current, NULL);
+		sp = get_stack_pointer(current, NULL);
+		bp = (unsigned long)get_frame_pointer(current, NULL);
 	}
 
 	show_stack_log_lvl(task, NULL, sp, bp, "");
diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 0967571..358fe1c 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -46,19 +46,9 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 	int graph = 0;
 	u32 *prev_esp;
 
-	if (!task)
-		task = current;
-
-	if (!stack) {
-		unsigned long dummy;
-
-		stack = &dummy;
-		if (task != current)
-			stack = (unsigned long *)task->thread.sp;
-	}
-
-	if (!bp)
-		bp = stack_frame(task, regs);
+	task = task ? : current;
+	stack = stack ? : get_stack_pointer(task, regs);
+	bp = bp ? : (unsigned long)get_frame_pointer(task, regs);
 
 	for (;;) {
 		void *end_stack;
@@ -95,14 +85,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	unsigned long *stack;
 	int i;
 
-	if (sp == NULL) {
-		if (regs)
-			sp = (unsigned long *)regs->sp;
-		else if (task)
-			sp = (unsigned long *)task->thread.sp;
-		else
-			sp = (unsigned long *)&sp;
-	}
+	sp = sp ? : get_stack_pointer(task, regs);
 
 	stack = sp;
 	for (i = 0; i < kstack_depth_to_print; i++) {
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 0fdd371..3c5dbc0 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -151,25 +151,14 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 {
 	const unsigned cpu = get_cpu();
 	unsigned long *irq_stack = (unsigned long *)per_cpu(irq_stack_ptr, cpu);
-	unsigned long dummy;
 	unsigned used = 0;
 	int graph = 0;
 	int done = 0;
 
-	if (!task)
-		task = current;
+	task = task ? : current;
+	stack = stack ? : get_stack_pointer(task, regs);
+	bp = bp ? : (unsigned long)get_frame_pointer(task, regs);
 
-	if (!stack) {
-		if (regs)
-			stack = (unsigned long *)regs->sp;
-		else if (task != current)
-			stack = (unsigned long *)task->thread.sp;
-		else
-			stack = &dummy;
-	}
-
-	if (!bp)
-		bp = stack_frame(task, regs);
 	/*
 	 * Print function call entries in all stacks, starting at the
 	 * current stack address. If the stacks consist of nested
@@ -256,18 +245,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	irq_stack_end = (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
 	irq_stack     = irq_stack_end - (IRQ_USABLE_STACK_SIZE / sizeof(long));
 
-	/*
-	 * Debugging aid: "show_stack(NULL, NULL);" prints the
-	 * back trace for this cpu:
-	 */
-	if (sp == NULL) {
-		if (regs)
-			sp = (unsigned long *)regs->sp;
-		else if (task)
-			sp = (unsigned long *)task->thread.sp;
-		else
-			sp = (unsigned long *)&sp;
-	}
+	sp = sp ? : get_stack_pointer(task, regs);
 
 	stack = sp;
 	for (i = 0; i < kstack_depth_to_print; i++) {
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1461259 — [PATCH v3 07/51] x86/dumpstack: remove extra brackets around "<EOE>"

When starting the dump of an exception stack, it shows "<<EOE>>" instead
of "<EOE>".  print_trace_stack() already adds brackets, no need to add
them again.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 43023ae..7ea6ed0 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -199,7 +199,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 
 			bp = ops->walk_stack(task, stack, bp, ops,
 					     data, stack_end, &graph);
-			ops->stack(data, "<EOE>");
+			ops->stack(data, "EOE");
 			/*
 			 * We link to the next stack via the
 			 * second-to-last pointer (index -2 to end) in the
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

Back to top | Article view | linux.kernel

csiph-web