Skip to contentSkip to conversation

x86/unwind: an unwinder fix and debugging improvement

PATCH New Josh Poimboeuf 2 participants 5 messages Dec 16, 2016 – Dec 19, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover x86/unwind: an unwinder fix and debugging improvement

Patch 1 is a fix for an unwinder issue found by Boris. Patch 2 is a debugging improvement which helped diagnose the problem.

Patch 1 needs to go into tip/urgent for 4.10. I'd argue that patch 2 should also go into urgent because it only affects the error path and it makes the new unwinder warnings much more useful.

Josh Poimboeuf (2):
x86/unwind: adjust last frame check for aligned function stacks x86/unwind: dump stack data on warnings

 arch/x86/include/asm/unwind.h  |  2 +-
 arch/x86/kernel/unwind_frame.c | 47 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 2 deletions(-)
Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/2 x86/unwind: adjust last frame check for aligned function stacks

Somehow, CONFIG_PARAVIRT=n convinces gcc to change the x86_64_start_kernel() prologue from:

0000000000000129 <x86_64_start_kernel>:

   129:	55                   	push   %rbp
   12a:	48 89 e5             	mov    %rsp,%rbp

to:

0000000000000124 <x86_64_start_kernel>:

   124:	4c 8d 54 24 08       	lea    0x8(%rsp),%r10
   129:	48 83 e4 f0          	and    $0xfffffffffffffff0,%rsp
   12d:	41 ff 72 f8          	pushq  -0x8(%r10)
   131:	55                   	push   %rbp
   132:	48 89 e5             	mov    %rsp,%rbp

