* [PATCH urgent 0/2] x86/unwind: an unwinder fix and debugging improvement @ 2016-12-16 16:05 Josh Poimboeuf 2016-12-16 16:05 ` [PATCH urgent 1/2] x86/unwind: adjust last frame check for aligned function stacks Josh Poimboeuf 2016-12-16 16:05 ` [PATCH urgent 2/2] x86/unwind: dump stack data on warnings Josh Poimboeuf 0 siblings, 2 replies; 5+ messages in thread From: Josh Poimboeuf @ 2016-12-16 16:05 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Borislav Petkov, Andy Lutomirski 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(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH urgent 1/2] x86/unwind: adjust last frame check for aligned function stacks 2016-12-16 16:05 [PATCH urgent 0/2] x86/unwind: an unwinder fix and debugging improvement Josh Poimboeuf @ 2016-12-16 16:05 ` Josh Poimboeuf 2016-12-19 10:54 ` [tip:x86/urgent] x86/unwind: Adjust " tip-bot for Josh Poimboeuf 2016-12-16 16:05 ` [PATCH urgent 2/2] x86/unwind: dump stack data on warnings Josh Poimboeuf 1 sibling, 1 reply; 5+ messages in thread From: Josh Poimboeuf @ 2016-12-16 16:05 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Borislav Petkov, Andy Lutomirski 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") Reported-by: Borislav Petkov <bp@alien8.de> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- arch/x86/kernel/unwind_frame.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c index ea7b7f9..33aeaae 100644 --- a/arch/x86/kernel/unwind_frame.c +++ b/arch/x86/kernel/unwind_frame.c @@ -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); } /* -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [tip:x86/urgent] x86/unwind: Adjust last frame check for aligned function stacks 2016-12-16 16:05 ` [PATCH urgent 1/2] x86/unwind: adjust last frame check for aligned function stacks Josh Poimboeuf @ 2016-12-19 10:54 ` tip-bot for Josh Poimboeuf 0 siblings, 0 replies; 5+ messages in thread From: tip-bot for Josh Poimboeuf @ 2016-12-19 10:54 UTC (permalink / raw) To: linux-tip-commits; +Cc: tglx, luto, jpoimboe, bp, hpa, mingo, linux-kernel Commit-ID: 8023e0e2a48d45e8d5363081fad9f7ed4402f953 Gitweb: http://git.kernel.org/tip/8023e0e2a48d45e8d5363081fad9f7ed4402f953 Author: Josh Poimboeuf <jpoimboe@redhat.com> AuthorDate: Fri, 16 Dec 2016 10:05:05 -0600 Committer: Thomas Gleixner <tglx@linutronix.de> 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. Fixes: acb4608ad186 ("x86/unwind: Create stack frames for saved syscall registers") Reported-by: Borislav Petkov <bp@alien8.de> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> Cc: Andy Lutomirski <luto@amacapital.net> Link: http://lkml.kernel.org/r/9d7b4eb8cf55a7d6002cb738f25c23e7429c99a0.1481904011.git.jpoimboe@redhat.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/unwind_frame.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c index ea7b7f9..33aeaae 100644 --- a/arch/x86/kernel/unwind_frame.c +++ b/arch/x86/kernel/unwind_frame.c @@ -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); } /* ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH urgent 2/2] x86/unwind: dump stack data on warnings 2016-12-16 16:05 [PATCH urgent 0/2] x86/unwind: an unwinder fix and debugging improvement Josh Poimboeuf 2016-12-16 16:05 ` [PATCH urgent 1/2] x86/unwind: adjust last frame check for aligned function stacks Josh Poimboeuf @ 2016-12-16 16:05 ` Josh Poimboeuf 2016-12-19 10:55 ` [tip:x86/urgent] x86/unwind: Dump " tip-bot for Josh Poimboeuf 1 sibling, 1 reply; 5+ messages in thread From: Josh Poimboeuf @ 2016-12-16 16:05 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Borislav Petkov, Andy Lutomirski 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. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- arch/x86/include/asm/unwind.h | 2 +- arch/x86/kernel/unwind_frame.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h index c5a7f3a..6fa75b1 100644 --- a/arch/x86/include/asm/unwind.h +++ b/arch/x86/include/asm/unwind.h @@ -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; diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c index 33aeaae..20d4b4e 100644 --- a/arch/x86/kernel/unwind_frame.c +++ b/arch/x86/kernel/unwind_frame.c @@ -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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [tip:x86/urgent] x86/unwind: Dump stack data on warnings 2016-12-16 16:05 ` [PATCH urgent 2/2] x86/unwind: dump stack data on warnings Josh Poimboeuf @ 2016-12-19 10:55 ` tip-bot for Josh Poimboeuf 0 siblings, 0 replies; 5+ messages in thread From: tip-bot for Josh Poimboeuf @ 2016-12-19 10:55 UTC (permalink / raw) To: linux-tip-commits; +Cc: hpa, jpoimboe, bp, linux-kernel, mingo, tglx, luto Commit-ID: 8b5e99f02264130782a10ba5c0c759797fb064ee Gitweb: http://git.kernel.org/tip/8b5e99f02264130782a10ba5c0c759797fb064ee Author: Josh Poimboeuf <jpoimboe@redhat.com> AuthorDate: Fri, 16 Dec 2016 10:05:06 -0600 Committer: Thomas Gleixner <tglx@linutronix.de> 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. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Andy Lutomirski <luto@amacapital.net> Link: http://lkml.kernel.org/r/15607370e3ddb1732b6a73d5c65937864df16ac8.1481904011.git.jpoimboe@redhat.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/include/asm/unwind.h | 2 +- arch/x86/kernel/unwind_frame.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h index c5a7f3a..6fa75b1 100644 --- a/arch/x86/include/asm/unwind.h +++ b/arch/x86/include/asm/unwind.h @@ -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; diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c index 33aeaae..20d4b4e 100644 --- a/arch/x86/kernel/unwind_frame.c +++ b/arch/x86/kernel/unwind_frame.c @@ -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; ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-19 10:56 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-16 16:05 [PATCH urgent 0/2] x86/unwind: an unwinder fix and debugging improvement Josh Poimboeuf 2016-12-16 16:05 ` [PATCH urgent 1/2] x86/unwind: adjust last frame check for aligned function stacks Josh Poimboeuf 2016-12-19 10:54 ` [tip:x86/urgent] x86/unwind: Adjust " tip-bot for Josh Poimboeuf 2016-12-16 16:05 ` [PATCH urgent 2/2] x86/unwind: dump stack data on warnings Josh Poimboeuf 2016-12-19 10:55 ` [tip:x86/urgent] x86/unwind: Dump " tip-bot for Josh Poimboeuf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).