[PATCH v2 1/5] x86/stacktrace: do now unwind after user regs

[PATCH v2 1/5] x86/stacktrace: do now unwind after user regs

[Jiri Slaby]

Josh pointed out, that there is no way a frame can be after user regs.
So remove the last unwind and the check.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/stacktrace.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 77835bc021c7..b088d4f9e43e 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -113,15 +113,6 @@ __save_stack_trace_reliable(struct stack_trace *trace,
 			if (!user_mode(regs))
 				return -EINVAL;
 
-			/*
-			 * The last frame contains the user mode syscall
-			 * pt_regs.  Skip it and finish the unwind.
-			 */
-			unwind_next_frame(&state);
-			if (!unwind_done(&state)) {
-				STACKTRACE_DUMP_ONCE(task);
-				return -EINVAL;
-			}
 			break;
 		}
 
-- 
2.15.1

[PATCH v2 2/5] x86/stacktrace: remove unwind_state->error

[Jiri Slaby]

Reorganize the unwinding in __save_stack_trace_reliable, so that we
don't need to set another variable -- unwind_state->error. In case,
unwinding fails, we fail after the for loop too. The only way to escape
the loop successfully is via the 'if (user_mode(regs))' check now.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/unwind.h  |  6 ------
 arch/x86/kernel/stacktrace.c   | 15 ++++++---------
 arch/x86/kernel/unwind_frame.c |  2 --
 3 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
index 5be2fb23825a..6f90295e6547 100644
--- a/arch/x86/include/asm/unwind.h
+++ b/arch/x86/include/asm/unwind.h
@@ -15,7 +15,6 @@ struct unwind_state {
 	unsigned long stack_mask;
 	struct task_struct *task;
 	int graph_idx;
-	bool error;
 #if defined(CONFIG_UNWINDER_ORC)
 	bool signal, full_regs;
 	unsigned long sp, bp, ip;
@@ -40,11 +39,6 @@ static inline bool unwind_done(struct unwind_state *state)
 	return state->stack_info.type == STACK_TYPE_UNKNOWN;
 }
 
-static inline bool unwind_error(struct unwind_state *state)
-{
-	return state->error;
-}
-
 static inline
 void unwind_start(struct unwind_state *state, struct task_struct *task,
 		  struct pt_regs *regs, unsigned long *first_frame)
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index b088d4f9e43e..9193607e3ead 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -104,16 +104,16 @@ __save_stack_trace_reliable(struct stack_trace *trace,
 
 		regs = unwind_get_entry_regs(&state);
 		if (regs) {
+			if (user_mode(regs))
+				goto success;
+
 			/*
 			 * Kernel mode registers on the stack indicate an
 			 * in-kernel interrupt or exception (e.g., preemption
 			 * or a page fault), which can make frame pointers
 			 * unreliable.
 			 */
-			if (!user_mode(regs))
-				return -EINVAL;
-
-			break;
+			return -EINVAL;
 		}
 
 		addr = unwind_get_return_address(&state);
@@ -132,12 +132,9 @@ __save_stack_trace_reliable(struct stack_trace *trace,
 			return -EINVAL;
 	}
 
-	/* Check for stack corruption */
-	if (unwind_error(&state)) {
-		STACKTRACE_DUMP_ONCE(task);
-		return -EINVAL;
-	}
+	return -EINVAL;
 
+success:
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 
diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 3dc26f95d46e..0c08a56adabe 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -332,8 +332,6 @@ bool unwind_next_frame(struct unwind_state *state)
 	return true;
 
 bad_address:
-	state->error = true;
-
 	/*
 	 * When unwinding a non-current task, the task might actually be
 	 * running on another CPU, in which case it could be modifying its
-- 
2.15.1

[PATCH v2 3/5] x86/stacktrace: remove STACKTRACE_DUMP_ONCE from __save_stack_trace_reliable

[Jiri Slaby]

The stack unwinding can sometimes fail yet. Especially with the
generated debug info. So do not yell at users -- live patching (the only
user of this interface) will inform the user about the failure
gracefully.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/stacktrace.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 9193607e3ead..9da8af2922b6 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -123,10 +123,8 @@ __save_stack_trace_reliable(struct stack_trace *trace,
 		 * generated code which __kernel_text_address() doesn't know
 		 * about.
 		 */
-		if (!addr) {
-			STACKTRACE_DUMP_ONCE(task);
+		if (!addr)
 			return -EINVAL;
-		}
 
 		if (save_stack_address(trace, addr, false))
 			return -EINVAL;
-- 
2.15.1

[PATCH v2 4/5] x86/stacktrace: do not fail for ORC with regs on stack

[Jiri Slaby]

save_stack_trace_reliable now returns "non reliable" when there are
kernel pt_regs on stack. This means an interrupt or exception happened
somewhere down the route. It is a problem for frame pointer unwinder,
because the frame might not have been set up yet when the irq happened,
so the unwinder might fail to unwind from the interrupted function.

With ORC, this is not a problem, as ORC has out-of-band data. We can
find ORC data even for the IP in the interrupted function and always
unwind one level up reliably.

So lift the check to apply only when CONFIG_FRAME_POINTER is enabled.

[v2]
- rewrite the code in favor of Josh's suggestions

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/stacktrace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 9da8af2922b6..45915bd01692 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -113,7 +113,8 @@ __save_stack_trace_reliable(struct stack_trace *trace,
 			 * or a page fault), which can make frame pointers
 			 * unreliable.
 			 */
-			return -EINVAL;
+			if (IS_ENABLED(CONFIG_FRAME_POINTER))
+				return -EINVAL;
 		}
 
 		addr = unwind_get_return_address(&state);
-- 
2.15.1

[PATCH v2 5/5] x86/stacktrace: orc, mark it as reliable

[Jiri Slaby]

In SUSE, we need a reliable stack unwinder for kernel live patching, but
we do not want to enable frame pointers for performance reasons. So
after the previous patches to make the ORC reliable, mark ORC as a
reliable stack unwinder on x86.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index bb757400869e..54d4f67ad4bf 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -172,7 +172,7 @@ config X86
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_RCU_TABLE_FREE
 	select HAVE_REGS_AND_STACK_ACCESS_API
-	select HAVE_RELIABLE_STACKTRACE		if X86_64 && UNWINDER_FRAME_POINTER && STACK_VALIDATION
+	select HAVE_RELIABLE_STACKTRACE		if X86_64 && (UNWINDER_FRAME_POINTER || UNWINDER_ORC) && STACK_VALIDATION
 	select HAVE_STACK_VALIDATION		if X86_64
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_UNSTABLE_SCHED_CLOCK
-- 
2.15.1