This is an unusual pattern which aligns rsp (though in this case it's already aligned) and saves the start_cpu() return address again on the stack before storing the frame pointer.

The unwinder assumes the last stack frame header is at a certain offset, but the above code breaks that assumption, resulting in the following warning:

WARNING: kernel stack frame pointer at ffffffff82e03f40 in swapper:0 has bad value (null)

Fix it by checking for the last task stack frame at the aligned offset in addition to the normal unaligned offset.

Fixes acb4608ad186 ("x86/unwind: Create stack frames for saved syscall registers")
---
 arch/x86/kernel/unwind_frame.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/x86/kernel/unwind_frame.c +8 -1
index ea7b7f9..33aeaae 100644
@@ -46,7 +46,14 @@ static bool is_last_task_frame(struct unwind_state *state)
 	unsigned long bp = (unsigned long)state->bp;
 	unsigned long regs = (unsigned long)task_pt_regs(state->task);
 
-	return bp == regs - FRAME_HEADER_SIZE;
+	/*
+	 * We have to check for the last task frame at two different locations
+	 * because gcc can occasionally decide to realign the stack pointer and
+	 * change the offset of the stack frame by a word in the prologue of a
+	 * function called by head/entry code.
+	 */
+	return bp == regs - FRAME_HEADER_SIZE ||
+	       bp == regs - FRAME_HEADER_SIZE - sizeof(long);
 }
 
 /*
Signature · 1 line
2.7.4
#2 lore raw

[tip:x86/urgent] x86/unwind: Adjust last frame check for aligned function stacks

x86/unwind: Adjust last frame check for aligned function stacks
Commit-ID:  8023e0e2a48d45e8d5363081fad9f7ed4402f953
Gitweb:     http://git.kernel.org/tip/8023e0e2a48d45e8d5363081fad9f7ed4402f953
Author:     Josh Poimboeuf <[email protected]>
AuthorDate: Fri, 16 Dec 2016 10:05:05 -0600
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 19 Dec 2016 11:47:05 +0100

x86/unwind: Adjust last frame check for aligned function stacks

Somehow, CONFIG_PARAVIRT=n convinces gcc to change the x86_64_start_kernel() prologue from:

0000000000000129 <x86_64_start_kernel>:

   129:	55                   	push   %rbp
   12a:	48 89 e5             	mov    %rsp,%rbp

to:

0000000000000124 <x86_64_start_kernel>:

   124:	4c 8d 54 24 08       	lea    0x8(%rsp),%r10
   129:	48 83 e4 f0          	and    $0xfffffffffffffff0,%rsp
   12d:	41 ff 72 f8          	pushq  -0x8(%r10)
   131:	55                   	push   %rbp
   132:	48 89 e5             	mov    %rsp,%rbp

This is an unusual pattern which aligns rsp (though in this case it's already aligned) and saves the start_cpu() return address again on the stack before storing the frame pointer.

The unwinder assumes the last stack frame header is at a certain offset, but the above code breaks that assumption, resulting in the following warning:

WARNING: kernel stack frame pointer at ffffffff82e03f40 in swapper:0 has bad value (null)

Fix it by checking for the last task stack frame at the aligned offset in addition to the normal unaligned offset.

---
 arch/x86/kernel/unwind_frame.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/x86/kernel/unwind_frame.c +8 -1
index ea7b7f9..33aeaae 100644
@@ -46,7 +46,14 @@ static bool is_last_task_frame(struct unwind_state *state)
 	unsigned long bp = (unsigned long)state->bp;
 	unsigned long regs = (unsigned long)task_pt_regs(state->task);
 
-	return bp == regs - FRAME_HEADER_SIZE;
+	/*
+	 * We have to check for the last task frame at two different locations
+	 * because gcc can occasionally decide to realign the stack pointer and
+	 * change the offset of the stack frame by a word in the prologue of a
+	 * function called by head/entry code.
+	 */
+	return bp == regs - FRAME_HEADER_SIZE ||
+	       bp == regs - FRAME_HEADER_SIZE - sizeof(long);
 }
 
 /*
#3 lore raw

PATCH 2/2 x86/unwind: dump stack data on warnings

In reply to Josh Poimboeuf

The unwinder warnings are good at finding unexpected unwinder issues, but they often don't give enough data to be able to fully diagnose them. Print a one-time stack dump when a warning is detected.

---
 arch/x86/include/asm/unwind.h  |  2 +-
 arch/x86/kernel/unwind_frame.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
arch/x86/include/asm/unwind.h +1 -1
index c5a7f3a..6fa75b1 100644
@@ -12,7 +12,7 @@ struct unwind_state {
 	struct task_struct *task;
 	int graph_idx;
 #ifdef CONFIG_FRAME_POINTER
-	unsigned long *bp;
+	unsigned long *bp, *orig_sp;
 	struct pt_regs *regs;
 #else
 	unsigned long *sp;
arch/x86/kernel/unwind_frame.c +38 -0
index 33aeaae..20d4b4e 100644
@@ -6,6 +6,37 @@
 
 #define FRAME_HEADER_SIZE (sizeof(long) * 2)
 
+static void unwind_dump(struct unwind_state *state, unsigned long *sp)
+{
+	static bool dumped_before = false;
+	bool prev_zero, zero = false;
+	unsigned long word;
+
+	if (dumped_before)
+		return;
+
+	dumped_before = true;
+
+	printk_deferred("unwind stack type:%d next_sp:%p mask:%lx graph_idx:%d\n",
+			state->stack_info.type, state->stack_info.next_sp,
+			state->stack_mask, state->graph_idx);
+
+	for (sp = state->orig_sp; sp < state->stack_info.end; sp++) {
+		word = READ_ONCE_NOCHECK(*sp);
+
+		prev_zero = zero;
+		zero = word == 0;
+
+		if (zero) {
+			if (!prev_zero)
+				printk_deferred("%p: %016x ...\n", sp, 0);
+			continue;
+		}
+
+		printk_deferred("%p: %016lx (%pB)\n", sp, word, (void *)word);
+	}
+}
+
 unsigned long unwind_get_return_address(struct unwind_state *state)
 {
 	unsigned long addr;
@@ -25,6 +56,7 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 			"WARNING: unrecognized kernel stack return address %p at %p in %s:%d\n",
 			(void *)addr, addr_p, state->task->comm,
 			state->task->pid);
+		unwind_dump(state, addr_p);
 		return 0;
 	}
 
@@ -74,6 +106,7 @@ static bool update_stack_state(struct unwind_state *state, void *addr,
 			       size_t len)
 {
 	struct stack_info *info = &state->stack_info;
+	enum stack_type orig_type = info->type;
 
 	/*
 	 * If addr isn't on the current stack, switch to the next one.
@@ -87,6 +120,9 @@ static bool update_stack_state(struct unwind_state *state, void *addr,
 				   &state->stack_mask))
 			return false;
 
+	if (!state->orig_sp || info->type != orig_type)
+		state->orig_sp = addr;
+
 	return true;
 }
 
@@ -185,11 +221,13 @@ bool unwind_next_frame(struct unwind_state *state)
 			"WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n",
 			state->regs, state->task->comm,
 			state->task->pid, next_frame);
+		unwind_dump(state, (unsigned long *)state->regs);
 	} else {
 		printk_deferred_once(KERN_WARNING
 			"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
 			state->bp, state->task->comm,
 			state->task->pid, next_frame);
+		unwind_dump(state, state->bp);
 	}
 the_end:
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
Signature · 1 line
2.7.4
#4 lore raw

[tip:x86/urgent] x86/unwind: Dump stack data on warnings

x86/unwind: Dump stack data on warnings
Commit-ID:  8b5e99f02264130782a10ba5c0c759797fb064ee
Gitweb:     http://git.kernel.org/tip/8b5e99f02264130782a10ba5c0c759797fb064ee
Author:     Josh Poimboeuf <[email protected]>
AuthorDate: Fri, 16 Dec 2016 10:05:06 -0600
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 19 Dec 2016 11:47:05 +0100

x86/unwind: Dump stack data on warnings

The unwinder warnings are good at finding unexpected unwinder issues, but they often don't give enough data to be able to fully diagnose them. Print a one-time stack dump when a warning is detected.

---
 arch/x86/include/asm/unwind.h  |  2 +-
 arch/x86/kernel/unwind_frame.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
arch/x86/include/asm/unwind.h +1 -1
index c5a7f3a..6fa75b1 100644
@@ -12,7 +12,7 @@ struct unwind_state {
 	struct task_struct *task;
 	int graph_idx;
 #ifdef CONFIG_FRAME_POINTER
-	unsigned long *bp;
+	unsigned long *bp, *orig_sp;
 	struct pt_regs *regs;
 #else
 	unsigned long *sp;
arch/x86/kernel/unwind_frame.c +38 -0
index 33aeaae..20d4b4e 100644
@@ -6,6 +6,37 @@
 
 #define FRAME_HEADER_SIZE (sizeof(long) * 2)
 
+static void unwind_dump(struct unwind_state *state, unsigned long *sp)
+{
+	static bool dumped_before = false;
+	bool prev_zero, zero = false;
+	unsigned long word;
+
+	if (dumped_before)
+		return;
+
+	dumped_before = true;
+
+	printk_deferred("unwind stack type:%d next_sp:%p mask:%lx graph_idx:%d\n",
+			state->stack_info.type, state->stack_info.next_sp,
+			state->stack_mask, state->graph_idx);
+
+	for (sp = state->orig_sp; sp < state->stack_info.end; sp++) {
+		word = READ_ONCE_NOCHECK(*sp);
+
+		prev_zero = zero;
+		zero = word == 0;
+
+		if (zero) {
+			if (!prev_zero)
+				printk_deferred("%p: %016x ...\n", sp, 0);
+			continue;
+		}
+
+		printk_deferred("%p: %016lx (%pB)\n", sp, word, (void *)word);
+	}
+}
+
 unsigned long unwind_get_return_address(struct unwind_state *state)
 {
 	unsigned long addr;
@@ -25,6 +56,7 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 			"WARNING: unrecognized kernel stack return address %p at %p in %s:%d\n",
 			(void *)addr, addr_p, state->task->comm,
 			state->task->pid);
+		unwind_dump(state, addr_p);
 		return 0;
 	}
 
@@ -74,6 +106,7 @@ static bool update_stack_state(struct unwind_state *state, void *addr,
 			       size_t len)
 {
 	struct stack_info *info = &state->stack_info;
+	enum stack_type orig_type = info->type;
 
 	/*
 	 * If addr isn't on the current stack, switch to the next one.
@@ -87,6 +120,9 @@ static bool update_stack_state(struct unwind_state *state, void *addr,
 				   &state->stack_mask))
 			return false;
 
+	if (!state->orig_sp || info->type != orig_type)
+		state->orig_sp = addr;
+
 	return true;
 }
 
@@ -185,11 +221,13 @@ bad_address:
 			"WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n",
 			state->regs, state->task->comm,
 			state->task->pid, next_frame);
+		unwind_dump(state, (unsigned long *)state->regs);
 	} else {
 		printk_deferred_once(KERN_WARNING
 			"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
 			state->bp, state->task->comm,
 			state->task->pid, next_frame);
+		unwind_dump(state, state->bp);
 	}
 the_end:
 	state->stack_info.type = STACK_TYPE_UNKNOWN;