Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1461224 > unrolled thread
| Started by | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| First post | 2016-08-12 16:40 +0200 |
| Last post | 2016-08-12 17:10 +0200 |
| Articles | 20 on this page of 69 — 6 participants |
Back to article view | Back to linux.kernel
[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 1 of 4 [1] 2 3 4 Next page →
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 00/51] x86/dumpstack: rewrite x86 stack dump code |
| Message-ID | <s5lMR-2Ra-3@gated-at.bofh.it> |
The patch set continues to grow. The most significant changes since
last time are a partial rewrite of the unwinder to give each pt_regs its
own frame, frame pointer encoding for 32-bit, and conversion of the new
CONFIG_HARDENED_USERCOPY arch_within_stack_frames() function to use the
unwinder. See below for the full list of changes.
A git branch is available at:
https://github.com/jpoimboe/linux unwind-v3
Based on tip/master.
v3:
- partial unwinder rewrite: each pt_regs gets its own frame
- add frame pointer encoding support for 32-bit
- several 32-bit fixes and cleanups for issues found by the new warnings
- convert CONFIG_HARDENED_USERCOPY arch_within_stack_frames()
- fix bug in unwinder when skipping stack frames (and add a comment)
- warn on stack recursion
- put start_cpu() in its own function
- export symbols in unwind_guess.c
v2:
- split up several of the patches and reorder them with lower-risk
patches first
- add a lot more comments
- remove the 64-byte gap at the end of the irq stack
- fix some existing ftrace function graph unwinding issues
- fix an existing bug in kernel_stack_pointer()
- clarify the origins of the stack_info "next stack" pointers
- do visit_mask checking in get_stack_info() instead of in_*_stack()
- add some new unwinder warnings
- remove uses of test_and_set_bit()
- dont print regs->ip twice
- remove unwind_state.sp
- have unwind_get_return_address() validate the return address
- change /proc/pid/stack to use %pB
- several minor cleanups and fixes
----
The x86 stack dump code is a bit of a mess. dump_trace() uses
callbacks, and each user of it seems to have slightly different
requirements, so there are several slightly different callbacks floating
around.
Also there are some upcoming features which will require more changes to
the stack dump code: reliable stack detection for live patching,
hardened user copy, and the DWARF unwinder. Each of those features
would at least need more callbacks and/or callback interfaces, resulting
in a much bigger mess than what we have today.
Before doing all that, we should try to clean things up and replace
dump_trace() with something cleaner and more flexible.
The new unwinder is a simple state machine which was heavily inspired by
a suggestion from Andy Lutomirski:
https://lkml.kernel.org/r/CALCETrUbNTqaM2LRyXGRx=kVLRPeY5A3Pc6k4TtQxF320rUT=w@mail.gmail.com
It's also similar to the libunwind API:
http://www.nongnu.org/libunwind/man/libunwind(3).html
Some if its advantages:
- simplicity: no more callback sprawl and less code duplication.
- flexibility: allows the caller to stop and inspect the stack state at
each step in the unwinding process.
- modularity: the unwinder code, console stack dump code, and stack
metadata analysis code are all better separated so that changing one
of them shouldn't have much of an impact on any of the others.
Josh Poimboeuf (51):
x86/dumpstack: remove show_trace()
x86/asm/head: remove unused init_rsp variable extern
x86/asm/head: rename 'stack_start' -> 'initial_stack'
x86/asm/head: use a common function for starting CPUs
x86/dumpstack: make printk_stack_address() more generally useful
x86/dumpstack: add IRQ_USABLE_STACK_SIZE define
x86/dumpstack: remove extra brackets around "<EOE>"
x86/dumpstack: fix irq stack bounds calculation in
show_stack_log_lvl()
x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access
x86/dumpstack: add get_stack_pointer() and get_frame_pointer()
x86/dumpstack: remove unnecessary stack pointer arguments
x86: move _stext marker to before head code
x86/asm/head: remove useless zeroed word
x86/asm/head: put real return address on idle task stack
x86/asm/head: standardize the end of the stack for idle tasks
x86/32: put real return address on stack in entry code
x86/smp: fix initial idle stack location on 32-bit
x86/entry/head/32: use local labels
x86/entry/32: rename 'error_code' to 'common_exception'
perf/x86: check perf_callchain_store() error
oprofile/x86: add regs->ip to oprofile trace
proc: fix return address printk conversion specifer in
/proc/<pid>/stack
ftrace: remove CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST from config
ftrace: only allocate the ret_stack 'fp' field when needed
ftrace: add return address pointer to ftrace_ret_stack
ftrace: add ftrace_graph_ret_addr() stack unwinding helpers
x86/dumpstack/ftrace: convert dump_trace() callbacks to use
ftrace_graph_ret_addr()
ftrace/x86: implement HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
x86/dumpstack/ftrace: mark function graph handler function as
unreliable
x86/dumpstack/ftrace: don't print unreliable addresses in
print_context_stack_bp()
x86/dumpstack: allow preemption in show_stack_log_lvl() and
dump_trace()
x86/dumpstack: simplify in_exception_stack()
x86/dumpstack: add get_stack_info() interface
x86/dumpstack: add recursion checking for all stacks
x86/unwind: add new unwind interface and implementations
perf/x86: convert perf_callchain_kernel() to use the new unwinder
x86/stacktrace: convert save_stack_trace_*() to use the new unwinder
oprofile/x86: convert x86_backtrace() to use the new unwinder
x86/dumpstack: convert show_trace_log_lvl() to use the new unwinder
x86/dumpstack: remove dump_trace() and related callbacks
x86/entry/unwind: create stack frames for saved interrupt registers
x86/unwind: create stack frames for saved syscall registers
x86/dumpstack: print stack identifier on its own line
x86/dumpstack: print any pt_regs found on the stack
x86: remove 64-byte gap at end of irq stack
x86/unwind: warn on kernel stack corruption
x86/unwind: warn on bad stack return address
x86/unwind: warn if stack grows up
x86/dumpstack: warn on stack recursion
x86/mm: move arch_within_stack_frames() to usercopy.c
x86/mm: convert arch_within_stack_frames() to use the new unwinder
Documentation/trace/ftrace-design.txt | 11 ++
arch/arm/kernel/ftrace.c | 2 +-
arch/arm64/kernel/entry-ftrace.S | 2 +-
arch/arm64/kernel/ftrace.c | 2 +-
arch/blackfin/kernel/ftrace-entry.S | 4 +-
arch/blackfin/kernel/ftrace.c | 2 +-
arch/microblaze/kernel/ftrace.c | 2 +-
arch/mips/kernel/ftrace.c | 4 +-
arch/parisc/kernel/ftrace.c | 2 +-
arch/powerpc/kernel/ftrace.c | 3 +-
arch/s390/kernel/ftrace.c | 3 +-
arch/sh/kernel/ftrace.c | 2 +-
arch/sparc/Kconfig | 1 -
arch/sparc/include/asm/ftrace.h | 4 +
arch/sparc/kernel/ftrace.c | 2 +-
arch/tile/kernel/ftrace.c | 2 +-
arch/x86/Kconfig | 1 -
arch/x86/entry/calling.h | 21 +++
arch/x86/entry/entry_32.S | 140 +++++++++------
arch/x86/entry/entry_64.S | 10 +-
arch/x86/events/core.c | 36 ++--
arch/x86/include/asm/ftrace.h | 3 +
arch/x86/include/asm/kdebug.h | 2 -
arch/x86/include/asm/page_64_types.h | 16 +-
arch/x86/include/asm/realmode.h | 2 +-
arch/x86/include/asm/smp.h | 3 -
arch/x86/include/asm/stacktrace.h | 114 ++++++------
arch/x86/include/asm/thread_info.h | 46 +----
arch/x86/include/asm/unwind.h | 107 ++++++++++++
arch/x86/kernel/Makefile | 6 +
arch/x86/kernel/acpi/sleep.c | 2 +-
arch/x86/kernel/cpu/common.c | 2 +-
arch/x86/kernel/dumpstack.c | 272 +++++++++++++----------------
arch/x86/kernel/dumpstack_32.c | 141 ++++++++-------
arch/x86/kernel/dumpstack_64.c | 320 +++++++++++-----------------------
arch/x86/kernel/ftrace.c | 2 +-
arch/x86/kernel/head_32.S | 54 +++---
arch/x86/kernel/head_64.S | 50 +++---
arch/x86/kernel/ptrace.c | 4 +-
arch/x86/kernel/setup_percpu.c | 2 +-
arch/x86/kernel/smpboot.c | 6 +-
arch/x86/kernel/stacktrace.c | 74 +++-----
arch/x86/kernel/unwind_frame.c | 234 +++++++++++++++++++++++++
arch/x86/kernel/unwind_guess.c | 42 +++++
arch/x86/kernel/vmlinux.lds.S | 2 +-
arch/x86/lib/usercopy.c | 56 ++++++
arch/x86/oprofile/backtrace.c | 49 +++---
fs/proc/base.c | 2 +-
include/linux/ftrace.h | 17 +-
kernel/trace/Kconfig | 5 -
kernel/trace/trace_functions_graph.c | 67 ++++++-
51 files changed, 1185 insertions(+), 773 deletions(-)
create mode 100644 arch/x86/include/asm/unwind.h
create mode 100644 arch/x86/kernel/unwind_frame.c
create mode 100644 arch/x86/kernel/unwind_guess.c
--
2.7.4
[toc] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 49/51] x86/dumpstack: warn on stack recursion |
| Message-ID | <s5lMT-2Ra-47@gated-at.bofh.it> |
| In reply to | #1461224 |
Print a warning if stack recursion is detected.
Use printk_deferred_once() because the unwinder can be called with the
console lock by lockdep via save_stack_trace().
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/kernel/dumpstack_32.c | 5 ++++-
arch/x86/kernel/dumpstack_64.c | 5 ++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 53d939e..cbbef0e 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -101,8 +101,11 @@ recursion_check:
* just break out and report an unknown stack type.
*/
if (visit_mask) {
- if (*visit_mask & (1UL << info->type))
+ if (*visit_mask & (1UL << info->type)) {
+ printk_deferred_once(KERN_WARNING "WARNING: stack recursion on stack type %d\n",
+ info->type);
goto unknown;
+ }
*visit_mask |= 1UL << info->type;
}
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 001a75d..36581f9 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -125,8 +125,11 @@ recursion_check:
* just break out and report an unknown stack type.
*/
if (visit_mask) {
- if (*visit_mask & (1UL << info->type))
+ if (*visit_mask & (1UL << info->type)) {
+ printk_deferred_once(KERN_WARNING "WARNING: stack recursion on stack type %d\n",
+ info->type);
goto unknown;
+ }
*visit_mask |= 1UL << info->type;
}
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 36/51] perf/x86: convert perf_callchain_kernel() to use the new unwinder |
| Message-ID | <s5lMT-2Ra-59@gated-at.bofh.it> |
| In reply to | #1461224 |
Convert perf_callchain_kernel() to use the new unwinder. dump_trace()
has been deprecated.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/events/core.c | 33 ++++++++++-----------------------
1 file changed, 10 insertions(+), 23 deletions(-)
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index dd3a1dc..b409e7c 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -37,6 +37,7 @@
#include <asm/timer.h>
#include <asm/desc.h>
#include <asm/ldt.h>
+#include <asm/unwind.h>
#include "perf_event.h"
@@ -2267,31 +2268,12 @@ void arch_perf_update_userpage(struct perf_event *event,
cyc2ns_read_end(data);
}
-/*
- * callchain support
- */
-
-static int backtrace_stack(void *data, const char *name)
-{
- return 0;
-}
-
-static int backtrace_address(void *data, unsigned long addr, int reliable)
-{
- struct perf_callchain_entry_ctx *entry = data;
-
- return perf_callchain_store(entry, addr);
-}
-
-static const struct stacktrace_ops backtrace_ops = {
- .stack = backtrace_stack,
- .address = backtrace_address,
- .walk_stack = print_context_stack_bp,
-};
-
void
perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs)
{
+ struct unwind_state state;
+ unsigned long addr;
+
if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
/* TODO: We don't support guest os callchain now */
return;
@@ -2300,7 +2282,12 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
if (perf_callchain_store(entry, regs->ip))
return;
- dump_trace(NULL, regs, NULL, 0, &backtrace_ops, entry);
+ for (unwind_start(&state, NULL, regs, NULL); !unwind_done(&state);
+ unwind_next_frame(&state)) {
+ addr = unwind_get_return_address(&state);
+ if (!addr || perf_callchain_store(entry, addr))
+ return;
+ }
}
static inline int
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 26/51] ftrace: add ftrace_graph_ret_addr() stack unwinding helpers |
| Message-ID | <s5lMT-2Ra-53@gated-at.bofh.it> |
| In reply to | #1461224 |
When function graph tracing is enabled for a function, ftrace modifies
the stack by replacing the original return address with the address of a
hook function (return_to_handler).
Stack unwinders need a way to get the original return address. Add an
arch-independent helper function for that named ftrace_graph_ret_addr().
This adds two variations of the function: one depends on
HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, and the other relies on an index state
variable.
The former is recommended because, in some cases, the latter can cause
problems when the unwinder skips stack frames. It can get out of sync
with the ret_stack index and wrong addresses can be reported for the
stack trace.
Once all arches have been ported to use
HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, we can get rid of the distinction.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
include/linux/ftrace.h | 10 +++++++
kernel/trace/trace_functions_graph.c | 58 ++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 483e02a..6f93ac4 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -814,6 +814,9 @@ extern int
ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
unsigned long frame_pointer, unsigned long *retp);
+unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
+ unsigned long ret, unsigned long *retp);
+
/*
* Sometimes we don't want to trace a function with the function
* graph tracer but we want them to keep traced by the usual function
@@ -875,6 +878,13 @@ static inline int task_curr_ret_stack(struct task_struct *tsk)
return -1;
}
+static inline unsigned long
+ftrace_graph_ret_addr(struct task_struct *task, int *idx, unsigned long ret,
+ unsigned long *retp)
+{
+ return ret;
+}
+
static inline void pause_graph_tracing(void) { }
static inline void unpause_graph_tracing(void) { }
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index f7212ec..0cbe38a 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -284,6 +284,64 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
return ret;
}
+/**
+ * ftrace_graph_ret_addr - convert a potentially modified stack return address
+ * to its original value
+ *
+ * This function can be called by stack unwinding code to convert a found stack
+ * return address ('ret') to its original value, in case the function graph
+ * tracer has modified it to be 'return_to_handler'. If the address hasn't
+ * been modified, the unchanged value of 'ret' is returned.
+ *
+ * 'idx' is a state variable which should be initialized by the caller to zero
+ * before the first call.
+ *
+ * 'retp' is a pointer to the return address on the stack. It's ignored if
+ * the arch doesn't have HAVE_FUNCTION_GRAPH_RET_ADDR_PTR defined.
+ */
+#ifdef HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
+unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
+ unsigned long ret, unsigned long *retp)
+{
+ int index = task->curr_ret_stack;
+ int i;
+
+ if (ret != (unsigned long)return_to_handler)
+ return ret;
+
+ if (index < -1)
+ index += FTRACE_NOTRACE_DEPTH;
+
+ if (index < 0)
+ return ret;
+
+ for (i = 0; i <= index; i++)
+ if (task->ret_stack[i].retp == retp)
+ return task->ret_stack[i].ret;
+
+ return ret;
+}
+#else /* !HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */
+unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
+ unsigned long ret, unsigned long *retp)
+{
+ int task_idx;
+
+ if (ret != (unsigned long)return_to_handler)
+ return ret;
+
+ task_idx = task->curr_ret_stack;
+
+ if (!task->ret_stack || task_idx < *idx)
+ return ret;
+
+ task_idx -= *idx;
+ (*idx)++;
+
+ return task->ret_stack[task_idx].ret;
+}
+#endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */
+
int __trace_graph_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned long flags,
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 33/51] x86/dumpstack: add get_stack_info() interface |
| Message-ID | <s5lMT-2Ra-45@gated-at.bofh.it> |
| In reply to | #1461224 |
valid_stack_ptr() is buggy: it assumes that all stacks are of size
THREAD_SIZE, which is not true for exception stacks. So the
walk_stack() callbacks will need to know the location of the beginning
of the stack as well as the end.
Another issue is that in general the various features of a stack (type,
size, next stack pointer, description string) are scattered around in
various places throughout the stack dump code.
Encapsulate all that information in a single place with a new stack_info
struct and a get_stack_info() interface.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/events/core.c | 2 +-
arch/x86/include/asm/stacktrace.h | 41 +++++++++-
arch/x86/kernel/dumpstack.c | 40 ++++-----
arch/x86/kernel/dumpstack_32.c | 106 ++++++++++++++++++------
arch/x86/kernel/dumpstack_64.c | 165 ++++++++++++++++++++------------------
arch/x86/kernel/stacktrace.c | 2 +-
arch/x86/oprofile/backtrace.c | 2 +-
7 files changed, 231 insertions(+), 127 deletions(-)
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index dcaa887..dd3a1dc 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2271,7 +2271,7 @@ void arch_perf_update_userpage(struct perf_event *event,
* callchain support
*/
-static int backtrace_stack(void *data, char *name)
+static int backtrace_stack(void *data, const char *name)
{
return 0;
}
diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index 6f65995..be9273c 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -9,6 +9,39 @@
#include <linux/uaccess.h>
#include <linux/ptrace.h>
+enum stack_type {
+ STACK_TYPE_UNKNOWN,
+ STACK_TYPE_TASK,
+ STACK_TYPE_IRQ,
+ STACK_TYPE_SOFTIRQ,
+ STACK_TYPE_EXCEPTION,
+ STACK_TYPE_EXCEPTION_LAST = STACK_TYPE_EXCEPTION + N_EXCEPTION_STACKS-1,
+};
+
+struct stack_info {
+ enum stack_type type;
+ unsigned long *begin, *end, *next_sp;
+};
+
+bool in_task_stack(unsigned long *stack, struct task_struct *task,
+ struct stack_info *info);
+
+int get_stack_info(unsigned long *stack, struct task_struct *task,
+ struct stack_info *info, unsigned long *visit_mask);
+
+void stack_type_str(enum stack_type type, const char **begin,
+ const char **end);
+
+static inline bool on_stack(struct stack_info *info, void *addr, size_t len)
+{
+ void *begin = info->begin;
+ void *end = info->end;
+
+ return (info->type != STACK_TYPE_UNKNOWN &&
+ addr >= begin && addr < end &&
+ addr + len > begin && addr + len <= end);
+}
+
extern int kstack_depth_to_print;
struct thread_info;
@@ -19,27 +52,27 @@ typedef unsigned long (*walk_stack_t)(struct task_struct *task,
unsigned long bp,
const struct stacktrace_ops *ops,
void *data,
- unsigned long *end,
+ struct stack_info *info,
int *graph);
extern unsigned long
print_context_stack(struct task_struct *task,
unsigned long *stack, unsigned long bp,
const struct stacktrace_ops *ops, void *data,
- unsigned long *end, int *graph);
+ struct stack_info *info, int *graph);
extern unsigned long
print_context_stack_bp(struct task_struct *task,
unsigned long *stack, unsigned long bp,
const struct stacktrace_ops *ops, void *data,
- unsigned long *end, int *graph);
+ struct stack_info *info, int *graph);
/* Generic stack tracer with callbacks */
struct stacktrace_ops {
int (*address)(void *data, unsigned long address, int reliable);
/* On negative return stop dumping */
- int (*stack)(void *data, char *name);
+ int (*stack)(void *data, const char *name);
walk_stack_t walk_stack;
};
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index c6c6c39..aa208e5 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -25,6 +25,23 @@ unsigned int code_bytes = 64;
int kstack_depth_to_print = 3 * STACKSLOTS_PER_LINE;
static int die_counter;
+bool in_task_stack(unsigned long *stack, struct task_struct *task,
+ struct stack_info *info)
+{
+ unsigned long *begin = task_stack_page(task);
+ unsigned long *end = task_stack_page(task) + THREAD_SIZE;
+
+ if (stack < begin || stack >= end)
+ return false;
+
+ info->type = STACK_TYPE_TASK;
+ info->begin = begin;
+ info->end = end;
+ info->next_sp = NULL;
+
+ return true;
+}
+
static void printk_stack_address(unsigned long address, int reliable,
char *log_lvl)
{
@@ -46,24 +63,11 @@ void printk_address(unsigned long address)
* severe exception (double fault, nmi, stack fault, debug, mce) hardware stack
*/
-static inline int valid_stack_ptr(struct task_struct *task,
- void *p, unsigned int size, void *end)
-{
- void *t = task_stack_page(task);
- if (end) {
- if (p < end && p >= (end-THREAD_SIZE))
- return 1;
- else
- return 0;
- }
- return p >= t && p < t + THREAD_SIZE - size;
-}
-
unsigned long
print_context_stack(struct task_struct *task,
unsigned long *stack, unsigned long bp,
const struct stacktrace_ops *ops, void *data,
- unsigned long *end, int *graph)
+ struct stack_info *info, int *graph)
{
struct stack_frame *frame = (struct stack_frame *)bp;
@@ -75,7 +79,7 @@ print_context_stack(struct task_struct *task,
PAGE_SIZE)
stack = (unsigned long *)task_stack_page(task);
- while (valid_stack_ptr(task, stack, sizeof(*stack), end)) {
+ while (on_stack(info, stack, sizeof(*stack))) {
unsigned long addr = *stack;
if (__kernel_text_address(addr)) {
@@ -114,12 +118,12 @@ unsigned long
print_context_stack_bp(struct task_struct *task,
unsigned long *stack, unsigned long bp,
const struct stacktrace_ops *ops, void *data,
- unsigned long *end, int *graph)
+ struct stack_info *info, int *graph)
{
struct stack_frame *frame = (struct stack_frame *)bp;
unsigned long *retp = &frame->return_address;
- while (valid_stack_ptr(task, retp, sizeof(*retp), end)) {
+ while (on_stack(info, stack, sizeof(*stack) * 2)) {
unsigned long addr = *retp;
unsigned long real_addr;
@@ -138,7 +142,7 @@ print_context_stack_bp(struct task_struct *task,
}
EXPORT_SYMBOL_GPL(print_context_stack_bp);
-static int print_trace_stack(void *data, char *name)
+static int print_trace_stack(void *data, const char *name)
{
printk("%s <%s> ", (char *)data, name);
return 0;
diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index b07d5c9..51a113b 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -16,61 +16,117 @@
#include <asm/stacktrace.h>
-static void *is_irq_stack(void *p, void *irq)
+void stack_type_str(enum stack_type type, const char **begin, const char **end)
{
- if (p < irq || p >= (irq + THREAD_SIZE))
- return NULL;
- return irq + THREAD_SIZE;
+ switch (type) {
+ case STACK_TYPE_IRQ:
+ case STACK_TYPE_SOFTIRQ:
+ *begin = "IRQ";
+ *end = "EOI";
+ break;
+ default:
+ *begin = NULL;
+ *end = NULL;
+ }
}
+static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info)
+{
+ unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
+ unsigned long *end = begin + (THREAD_SIZE / sizeof(long));
+
+ if (stack < begin || stack >= end)
+ return false;
+
+ info->type = STACK_TYPE_IRQ;
+ info->begin = begin;
+ info->end = end;
+
+ /*
+ * See irq_32.c -- the next stack pointer is stored at the beginning of
+ * the stack.
+ */
+ info->next_sp = (unsigned long *)*begin;
+
+ return true;
+}
-static void *is_hardirq_stack(unsigned long *stack)
+static bool in_softirq_stack(unsigned long *stack, struct stack_info *info)
{
- void *irq = this_cpu_read(hardirq_stack);
+ unsigned long *begin = (unsigned long *)this_cpu_read(softirq_stack);
+ unsigned long *end = begin + (THREAD_SIZE / sizeof(long));
+
+ if (stack < begin || stack >= end)
+ return false;
+
+ info->type = STACK_TYPE_SOFTIRQ;
+ info->begin = begin;
+ info->end = end;
+
+ /*
+ * See irq_32.c -- the next stack pointer is stored at the beginning of
+ * the stack.
+ */
+ info->next_sp = (unsigned long *)*begin;
- return is_irq_stack(stack, irq);
+ return true;
}
-static void *is_softirq_stack(unsigned long *stack);
+int get_stack_info(unsigned long *stack, struct task_struct *task,
+ struct stack_info *info, unsigned long *visit_mask)
{
- void *irq = this_cpu_read(softirq_stack);
+ if (!stack)
+ goto unknown;
- return is_irq_stack(stack, irq);
+ task = task ? : current;
+
+ if (in_task_stack(stack, task, info))
+ return 0;
+
+ if (task != current)
+ goto unknown;
+
+ if (in_hardirq_stack(stack, info))
+ return 0;
+
+ if (in_softirq_stack(stack, info))
+ return 0;
+
+unknown:
+ info->type = STACK_TYPE_UNKNOWN;
+ return -EINVAL;
}
void dump_trace(struct task_struct *task, struct pt_regs *regs,
unsigned long *stack, unsigned long bp,
const struct stacktrace_ops *ops, void *data)
{
+ unsigned long visit_mask = 0;
int graph = 0;
- u32 *prev_esp;
task = task ? : current;
stack = stack ? : get_stack_pointer(task, regs);
bp = bp ? : (unsigned long)get_frame_pointer(task, regs);
for (;;) {
- void *end_stack;
+ const char *begin_str, *end_str;
+ struct stack_info info;
- end_stack = is_hardirq_stack(stack);
- if (!end_stack)
- end_stack = is_softirq_stack(stack);
+ if (get_stack_info(stack, task, &info, &visit_mask))
+ break;
- bp = ops->walk_stack(task, stack, bp, ops, data,
- end_stack, &graph);
+ stack_type_str(info.type, &begin_str, &end_str);
- /* Stop if not on irq stack */
- if (!end_stack)
+ if (begin_str && ops->stack(data, begin_str) < 0)
break;
- /* The previous esp is saved on the bottom of the stack */
- prev_esp = (u32 *)(end_stack - THREAD_SIZE);
- stack = (unsigned long *)*prev_esp;
- if (!stack)
- break;
+ bp = ops->walk_stack(task, stack, bp, ops, data, &info, &graph);
- if (ops->stack(data, "IRQ") < 0)
+ if (end_str && ops->stack(data, end_str) < 0)
break;
+
+ stack = info.next_sp;
+
touch_nmi_watchdog();
}
}
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 69f6ba2..2e8c750 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -28,17 +28,38 @@ static unsigned long exception_stack_sizes[N_EXCEPTION_STACKS] = {
[DEBUG_STACK - 1] = DEBUG_STKSZ
};
-static unsigned long *in_exception_stack(unsigned long stack, unsigned *usedp,
- char **idp)
+void stack_type_str(enum stack_type type, const char **begin, const char **end)
{
- unsigned long begin, end;
+ BUILD_BUG_ON(N_EXCEPTION_STACKS != 4);
+
+ switch (type) {
+ case STACK_TYPE_IRQ:
+ *begin = "IRQ";
+ *end = "EOI";
+ break;
+ case STACK_TYPE_EXCEPTION ... STACK_TYPE_EXCEPTION_LAST:
+ *begin = exception_stack_names[type - STACK_TYPE_EXCEPTION];
+ *end = "EOE";
+ break;
+ default:
+ *begin = NULL;
+ *end = NULL;
+ }
+}
+
+static bool in_exception_stack(unsigned long *stack, struct stack_info *info,
+ unsigned long *visit_mask)
+{
+ unsigned long *begin, *end;
+ struct pt_regs *regs;
unsigned k;
BUILD_BUG_ON(N_EXCEPTION_STACKS != 4);
for (k = 0; k < N_EXCEPTION_STACKS; k++) {
- end = raw_cpu_ptr(&orig_ist)->ist[k];
- begin = end - exception_stack_sizes[k];
+ end = (unsigned long *)raw_cpu_ptr(&orig_ist)->ist[k];
+ begin = end - (exception_stack_sizes[k] / sizeof(long));
+ regs = (struct pt_regs *)end - 1;
if (stack < begin || stack >= end)
continue;
@@ -48,56 +69,67 @@ static unsigned long *in_exception_stack(unsigned long stack, unsigned *usedp,
* If it comes up for the second time then there's something
* wrong going on - just break and return NULL:
*/
- if (*usedp & (1U << k))
+ if (*visit_mask & (1U << k))
break;
- *usedp |= 1U << k;
+ *visit_mask |= 1U << k;
- *idp = exception_stack_names[k];
- return (unsigned long *)end;
+ info->type = STACK_TYPE_EXCEPTION + k;
+ info->begin = begin;
+ info->end = end;
+ info->next_sp = (unsigned long *)regs->sp;
+
+ return true;
}
- return NULL;
+ return false;
}
-static inline int
-in_irq_stack(unsigned long *stack, unsigned long *irq_stack,
- unsigned long *irq_stack_end)
+static bool in_irq_stack(unsigned long *stack, struct stack_info *info)
{
- return (stack >= irq_stack && stack < irq_stack_end);
-}
+ unsigned long *end = (unsigned long *)this_cpu_read(irq_stack_ptr);
+ unsigned long *begin = end - (IRQ_USABLE_STACK_SIZE / sizeof(long));
-enum stack_type {
- STACK_IS_UNKNOWN,
- STACK_IS_NORMAL,
- STACK_IS_EXCEPTION,
- STACK_IS_IRQ,
-};
+ if (stack < begin || stack >= end)
+ return false;
+
+ info->type = STACK_TYPE_IRQ;
+ info->begin = begin;
+ info->end = end;
+
+ /*
+ * The next stack pointer is the first thing pushed by the entry code
+ * after switching to the irq stack.
+ */
+ info->next_sp = (unsigned long *)*(end - 1);
+
+ return true;
+}
-static enum stack_type
-analyze_stack(struct task_struct *task, unsigned long *stack,
- unsigned long **stack_end, unsigned long *irq_stack,
- unsigned *used, char **id)
+int get_stack_info(unsigned long *stack, struct task_struct *task,
+ struct stack_info *info, unsigned long *visit_mask)
{
- unsigned long addr;
+ if (!stack)
+ goto unknown;
- addr = ((unsigned long)stack & (~(THREAD_SIZE - 1)));
- if ((unsigned long)task_stack_page(task) == addr)
- return STACK_IS_NORMAL;
+ task = task ? : current;
+
+ if (in_task_stack(stack, task, info))
+ return 0;
- *stack_end = in_exception_stack((unsigned long)stack, used, id);
- if (*stack_end)
- return STACK_IS_EXCEPTION;
+ if (task != current)
+ goto unknown;
- if (!irq_stack)
- return STACK_IS_NORMAL;
+ if (in_exception_stack(stack, info, visit_mask))
+ return 0;
- *stack_end = irq_stack;
- irq_stack -= (IRQ_USABLE_STACK_SIZE / sizeof(long));
+ if (in_irq_stack(stack, info))
+ return 0;
- if (in_irq_stack(stack, irq_stack, *stack_end))
- return STACK_IS_IRQ;
+ return 0;
- return STACK_IS_UNKNOWN;
+unknown:
+ info->type = STACK_TYPE_UNKNOWN;
+ return -EINVAL;
}
/*
@@ -111,8 +143,8 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
unsigned long *stack, unsigned long bp,
const struct stacktrace_ops *ops, void *data)
{
- unsigned long *irq_stack = (unsigned long *)this_cpu_read(irq_stack_ptr);
- unsigned used = 0;
+ unsigned long visit_mask = 0;
+ struct stack_info info;
int graph = 0;
int done = 0;
@@ -126,57 +158,37 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
* exceptions
*/
while (!done) {
- unsigned long *stack_end;
- enum stack_type stype;
- char *id;
+ const char *begin_str, *end_str;
- stype = analyze_stack(task, stack, &stack_end, irq_stack, &used,
- &id);
+ get_stack_info(stack, task, &info, &visit_mask);
/* Default finish unless specified to continue */
done = 1;
- switch (stype) {
+ switch (info.type) {
/* Break out early if we are on the thread stack */
- case STACK_IS_NORMAL:
+ case STACK_TYPE_TASK:
break;
- case STACK_IS_EXCEPTION:
+ case STACK_TYPE_IRQ:
+ case STACK_TYPE_EXCEPTION ... STACK_TYPE_EXCEPTION_LAST:
+
+ stack_type_str(info.type, &begin_str, &end_str);
- if (ops->stack(data, id) < 0)
+ if (ops->stack(data, begin_str) < 0)
break;
bp = ops->walk_stack(task, stack, bp, ops,
- data, stack_end, &graph);
- ops->stack(data, "EOE");
- /*
- * We link to the next stack via the
- * second-to-last pointer (index -2 to end) in the
- * exception stack:
- */
- stack = (unsigned long *) stack_end[-2];
- done = 0;
- break;
+ data, &info, &graph);
- case STACK_IS_IRQ:
+ ops->stack(data, end_str);
- if (ops->stack(data, "IRQ") < 0)
- break;
- bp = ops->walk_stack(task, stack, bp,
- ops, data, stack_end, &graph);
- /*
- * We link to the next stack (which would be
- * the process stack normally) the last
- * pointer (index -1 to end) in the IRQ stack:
- */
- stack = (unsigned long *) (stack_end[-1]);
- irq_stack = NULL;
- ops->stack(data, "EOI");
+ stack = info.next_sp;
done = 0;
break;
- case STACK_IS_UNKNOWN:
+ default:
ops->stack(data, "UNK");
break;
}
@@ -185,7 +197,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
/*
* This handles the process stack:
*/
- bp = ops->walk_stack(task, stack, bp, ops, data, NULL, &graph);
+ bp = ops->walk_stack(task, stack, bp, ops, data, &info, &graph);
}
EXPORT_SYMBOL(dump_trace);
@@ -193,8 +205,7 @@ void
show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
unsigned long *sp, unsigned long bp, char *log_lvl)
{
- unsigned long *irq_stack_end;
- unsigned long *irq_stack;
+ unsigned long *irq_stack, *irq_stack_end;
unsigned long *stack;
int i;
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 4738f5e..785aef1 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -9,7 +9,7 @@
#include <linux/uaccess.h>
#include <asm/stacktrace.h>
-static int save_stack_stack(void *data, char *name)
+static int save_stack_stack(void *data, const char *name)
{
return 0;
}
diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c
index d950f9e..7539148 100644
--- a/arch/x86/oprofile/backtrace.c
+++ b/arch/x86/oprofile/backtrace.c
@@ -17,7 +17,7 @@
#include <asm/ptrace.h>
#include <asm/stacktrace.h>
-static int backtrace_stack(void *data, char *name)
+static int backtrace_stack(void *data, const char *name)
{
/* Yes, we want all stacks */
return 0;
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 48/51] x86/unwind: warn if stack grows up |
| Message-ID | <s5lMT-2Ra-55@gated-at.bofh.it> |
| In reply to | #1461224 |
Add a sanity check to ensure the stack only grows down, and print a
warning if the check fails.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/kernel/unwind_frame.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 5496462..f21b7ef 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -32,6 +32,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
}
EXPORT_SYMBOL_GPL(unwind_get_return_address);
+static size_t regs_size(struct pt_regs *regs)
+{
+ /* x86_32 regs from kernel mode are two words shorter */
+ if (IS_ENABLED(CONFIG_X86_32) && !user_mode(regs))
+ return sizeof(*regs) - (2*sizeof(long));
+
+ return sizeof(*regs);
+}
+
static bool is_last_task_frame(struct unwind_state *state)
{
unsigned long bp = (unsigned long)state->bp;
@@ -85,6 +94,7 @@ bool unwind_next_frame(struct unwind_state *state)
struct pt_regs *regs;
unsigned long *next_bp, *next_sp;
size_t next_len;
+ enum stack_type prev_type = state->stack_info.type;
if (unwind_done(state))
return false;
@@ -140,6 +150,18 @@ bool unwind_next_frame(struct unwind_state *state)
if (!update_stack_state(state, next_sp, next_len))
goto bad_address;
+ /* make sure it only unwinds up and doesn't overlap the last frame */
+ if (state->stack_info.type == prev_type) {
+ if (state->regs &&
+ (void *)next_sp < (void *)state->regs +
+ regs_size(state->regs))
+ goto bad_address;
+
+ if (state->bp &&
+ (void *)next_sp < (void *)state->bp + FRAME_HEADER_SIZE)
+ goto bad_address;
+ }
+
/* move to the next frame */
if (regs) {
state->regs = regs;
@@ -156,12 +178,12 @@ bad_address:
printk_deferred_once(KERN_WARNING
"WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n",
state->regs, state->task->comm,
- state->task->pid, next_bp);
+ state->task->pid, next_sp);
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_bp);
+ state->task->pid, next_sp);
the_end:
state->stack_info.type = STACK_TYPE_UNKNOWN;
return false;
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-08-14 10:30 +0200 |
| Subject | Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up |
| Message-ID | <s5YXT-5oj-11@gated-at.bofh.it> |
| In reply to | #1461231 |
On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> Add a sanity check to ensure the stack only grows down, and print a
> warning if the check fails.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> arch/x86/kernel/unwind_frame.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> index 5496462..f21b7ef 100644
> --- a/arch/x86/kernel/unwind_frame.c
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -32,6 +32,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
> }
> EXPORT_SYMBOL_GPL(unwind_get_return_address);
>
> +static size_t regs_size(struct pt_regs *regs)
> +{
> + /* x86_32 regs from kernel mode are two words shorter */
> + if (IS_ENABLED(CONFIG_X86_32) && !user_mode(regs))
> + return sizeof(*regs) - (2*sizeof(long));
> +
> + return sizeof(*regs);
> +}
> +
> static bool is_last_task_frame(struct unwind_state *state)
> {
> unsigned long bp = (unsigned long)state->bp;
> @@ -85,6 +94,7 @@ bool unwind_next_frame(struct unwind_state *state)
> struct pt_regs *regs;
> unsigned long *next_bp, *next_sp;
> size_t next_len;
> + enum stack_type prev_type = state->stack_info.type;
>
> if (unwind_done(state))
> return false;
> @@ -140,6 +150,18 @@ bool unwind_next_frame(struct unwind_state *state)
> if (!update_stack_state(state, next_sp, next_len))
> goto bad_address;
>
> + /* make sure it only unwinds up and doesn't overlap the last frame */
> + if (state->stack_info.type == prev_type) {
> + if (state->regs &&
> + (void *)next_sp < (void *)state->regs +
> + regs_size(state->regs))
> + goto bad_address;
> +
> + if (state->bp &&
> + (void *)next_sp < (void *)state->bp + FRAME_HEADER_SIZE)
> + goto bad_address;
> + }
> +
Maybe this is obvious in context, but does something prevent this
error from firing if the stack switched? That is:
pushq $rbp
movq $rsp, $rbp
...
movq [irq stack], $rsp
<- rsp and rbp have no particular relationship right now.
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-15 18:30 +0200 |
| Subject | Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up |
| Message-ID | <s6sVY-7S0-15@gated-at.bofh.it> |
| In reply to | #1461683 |
On Sun, Aug 14, 2016 at 12:56:40AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > Add a sanity check to ensure the stack only grows down, and print a
> > warning if the check fails.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> > arch/x86/kernel/unwind_frame.c | 26 ++++++++++++++++++++++++--
> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > index 5496462..f21b7ef 100644
> > --- a/arch/x86/kernel/unwind_frame.c
> > +++ b/arch/x86/kernel/unwind_frame.c
> > @@ -32,6 +32,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
> > }
> > EXPORT_SYMBOL_GPL(unwind_get_return_address);
> >
> > +static size_t regs_size(struct pt_regs *regs)
> > +{
> > + /* x86_32 regs from kernel mode are two words shorter */
> > + if (IS_ENABLED(CONFIG_X86_32) && !user_mode(regs))
> > + return sizeof(*regs) - (2*sizeof(long));
> > +
> > + return sizeof(*regs);
> > +}
> > +
> > static bool is_last_task_frame(struct unwind_state *state)
> > {
> > unsigned long bp = (unsigned long)state->bp;
> > @@ -85,6 +94,7 @@ bool unwind_next_frame(struct unwind_state *state)
> > struct pt_regs *regs;
> > unsigned long *next_bp, *next_sp;
> > size_t next_len;
> > + enum stack_type prev_type = state->stack_info.type;
> >
> > if (unwind_done(state))
> > return false;
> > @@ -140,6 +150,18 @@ bool unwind_next_frame(struct unwind_state *state)
> > if (!update_stack_state(state, next_sp, next_len))
> > goto bad_address;
> >
> > + /* make sure it only unwinds up and doesn't overlap the last frame */
> > + if (state->stack_info.type == prev_type) {
> > + if (state->regs &&
> > + (void *)next_sp < (void *)state->regs +
> > + regs_size(state->regs))
> > + goto bad_address;
> > +
> > + if (state->bp &&
> > + (void *)next_sp < (void *)state->bp + FRAME_HEADER_SIZE)
> > + goto bad_address;
> > + }
> > +
>
> Maybe this is obvious in context, but does something prevent this
> error from firing if the stack switched? That is:
>
> pushq $rbp
> movq $rsp, $rbp
> ...
> movq [irq stack], $rsp
> <- rsp and rbp have no particular relationship right now.
Short answer:
No, because the above warning only happens between two "frame" pointers
(where frame pointer might be a regs pointer) when the two frames are on
the same stack. This warning has nothing to do with the stack pointer,
despite the "next_sp" name. I should probably rename "next_sp" to
"next_frame" or something.
Long answer:
The unwinder is frame-pointer based, so in most cases it completely
ignores the value of the stack pointer. The only exceptions are:
a) in __unwind_start() where it can use the value of regs->sp to determine how
many frames to skip; and
b) when reading the next stack pointer to switch to the next stack.
If for example the regs where taken from an interrupt right after the
stack had been switched, then no frame would contain the stack pointer,
and __unwind_start() would skip all the frames, and the unwind would be
reported as empty.
Such edge cases are exceedingly rare, and are acceptable IMO, because
frame pointers and interrupts are inherently not 100% compatible. And
these edge cases already exist in today's code.
(And I should reiterate that even when the unwinder breaks down like
that, the oops dump code should still keep going and show all the
addresses anyway.)
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| Message-ID | <s5lMT-2Ra-61@gated-at.bofh.it> |
| In reply to | #1461224 |
When I tried to port arch_within_stack_frames() to use the new unwinder,
I got a nightmare include file "header soup" scenario when unwind.h was
included from thread_info.h. And anyway, I think thread_info.h isn't
really an appropriate place for this function. So move it to usercopy.c
instead.
Since it relies on its parent's stack pointer, and the function is no
longer inlined, the arguments to the __builtin_frame_address() calls
have been incremented.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/include/asm/thread_info.h | 46 ++++++++------------------------------
arch/x86/lib/usercopy.c | 43 +++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 37 deletions(-)
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 8b7c8d8e..fd849e6 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -176,49 +176,21 @@ static inline unsigned long current_stack_pointer(void)
return sp;
}
-/*
- * Walks up the stack frames to make sure that the specified object is
- * entirely contained by a single stack frame.
- *
- * Returns:
- * 1 if within a frame
- * -1 if placed across a frame boundary (or outside stack)
- * 0 unable to determine (no frame pointers, etc)
- */
+#ifdef CONFIG_HARDENED_USERCOPY
+#ifdef CONFIG_FRAME_POINTER
+int arch_within_stack_frames(const void * const stack,
+ const void * const stackend,
+ const void *obj, unsigned long len);
+#else
static inline int arch_within_stack_frames(const void * const stack,
const void * const stackend,
const void *obj, unsigned long len)
{
-#if defined(CONFIG_FRAME_POINTER)
- const void *frame = NULL;
- const void *oldframe;
-
- oldframe = __builtin_frame_address(1);
- if (oldframe)
- frame = __builtin_frame_address(2);
- /*
- * low ----------------------------------------------> high
- * [saved bp][saved ip][args][local vars][saved bp][saved ip]
- * ^----------------^
- * allow copies only within here
- */
- while (stack <= frame && frame < stackend) {
- /*
- * If obj + len extends past the last frame, this
- * check won't pass and the next frame will be 0,
- * causing us to bail out and correctly report
- * the copy as invalid.
- */
- if (obj + len <= frame)
- return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
- oldframe = frame;
- frame = *(const void * const *)frame;
- }
- return -1;
-#else
return 0;
-#endif
}
+#endif /* CONFIG_FRAME_POINTER */
+#endif /* CONFIG_HARDENED_USERCOPY */
+
#else /* !__ASSEMBLY__ */
diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
index b490878..96ce151 100644
--- a/arch/x86/lib/usercopy.c
+++ b/arch/x86/lib/usercopy.c
@@ -9,6 +9,7 @@
#include <asm/word-at-a-time.h>
#include <linux/sched.h>
+#include <asm/unwind.h>
/*
* We rely on the nested NMI work to allow atomic faults from the NMI path; the
@@ -34,3 +35,45 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
return ret;
}
EXPORT_SYMBOL_GPL(copy_from_user_nmi);
+
+#ifdef CONFIG_HARDENED_USERCOPY
+/*
+ * Walks up the stack frames to make sure that the specified object is
+ * entirely contained by a single stack frame.
+ *
+ * Returns:
+ * 1 if within a frame
+ * -1 if placed across a frame boundary (or outside stack)
+ * 0 unable to determine (no frame pointers, etc)
+ */
+int arch_within_stack_frames(const void * const stack,
+ const void * const stackend,
+ const void *obj, unsigned long len)
+{
+ const void *frame = NULL;
+ const void *oldframe;
+
+ oldframe = __builtin_frame_address(2);
+ if (oldframe)
+ frame = __builtin_frame_address(3);
+ /*
+ * low ----------------------------------------------> high
+ * [saved bp][saved ip][args][local vars][saved bp][saved ip]
+ * ^----------------^
+ * allow copies only within here
+ */
+ while (stack <= frame && frame < stackend) {
+ /*
+ * If obj + len extends past the last frame, this
+ * check won't pass and the next frame will be 0,
+ * causing us to bail out and correctly report
+ * the copy as invalid.
+ */
+ if (obj + len <= frame)
+ return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
+ oldframe = frame;
+ frame = *(const void * const *)frame;
+ }
+ return -1;
+}
+#endif /* CONFIG_HARDENED_USERCOPY */
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-08-12 19:40 +0200 |
| Subject | Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| Message-ID | <s5oB4-4Ie-7@gated-at.bofh.it> |
| In reply to | #1461232 |
On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> When I tried to port arch_within_stack_frames() to use the new unwinder,
> I got a nightmare include file "header soup" scenario when unwind.h was
> included from thread_info.h. And anyway, I think thread_info.h isn't
> really an appropriate place for this function. So move it to usercopy.c
> instead.
>
> Since it relies on its parent's stack pointer, and the function is no
> longer inlined, the arguments to the __builtin_frame_address() calls
> have been incremented.
Cool, looks good (minor change noted below). This patch might be a
good place to drop this from mm/Makefile too:
# Since __builtin_frame_address does work as used, disable the warning.
CFLAGS_usercopy.o += $(call cc-disable-warning, frame-address)
Since frame-address warnings have been disabled globally now since
commit 124a3d88fa20 ("Disable "frame-address" warning").
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> arch/x86/include/asm/thread_info.h | 46 ++++++++------------------------------
> arch/x86/lib/usercopy.c | 43 +++++++++++++++++++++++++++++++++++
> 2 files changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index 8b7c8d8e..fd849e6 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -176,49 +176,21 @@ static inline unsigned long current_stack_pointer(void)
> return sp;
> }
>
> -/*
> - * Walks up the stack frames to make sure that the specified object is
> - * entirely contained by a single stack frame.
> - *
> - * Returns:
> - * 1 if within a frame
> - * -1 if placed across a frame boundary (or outside stack)
> - * 0 unable to determine (no frame pointers, etc)
> - */
> +#ifdef CONFIG_HARDENED_USERCOPY
This ifdef shouldn't be needed: the arch_within_stack_frames wasn't
designed to depend on it.
> +#ifdef CONFIG_FRAME_POINTER
> +int arch_within_stack_frames(const void * const stack,
> + const void * const stackend,
> + const void *obj, unsigned long len);
> +#else
> static inline int arch_within_stack_frames(const void * const stack,
> const void * const stackend,
> const void *obj, unsigned long len)
> {
> -#if defined(CONFIG_FRAME_POINTER)
> - const void *frame = NULL;
> - const void *oldframe;
> -
> - oldframe = __builtin_frame_address(1);
> - if (oldframe)
> - frame = __builtin_frame_address(2);
> - /*
> - * low ----------------------------------------------> high
> - * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> - * ^----------------^
> - * allow copies only within here
> - */
> - while (stack <= frame && frame < stackend) {
> - /*
> - * If obj + len extends past the last frame, this
> - * check won't pass and the next frame will be 0,
> - * causing us to bail out and correctly report
> - * the copy as invalid.
> - */
> - if (obj + len <= frame)
> - return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
> - oldframe = frame;
> - frame = *(const void * const *)frame;
> - }
> - return -1;
> -#else
> return 0;
> -#endif
> }
> +#endif /* CONFIG_FRAME_POINTER */
> +#endif /* CONFIG_HARDENED_USERCOPY */
> +
>
> #else /* !__ASSEMBLY__ */
>
> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> index b490878..96ce151 100644
> --- a/arch/x86/lib/usercopy.c
> +++ b/arch/x86/lib/usercopy.c
> @@ -9,6 +9,7 @@
>
> #include <asm/word-at-a-time.h>
> #include <linux/sched.h>
> +#include <asm/unwind.h>
>
> /*
> * We rely on the nested NMI work to allow atomic faults from the NMI path; the
> @@ -34,3 +35,45 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
> return ret;
> }
> EXPORT_SYMBOL_GPL(copy_from_user_nmi);
> +
> +#ifdef CONFIG_HARDENED_USERCOPY
Same thing: no need to check CONFIG_HARDENED_USERCOPY here: it should
be checking CONFIG_FRAME_POINTER instead.
> +/*
> + * Walks up the stack frames to make sure that the specified object is
> + * entirely contained by a single stack frame.
> + *
> + * Returns:
> + * 1 if within a frame
> + * -1 if placed across a frame boundary (or outside stack)
> + * 0 unable to determine (no frame pointers, etc)
> + */
> +int arch_within_stack_frames(const void * const stack,
> + const void * const stackend,
> + const void *obj, unsigned long len)
> +{
> + const void *frame = NULL;
> + const void *oldframe;
> +
> + oldframe = __builtin_frame_address(2);
> + if (oldframe)
> + frame = __builtin_frame_address(3);
> + /*
> + * low ----------------------------------------------> high
> + * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> + * ^----------------^
> + * allow copies only within here
> + */
> + while (stack <= frame && frame < stackend) {
> + /*
> + * If obj + len extends past the last frame, this
> + * check won't pass and the next frame will be 0,
> + * causing us to bail out and correctly report
> + * the copy as invalid.
> + */
> + if (obj + len <= frame)
> + return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
> + oldframe = frame;
> + frame = *(const void * const *)frame;
> + }
> + return -1;
> +}
> +#endif /* CONFIG_HARDENED_USERCOPY */
> --
> 2.7.4
>
-Kees
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 21:20 +0200 |
| Subject | Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| Message-ID | <s5q9P-5PW-1@gated-at.bofh.it> |
| In reply to | #1461340 |
On Fri, Aug 12, 2016 at 10:36:21AM -0700, Kees Cook wrote:
> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > When I tried to port arch_within_stack_frames() to use the new unwinder,
> > I got a nightmare include file "header soup" scenario when unwind.h was
> > included from thread_info.h. And anyway, I think thread_info.h isn't
> > really an appropriate place for this function. So move it to usercopy.c
> > instead.
> >
> > Since it relies on its parent's stack pointer, and the function is no
> > longer inlined, the arguments to the __builtin_frame_address() calls
> > have been incremented.
>
> Cool, looks good (minor change noted below). This patch might be a
> good place to drop this from mm/Makefile too:
>
> # Since __builtin_frame_address does work as used, disable the warning.
> CFLAGS_usercopy.o += $(call cc-disable-warning, frame-address)
>
> Since frame-address warnings have been disabled globally now since
> commit 124a3d88fa20 ("Disable "frame-address" warning").
Ok, I'll do that with the next patch (51/51) which removes the
__builtin_frame_address() calls.
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> > arch/x86/include/asm/thread_info.h | 46 ++++++++------------------------------
> > arch/x86/lib/usercopy.c | 43 +++++++++++++++++++++++++++++++++++
> > 2 files changed, 52 insertions(+), 37 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> > index 8b7c8d8e..fd849e6 100644
> > --- a/arch/x86/include/asm/thread_info.h
> > +++ b/arch/x86/include/asm/thread_info.h
> > @@ -176,49 +176,21 @@ static inline unsigned long current_stack_pointer(void)
> > return sp;
> > }
> >
> > -/*
> > - * Walks up the stack frames to make sure that the specified object is
> > - * entirely contained by a single stack frame.
> > - *
> > - * Returns:
> > - * 1 if within a frame
> > - * -1 if placed across a frame boundary (or outside stack)
> > - * 0 unable to determine (no frame pointers, etc)
> > - */
> > +#ifdef CONFIG_HARDENED_USERCOPY
>
> This ifdef shouldn't be needed: the arch_within_stack_frames wasn't
> designed to depend on it.
>
> > +#ifdef CONFIG_FRAME_POINTER
> > +int arch_within_stack_frames(const void * const stack,
> > + const void * const stackend,
> > + const void *obj, unsigned long len);
> > +#else
> > static inline int arch_within_stack_frames(const void * const stack,
> > const void * const stackend,
> > const void *obj, unsigned long len)
> > {
> > -#if defined(CONFIG_FRAME_POINTER)
> > - const void *frame = NULL;
> > - const void *oldframe;
> > -
> > - oldframe = __builtin_frame_address(1);
> > - if (oldframe)
> > - frame = __builtin_frame_address(2);
> > - /*
> > - * low ----------------------------------------------> high
> > - * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> > - * ^----------------^
> > - * allow copies only within here
> > - */
> > - while (stack <= frame && frame < stackend) {
> > - /*
> > - * If obj + len extends past the last frame, this
> > - * check won't pass and the next frame will be 0,
> > - * causing us to bail out and correctly report
> > - * the copy as invalid.
> > - */
> > - if (obj + len <= frame)
> > - return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
> > - oldframe = frame;
> > - frame = *(const void * const *)frame;
> > - }
> > - return -1;
> > -#else
> > return 0;
> > -#endif
> > }
> > +#endif /* CONFIG_FRAME_POINTER */
> > +#endif /* CONFIG_HARDENED_USERCOPY */
> > +
> >
> > #else /* !__ASSEMBLY__ */
> >
> > diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> > index b490878..96ce151 100644
> > --- a/arch/x86/lib/usercopy.c
> > +++ b/arch/x86/lib/usercopy.c
> > @@ -9,6 +9,7 @@
> >
> > #include <asm/word-at-a-time.h>
> > #include <linux/sched.h>
> > +#include <asm/unwind.h>
> >
> > /*
> > * We rely on the nested NMI work to allow atomic faults from the NMI path; the
> > @@ -34,3 +35,45 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(copy_from_user_nmi);
> > +
> > +#ifdef CONFIG_HARDENED_USERCOPY
>
> Same thing: no need to check CONFIG_HARDENED_USERCOPY here: it should
> be checking CONFIG_FRAME_POINTER instead.
Now that this function is no longer inlined and is instead compiled in
its own .c file, I was thinking that the tinyconfig folks would
appreciate not growing the text size if there's no reason to do so.
Keeping this #ifdef won't break anything, right?
Also I moved the CONFIG_FRAME_POINTER check to the header file so it
doesn't pollute the .c code.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-08-12 22:10 +0200 |
| Subject | Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| Message-ID | <s5qWd-6qZ-5@gated-at.bofh.it> |
| In reply to | #1461425 |
On Fri, Aug 12, 2016 at 12:12 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Aug 12, 2016 at 10:36:21AM -0700, Kees Cook wrote:
>> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > When I tried to port arch_within_stack_frames() to use the new unwinder,
>> > I got a nightmare include file "header soup" scenario when unwind.h was
>> > included from thread_info.h. And anyway, I think thread_info.h isn't
>> > really an appropriate place for this function. So move it to usercopy.c
>> > instead.
>> >
>> > Since it relies on its parent's stack pointer, and the function is no
>> > longer inlined, the arguments to the __builtin_frame_address() calls
>> > have been incremented.
>>
>> Cool, looks good (minor change noted below). This patch might be a
>> good place to drop this from mm/Makefile too:
>>
>> # Since __builtin_frame_address does work as used, disable the warning.
>> CFLAGS_usercopy.o += $(call cc-disable-warning, frame-address)
>>
>> Since frame-address warnings have been disabled globally now since
>> commit 124a3d88fa20 ("Disable "frame-address" warning").
>
> Ok, I'll do that with the next patch (51/51) which removes the
> __builtin_frame_address() calls.
>
>> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> > ---
>> > arch/x86/include/asm/thread_info.h | 46 ++++++++------------------------------
>> > arch/x86/lib/usercopy.c | 43 +++++++++++++++++++++++++++++++++++
>> > 2 files changed, 52 insertions(+), 37 deletions(-)
>> >
>> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>> > index 8b7c8d8e..fd849e6 100644
>> > --- a/arch/x86/include/asm/thread_info.h
>> > +++ b/arch/x86/include/asm/thread_info.h
>> > @@ -176,49 +176,21 @@ static inline unsigned long current_stack_pointer(void)
>> > return sp;
>> > }
>> >
>> > -/*
>> > - * Walks up the stack frames to make sure that the specified object is
>> > - * entirely contained by a single stack frame.
>> > - *
>> > - * Returns:
>> > - * 1 if within a frame
>> > - * -1 if placed across a frame boundary (or outside stack)
>> > - * 0 unable to determine (no frame pointers, etc)
>> > - */
>> > +#ifdef CONFIG_HARDENED_USERCOPY
>>
>> This ifdef shouldn't be needed: the arch_within_stack_frames wasn't
>> designed to depend on it.
>>
>> > +#ifdef CONFIG_FRAME_POINTER
>> > +int arch_within_stack_frames(const void * const stack,
>> > + const void * const stackend,
>> > + const void *obj, unsigned long len);
>> > +#else
>> > static inline int arch_within_stack_frames(const void * const stack,
>> > const void * const stackend,
>> > const void *obj, unsigned long len)
>> > {
>> > -#if defined(CONFIG_FRAME_POINTER)
>> > - const void *frame = NULL;
>> > - const void *oldframe;
>> > -
>> > - oldframe = __builtin_frame_address(1);
>> > - if (oldframe)
>> > - frame = __builtin_frame_address(2);
>> > - /*
>> > - * low ----------------------------------------------> high
>> > - * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> > - * ^----------------^
>> > - * allow copies only within here
>> > - */
>> > - while (stack <= frame && frame < stackend) {
>> > - /*
>> > - * If obj + len extends past the last frame, this
>> > - * check won't pass and the next frame will be 0,
>> > - * causing us to bail out and correctly report
>> > - * the copy as invalid.
>> > - */
>> > - if (obj + len <= frame)
>> > - return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
>> > - oldframe = frame;
>> > - frame = *(const void * const *)frame;
>> > - }
>> > - return -1;
>> > -#else
>> > return 0;
>> > -#endif
>> > }
>> > +#endif /* CONFIG_FRAME_POINTER */
>> > +#endif /* CONFIG_HARDENED_USERCOPY */
>> > +
>> >
>> > #else /* !__ASSEMBLY__ */
>> >
>> > diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
>> > index b490878..96ce151 100644
>> > --- a/arch/x86/lib/usercopy.c
>> > +++ b/arch/x86/lib/usercopy.c
>> > @@ -9,6 +9,7 @@
>> >
>> > #include <asm/word-at-a-time.h>
>> > #include <linux/sched.h>
>> > +#include <asm/unwind.h>
>> >
>> > /*
>> > * We rely on the nested NMI work to allow atomic faults from the NMI path; the
>> > @@ -34,3 +35,45 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
>> > return ret;
>> > }
>> > EXPORT_SYMBOL_GPL(copy_from_user_nmi);
>> > +
>> > +#ifdef CONFIG_HARDENED_USERCOPY
>>
>> Same thing: no need to check CONFIG_HARDENED_USERCOPY here: it should
>> be checking CONFIG_FRAME_POINTER instead.
>
> Now that this function is no longer inlined and is instead compiled in
> its own .c file, I was thinking that the tinyconfig folks would
> appreciate not growing the text size if there's no reason to do so.
> Keeping this #ifdef won't break anything, right?
Hrm, well, I guess not, but it means if anyone else wants to use it
they have to remove the ifdef. I guess I don't object that much. :P
> Also I moved the CONFIG_FRAME_POINTER check to the header file so it
> doesn't pollute the .c code.
Right, but if FRAME_POINTER=n and HARDENED_USERCOPY=y you'll get a
build error about it being both in the .h and the .c file, if I'm
reading that correctly.
-Kees
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 22:40 +0200 |
| Subject | Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| Message-ID | <s5rpf-6B6-13@gated-at.bofh.it> |
| In reply to | #1461443 |
On Fri, Aug 12, 2016 at 01:06:41PM -0700, Kees Cook wrote:
> On Fri, Aug 12, 2016 at 12:12 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Fri, Aug 12, 2016 at 10:36:21AM -0700, Kees Cook wrote:
> >> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > When I tried to port arch_within_stack_frames() to use the new unwinder,
> >> > I got a nightmare include file "header soup" scenario when unwind.h was
> >> > included from thread_info.h. And anyway, I think thread_info.h isn't
> >> > really an appropriate place for this function. So move it to usercopy.c
> >> > instead.
> >> >
> >> > Since it relies on its parent's stack pointer, and the function is no
> >> > longer inlined, the arguments to the __builtin_frame_address() calls
> >> > have been incremented.
> >>
> >> Cool, looks good (minor change noted below). This patch might be a
> >> good place to drop this from mm/Makefile too:
> >>
> >> # Since __builtin_frame_address does work as used, disable the warning.
> >> CFLAGS_usercopy.o += $(call cc-disable-warning, frame-address)
> >>
> >> Since frame-address warnings have been disabled globally now since
> >> commit 124a3d88fa20 ("Disable "frame-address" warning").
> >
> > Ok, I'll do that with the next patch (51/51) which removes the
> > __builtin_frame_address() calls.
> >
> >> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> >> > ---
> >> > arch/x86/include/asm/thread_info.h | 46 ++++++++------------------------------
> >> > arch/x86/lib/usercopy.c | 43 +++++++++++++++++++++++++++++++++++
> >> > 2 files changed, 52 insertions(+), 37 deletions(-)
> >> >
> >> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> >> > index 8b7c8d8e..fd849e6 100644
> >> > --- a/arch/x86/include/asm/thread_info.h
> >> > +++ b/arch/x86/include/asm/thread_info.h
> >> > @@ -176,49 +176,21 @@ static inline unsigned long current_stack_pointer(void)
> >> > return sp;
> >> > }
> >> >
> >> > -/*
> >> > - * Walks up the stack frames to make sure that the specified object is
> >> > - * entirely contained by a single stack frame.
> >> > - *
> >> > - * Returns:
> >> > - * 1 if within a frame
> >> > - * -1 if placed across a frame boundary (or outside stack)
> >> > - * 0 unable to determine (no frame pointers, etc)
> >> > - */
> >> > +#ifdef CONFIG_HARDENED_USERCOPY
> >>
> >> This ifdef shouldn't be needed: the arch_within_stack_frames wasn't
> >> designed to depend on it.
> >>
> >> > +#ifdef CONFIG_FRAME_POINTER
> >> > +int arch_within_stack_frames(const void * const stack,
> >> > + const void * const stackend,
> >> > + const void *obj, unsigned long len);
> >> > +#else
> >> > static inline int arch_within_stack_frames(const void * const stack,
> >> > const void * const stackend,
> >> > const void *obj, unsigned long len)
> >> > {
> >> > -#if defined(CONFIG_FRAME_POINTER)
> >> > - const void *frame = NULL;
> >> > - const void *oldframe;
> >> > -
> >> > - oldframe = __builtin_frame_address(1);
> >> > - if (oldframe)
> >> > - frame = __builtin_frame_address(2);
> >> > - /*
> >> > - * low ----------------------------------------------> high
> >> > - * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> >> > - * ^----------------^
> >> > - * allow copies only within here
> >> > - */
> >> > - while (stack <= frame && frame < stackend) {
> >> > - /*
> >> > - * If obj + len extends past the last frame, this
> >> > - * check won't pass and the next frame will be 0,
> >> > - * causing us to bail out and correctly report
> >> > - * the copy as invalid.
> >> > - */
> >> > - if (obj + len <= frame)
> >> > - return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
> >> > - oldframe = frame;
> >> > - frame = *(const void * const *)frame;
> >> > - }
> >> > - return -1;
> >> > -#else
> >> > return 0;
> >> > -#endif
> >> > }
> >> > +#endif /* CONFIG_FRAME_POINTER */
> >> > +#endif /* CONFIG_HARDENED_USERCOPY */
> >> > +
> >> >
> >> > #else /* !__ASSEMBLY__ */
> >> >
> >> > diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> >> > index b490878..96ce151 100644
> >> > --- a/arch/x86/lib/usercopy.c
> >> > +++ b/arch/x86/lib/usercopy.c
> >> > @@ -9,6 +9,7 @@
> >> >
> >> > #include <asm/word-at-a-time.h>
> >> > #include <linux/sched.h>
> >> > +#include <asm/unwind.h>
> >> >
> >> > /*
> >> > * We rely on the nested NMI work to allow atomic faults from the NMI path; the
> >> > @@ -34,3 +35,45 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
> >> > return ret;
> >> > }
> >> > EXPORT_SYMBOL_GPL(copy_from_user_nmi);
> >> > +
> >> > +#ifdef CONFIG_HARDENED_USERCOPY
> >>
> >> Same thing: no need to check CONFIG_HARDENED_USERCOPY here: it should
> >> be checking CONFIG_FRAME_POINTER instead.
> >
> > Now that this function is no longer inlined and is instead compiled in
> > its own .c file, I was thinking that the tinyconfig folks would
> > appreciate not growing the text size if there's no reason to do so.
> > Keeping this #ifdef won't break anything, right?
>
> Hrm, well, I guess not, but it means if anyone else wants to use it
> they have to remove the ifdef. I guess I don't object that much. :P
Ah. Do you expect other uses for it?
> > Also I moved the CONFIG_FRAME_POINTER check to the header file so it
> > doesn't pollute the .c code.
>
> Right, but if FRAME_POINTER=n and HARDENED_USERCOPY=y you'll get a
> build error about it being both in the .h and the .c file, if I'm
> reading that correctly.
Oh, right.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-08-12 22:50 +0200 |
| Subject | Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| Message-ID | <s5ryV-6EZ-3@gated-at.bofh.it> |
| In reply to | #1461450 |
On Fri, Aug 12, 2016 at 1:36 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Aug 12, 2016 at 01:06:41PM -0700, Kees Cook wrote:
>> On Fri, Aug 12, 2016 at 12:12 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > On Fri, Aug 12, 2016 at 10:36:21AM -0700, Kees Cook wrote:
>> >> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> >> > When I tried to port arch_within_stack_frames() to use the new unwinder,
>> >> > I got a nightmare include file "header soup" scenario when unwind.h was
>> >> > included from thread_info.h. And anyway, I think thread_info.h isn't
>> >> > really an appropriate place for this function. So move it to usercopy.c
>> >> > instead.
>> >> >
>> >> > Since it relies on its parent's stack pointer, and the function is no
>> >> > longer inlined, the arguments to the __builtin_frame_address() calls
>> >> > have been incremented.
>> >>
>> >> Cool, looks good (minor change noted below). This patch might be a
>> >> good place to drop this from mm/Makefile too:
>> >>
>> >> # Since __builtin_frame_address does work as used, disable the warning.
>> >> CFLAGS_usercopy.o += $(call cc-disable-warning, frame-address)
>> >>
>> >> Since frame-address warnings have been disabled globally now since
>> >> commit 124a3d88fa20 ("Disable "frame-address" warning").
>> >
>> > Ok, I'll do that with the next patch (51/51) which removes the
>> > __builtin_frame_address() calls.
>> >
>> >> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> >> > ---
>> >> > arch/x86/include/asm/thread_info.h | 46 ++++++++------------------------------
>> >> > arch/x86/lib/usercopy.c | 43 +++++++++++++++++++++++++++++++++++
>> >> > 2 files changed, 52 insertions(+), 37 deletions(-)
>> >> >
>> >> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>> >> > index 8b7c8d8e..fd849e6 100644
>> >> > --- a/arch/x86/include/asm/thread_info.h
>> >> > +++ b/arch/x86/include/asm/thread_info.h
>> >> > @@ -176,49 +176,21 @@ static inline unsigned long current_stack_pointer(void)
>> >> > return sp;
>> >> > }
>> >> >
>> >> > -/*
>> >> > - * Walks up the stack frames to make sure that the specified object is
>> >> > - * entirely contained by a single stack frame.
>> >> > - *
>> >> > - * Returns:
>> >> > - * 1 if within a frame
>> >> > - * -1 if placed across a frame boundary (or outside stack)
>> >> > - * 0 unable to determine (no frame pointers, etc)
>> >> > - */
>> >> > +#ifdef CONFIG_HARDENED_USERCOPY
>> >>
>> >> This ifdef shouldn't be needed: the arch_within_stack_frames wasn't
>> >> designed to depend on it.
>> >>
>> >> > +#ifdef CONFIG_FRAME_POINTER
>> >> > +int arch_within_stack_frames(const void * const stack,
>> >> > + const void * const stackend,
>> >> > + const void *obj, unsigned long len);
>> >> > +#else
>> >> > static inline int arch_within_stack_frames(const void * const stack,
>> >> > const void * const stackend,
>> >> > const void *obj, unsigned long len)
>> >> > {
>> >> > -#if defined(CONFIG_FRAME_POINTER)
>> >> > - const void *frame = NULL;
>> >> > - const void *oldframe;
>> >> > -
>> >> > - oldframe = __builtin_frame_address(1);
>> >> > - if (oldframe)
>> >> > - frame = __builtin_frame_address(2);
>> >> > - /*
>> >> > - * low ----------------------------------------------> high
>> >> > - * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> >> > - * ^----------------^
>> >> > - * allow copies only within here
>> >> > - */
>> >> > - while (stack <= frame && frame < stackend) {
>> >> > - /*
>> >> > - * If obj + len extends past the last frame, this
>> >> > - * check won't pass and the next frame will be 0,
>> >> > - * causing us to bail out and correctly report
>> >> > - * the copy as invalid.
>> >> > - */
>> >> > - if (obj + len <= frame)
>> >> > - return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
>> >> > - oldframe = frame;
>> >> > - frame = *(const void * const *)frame;
>> >> > - }
>> >> > - return -1;
>> >> > -#else
>> >> > return 0;
>> >> > -#endif
>> >> > }
>> >> > +#endif /* CONFIG_FRAME_POINTER */
>> >> > +#endif /* CONFIG_HARDENED_USERCOPY */
>> >> > +
>> >> >
>> >> > #else /* !__ASSEMBLY__ */
>> >> >
>> >> > diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
>> >> > index b490878..96ce151 100644
>> >> > --- a/arch/x86/lib/usercopy.c
>> >> > +++ b/arch/x86/lib/usercopy.c
>> >> > @@ -9,6 +9,7 @@
>> >> >
>> >> > #include <asm/word-at-a-time.h>
>> >> > #include <linux/sched.h>
>> >> > +#include <asm/unwind.h>
>> >> >
>> >> > /*
>> >> > * We rely on the nested NMI work to allow atomic faults from the NMI path; the
>> >> > @@ -34,3 +35,45 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
>> >> > return ret;
>> >> > }
>> >> > EXPORT_SYMBOL_GPL(copy_from_user_nmi);
>> >> > +
>> >> > +#ifdef CONFIG_HARDENED_USERCOPY
>> >>
>> >> Same thing: no need to check CONFIG_HARDENED_USERCOPY here: it should
>> >> be checking CONFIG_FRAME_POINTER instead.
>> >
>> > Now that this function is no longer inlined and is instead compiled in
>> > its own .c file, I was thinking that the tinyconfig folks would
>> > appreciate not growing the text size if there's no reason to do so.
>> > Keeping this #ifdef won't break anything, right?
>>
>> Hrm, well, I guess not, but it means if anyone else wants to use it
>> they have to remove the ifdef. I guess I don't object that much. :P
>
> Ah. Do you expect other uses for it?
None that I'm aware of. :)
-Kees
>
>> > Also I moved the CONFIG_FRAME_POINTER check to the header file so it
>> > doesn't pollute the .c code.
>>
>> Right, but if FRAME_POINTER=n and HARDENED_USERCOPY=y you'll get a
>> build error about it being both in the .h and the .c file, if I'm
>> reading that correctly.
>
> Oh, right.
>
> --
> Josh
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 44/51] x86/dumpstack: print any pt_regs found on the stack |
| Message-ID | <s5lMT-2Ra-69@gated-at.bofh.it> |
| In reply to | #1461224 |
Now that we can find pt_regs registers on the stack, print them. Here's
an example of what it looks like:
Call Trace:
<IRQ>
[<ffffffff8144b793>] dump_stack+0x86/0xc3
[<ffffffff81142c73>] hrtimer_interrupt+0xb3/0x1c0
[<ffffffff8105eb86>] local_apic_timer_interrupt+0x36/0x60
[<ffffffff818b27cd>] smp_apic_timer_interrupt+0x3d/0x50
[<ffffffff818b06ee>] apic_timer_interrupt+0x9e/0xb0
RIP: 0010:[<ffffffff818aef43>] [<ffffffff818aef43>] _raw_spin_unlock_irq+0x33/0x60
RSP: 0018:ffff880079c4f760 EFLAGS: 00000202
RAX: ffff880078738000 RBX: ffff88007d3da0c0 RCX: 0000000000000007
RDX: 0000000000006d78 RSI: ffff8800787388f0 RDI: ffff880078738000
RBP: ffff880079c4f768 R08: 0000002199088f38 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff81e0d540
R13: ffff8800369fb700 R14: 0000000000000000 R15: ffff880078738000
<EOI>
[<ffffffff810e1f14>] finish_task_switch+0xb4/0x250
[<ffffffff810e1ed6>] ? finish_task_switch+0x76/0x250
[<ffffffff818a7b61>] __schedule+0x3e1/0xb20
...
[<ffffffff810759c8>] trace_do_page_fault+0x58/0x2c0
[<ffffffff8106f7dc>] do_async_page_fault+0x2c/0xa0
[<ffffffff818b1dd8>] async_page_fault+0x28/0x30
RIP: 0010:[<ffffffff8145b062>] [<ffffffff8145b062>] __clear_user+0x42/0x70
RSP: 0018:ffff880079c4fd38 EFLAGS: 00010202
RAX: 0000000000000000 RBX: 0000000000000138 RCX: 0000000000000138
RDX: 0000000000000000 RSI: 0000000000000008 RDI: 000000000061b640
RBP: ffff880079c4fd48 R08: 0000002198feefd7 R09: ffffffff82a40928
R10: 0000000000000001 R11: 0000000000000000 R12: 000000000061b640
R13: 0000000000000000 R14: ffff880079c50000 R15: ffff8800791d7400
[<ffffffff8145b043>] ? __clear_user+0x23/0x70
[<ffffffff8145b0fb>] clear_user+0x2b/0x40
[<ffffffff812fbda2>] load_elf_binary+0x1472/0x1750
[<ffffffff8129a591>] search_binary_handler+0xa1/0x200
[<ffffffff8129b69b>] do_execveat_common.isra.36+0x6cb/0x9f0
[<ffffffff8129b5f3>] ? do_execveat_common.isra.36+0x623/0x9f0
[<ffffffff8129bcaa>] SyS_execve+0x3a/0x50
[<ffffffff81003f5c>] do_syscall_64+0x6c/0x1e0
[<ffffffff818afa3f>] entry_SYSCALL64_slow_path+0x25/0x25
RIP: 0033:[<00007fd2e2f2e537>] [<00007fd2e2f2e537>] 0x7fd2e2f2e537
RSP: 002b:00007ffc449c5fc8 EFLAGS: 00000246
RAX: ffffffffffffffda RBX: 00007ffc449c8860 RCX: 00007fd2e2f2e537
RDX: 000000000127cc40 RSI: 00007ffc449c8860 RDI: 00007ffc449c6029
RBP: 00007ffc449c60b0 R08: 65726f632d667265 R09: 00007ffc449c5e20
R10: 00000000000005a7 R11: 0000000000000246 R12: 000000000127cc40
R13: 000000000127ce05 R14: 00007ffc449c6029 R15: 000000000127ce01
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/kernel/dumpstack.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 92a2f82..cae70c1 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -86,7 +86,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
* - softirq stack
* - hardirq stack
*/
- for (; stack; stack = stack_info.next_sp) {
+ for (regs = NULL; stack; stack = stack_info.next_sp) {
const char *str_begin, *str_end;
/*
@@ -123,6 +123,15 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
if (!__kernel_text_address(addr))
continue;
+ /*
+ * Don't print regs->ip again if it was already printed
+ * by __show_regs() below.
+ */
+ if (regs && stack == ®s->ip) {
+ unwind_next_frame(&state);
+ continue;
+ }
+
if (stack == ret_addr_p)
reliable = 1;
@@ -150,6 +159,11 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
* of the addresses will just be printed as unreliable.
*/
unwind_next_frame(&state);
+
+ /* if the frame has entry regs, print them */
+ regs = unwind_get_entry_regs(&state);
+ if (regs)
+ __show_regs(regs, 0);
}
if (str_end)
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-08-14 10:50 +0200 |
| Subject | Re: [PATCH v3 44/51] x86/dumpstack: print any pt_regs found on the stack |
| Message-ID | <s5Zhg-5wm-23@gated-at.bofh.it> |
| In reply to | #1461233 |
On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > Now that we can find pt_regs registers on the stack, print them. Here's > an example of what it looks like: > > Call Trace: > <IRQ> > [<ffffffff8144b793>] dump_stack+0x86/0xc3 > [<ffffffff81142c73>] hrtimer_interrupt+0xb3/0x1c0 > [<ffffffff8105eb86>] local_apic_timer_interrupt+0x36/0x60 > [<ffffffff818b27cd>] smp_apic_timer_interrupt+0x3d/0x50 > [<ffffffff818b06ee>] apic_timer_interrupt+0x9e/0xb0 > RIP: 0010:[<ffffffff818aef43>] [<ffffffff818aef43>] _raw_spin_unlock_irq+0x33/0x60 > RSP: 0018:ffff880079c4f760 EFLAGS: 00000202 > RAX: ffff880078738000 RBX: ffff88007d3da0c0 RCX: 0000000000000007 > RDX: 0000000000006d78 RSI: ffff8800787388f0 RDI: ffff880078738000 > RBP: ffff880079c4f768 R08: 0000002199088f38 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff81e0d540 > R13: ffff8800369fb700 R14: 0000000000000000 R15: ffff880078738000 > <EOI> > [<ffffffff810e1f14>] finish_task_switch+0xb4/0x250 > [<ffffffff810e1ed6>] ? finish_task_switch+0x76/0x250 > [<ffffffff818a7b61>] __schedule+0x3e1/0xb20 > ... > [<ffffffff810759c8>] trace_do_page_fault+0x58/0x2c0 > [<ffffffff8106f7dc>] do_async_page_fault+0x2c/0xa0 > [<ffffffff818b1dd8>] async_page_fault+0x28/0x30 > RIP: 0010:[<ffffffff8145b062>] [<ffffffff8145b062>] __clear_user+0x42/0x70 > RSP: 0018:ffff880079c4fd38 EFLAGS: 00010202 > RAX: 0000000000000000 RBX: 0000000000000138 RCX: 0000000000000138 > RDX: 0000000000000000 RSI: 0000000000000008 RDI: 000000000061b640 > RBP: ffff880079c4fd48 R08: 0000002198feefd7 R09: ffffffff82a40928 > R10: 0000000000000001 R11: 0000000000000000 R12: 000000000061b640 > R13: 0000000000000000 R14: ffff880079c50000 R15: ffff8800791d7400 > [<ffffffff8145b043>] ? __clear_user+0x23/0x70 > [<ffffffff8145b0fb>] clear_user+0x2b/0x40 > [<ffffffff812fbda2>] load_elf_binary+0x1472/0x1750 > [<ffffffff8129a591>] search_binary_handler+0xa1/0x200 > [<ffffffff8129b69b>] do_execveat_common.isra.36+0x6cb/0x9f0 > [<ffffffff8129b5f3>] ? do_execveat_common.isra.36+0x623/0x9f0 > [<ffffffff8129bcaa>] SyS_execve+0x3a/0x50 > [<ffffffff81003f5c>] do_syscall_64+0x6c/0x1e0 > [<ffffffff818afa3f>] entry_SYSCALL64_slow_path+0x25/0x25 > RIP: 0033:[<00007fd2e2f2e537>] [<00007fd2e2f2e537>] 0x7fd2e2f2e537 > RSP: 002b:00007ffc449c5fc8 EFLAGS: 00000246 > RAX: ffffffffffffffda RBX: 00007ffc449c8860 RCX: 00007fd2e2f2e537 > RDX: 000000000127cc40 RSI: 00007ffc449c8860 RDI: 00007ffc449c6029 > RBP: 00007ffc449c60b0 R08: 65726f632d667265 R09: 00007ffc449c5e20 > R10: 00000000000005a7 R11: 0000000000000246 R12: 000000000127cc40 > R13: 000000000127ce05 R14: 00007ffc449c6029 R15: 000000000127ce01 I really like this, and I think it'll be quite useful for future debugging. Some day I want to teach this thing to print gsbase as well, but it's not quite obvious to me how to do that except maybe by using DWARF and a lot of special case code. --Andy
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 12/51] x86: move _stext marker to before head code |
| Message-ID | <s5lMT-2Ra-63@gated-at.bofh.it> |
| In reply to | #1461224 |
When core_kernel_text() is used to determine whether an address on a
task's stack trace is a kernel text address, it incorrectly returns
false for early text addresses for the head code between the _text and
_stext markers.
Head code is text code too, so mark it as such. This seems to match the
intent of other users of the _stext symbol, and it also seems consistent
with what other architectures are already doing.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/kernel/vmlinux.lds.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 9297a00..1d9b636 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -91,10 +91,10 @@ SECTIONS
/* Text and read-only data */
.text : AT(ADDR(.text) - LOAD_OFFSET) {
_text = .;
+ _stext = .;
/* bootstrapping code */
HEAD_TEXT
. = ALIGN(8);
- _stext = .;
TEXT_TEXT
SCHED_TEXT
LOCK_TEXT
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-12 16:40 +0200 |
| Subject | [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() |
| Message-ID | <s5lMT-2Ra-65@gated-at.bofh.it> |
| In reply to | #1461224 |
in_exception_stack() does some bad, bad things just so the unwinder can
print different values for different areas of the debug exception stack.
There's no need to clarify where exactly on the stack it is. Just print
"#DB" and be done with it.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/kernel/dumpstack_64.c | 89 ++++++++++++------------------------------
1 file changed, 26 insertions(+), 63 deletions(-)
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index f1b843a..69f6ba2 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -16,83 +16,46 @@
#include <asm/stacktrace.h>
+static char *exception_stack_names[N_EXCEPTION_STACKS] = {
+ [ DOUBLEFAULT_STACK-1 ] = "#DF",
+ [ NMI_STACK-1 ] = "NMI",
+ [ DEBUG_STACK-1 ] = "#DB",
+ [ MCE_STACK-1 ] = "#MC",
+};
-#define N_EXCEPTION_STACKS_END \
- (N_EXCEPTION_STACKS + DEBUG_STKSZ/EXCEPTION_STKSZ - 2)
-
-static char x86_stack_ids[][8] = {
- [ DEBUG_STACK-1 ] = "#DB",
- [ NMI_STACK-1 ] = "NMI",
- [ DOUBLEFAULT_STACK-1 ] = "#DF",
- [ MCE_STACK-1 ] = "#MC",
-#if DEBUG_STKSZ > EXCEPTION_STKSZ
- [ N_EXCEPTION_STACKS ...
- N_EXCEPTION_STACKS_END ] = "#DB[?]"
-#endif
+static unsigned long exception_stack_sizes[N_EXCEPTION_STACKS] = {
+ [0 ... N_EXCEPTION_STACKS - 1] = EXCEPTION_STKSZ,
+ [DEBUG_STACK - 1] = DEBUG_STKSZ
};
static unsigned long *in_exception_stack(unsigned long stack, unsigned *usedp,
char **idp)
{
+ unsigned long begin, end;
unsigned k;
- /*
- * Iterate over all exception stacks, and figure out whether
- * 'stack' is in one of them:
- */
+ BUILD_BUG_ON(N_EXCEPTION_STACKS != 4);
+
for (k = 0; k < N_EXCEPTION_STACKS; k++) {
- unsigned long end = raw_cpu_ptr(&orig_ist)->ist[k];
- /*
- * Is 'stack' above this exception frame's end?
- * If yes then skip to the next frame.
- */
- if (stack >= end)
+ end = raw_cpu_ptr(&orig_ist)->ist[k];
+ begin = end - exception_stack_sizes[k];
+
+ if (stack < begin || stack >= end)
continue;
+
/*
- * Is 'stack' above this exception frame's start address?
- * If yes then we found the right frame.
- */
- if (stack >= end - EXCEPTION_STKSZ) {
- /*
- * Make sure we only iterate through an exception
- * stack once. If it comes up for the second time
- * then there's something wrong going on - just
- * break out and return NULL:
- */
- if (*usedp & (1U << k))
- break;
- *usedp |= 1U << k;
- *idp = x86_stack_ids[k];
- return (unsigned long *)end;
- }
- /*
- * If this is a debug stack, and if it has a larger size than
- * the usual exception stacks, then 'stack' might still
- * be within the lower portion of the debug stack:
+ * Make sure we only iterate through an exception stack once.
+ * If it comes up for the second time then there's something
+ * wrong going on - just break and return NULL:
*/
-#if DEBUG_STKSZ > EXCEPTION_STKSZ
- if (k == DEBUG_STACK - 1 && stack >= end - DEBUG_STKSZ) {
- unsigned j = N_EXCEPTION_STACKS - 1;
+ if (*usedp & (1U << k))
+ break;
+ *usedp |= 1U << k;
- /*
- * Black magic. A large debug stack is composed of
- * multiple exception stack entries, which we
- * iterate through now. Dont look:
- */
- do {
- ++j;
- end -= EXCEPTION_STKSZ;
- x86_stack_ids[j][4] = '1' +
- (j - N_EXCEPTION_STACKS);
- } while (stack < end - EXCEPTION_STKSZ);
- if (*usedp & (1U << j))
- break;
- *usedp |= 1U << j;
- *idp = x86_stack_ids[j];
- return (unsigned long *)end;
- }
-#endif
+ *idp = exception_stack_names[k];
+ return (unsigned long *)end;
}
+
return NULL;
}
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-08-14 10:50 +0200 |
| Subject | Re: [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() |
| Message-ID | <s5Zhf-5wm-7@gated-at.bofh.it> |
| In reply to | #1461235 |
On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > in_exception_stack() does some bad, bad things just so the unwinder can > print different values for different areas of the debug exception stack. > > There's no need to clarify where exactly on the stack it is. Just print > "#DB" and be done with it. I'm okay with the printing part, but you're also using this to prevent infinite looping. Will this cause the unwind to fail if we go debug -> page fault -> debug or similar? (Or whatever actually uses the deeper debug stacks? I figured this out once and then forgot exactly what's going on. I really need to dust off my patches that stop using IST for #DB.) --Andy
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-08-15 17:40 +0200 |
| Subject | Re: [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() |
| Message-ID | <s6s9A-7m4-37@gated-at.bofh.it> |
| In reply to | #1461704 |
On Sun, Aug 14, 2016 at 12:48:15AM -0700, Andy Lutomirski wrote: > On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > in_exception_stack() does some bad, bad things just so the unwinder can > > print different values for different areas of the debug exception stack. > > > > There's no need to clarify where exactly on the stack it is. Just print > > "#DB" and be done with it. > > I'm okay with the printing part, but you're also using this to prevent > infinite looping. Will this cause the unwind to fail if we go debug > -> page fault -> debug or similar? Yes, but that behavior already existed. This patch doesn't change that; it just makes it clearer what's going on. -- Josh
[toc] | [prev] | [next] | [standalone]
Page 1 of 4 [1] 2 3 4 Next page →
Back to top | Article view | linux.kernel
csiph-web