The livepatch consistency model requires reliable stack tracing
architecture support in order to work properly. In order to achieve
this, two main issues have to be solved. First, reliable and consistent
call chain backtracing has to be ensured. Second, the unwinder needs to
be able to detect stack corruptions and return errors.
The former should be guaranteed (see 4/4 for details), the latter is
implemented by the patch set (mainly 4/4).
It passes livepatch kselftests (with timeout disabled, see
https://lore.kernel.org/live-patching/[email protected]/)
and tests from https://github.com/lpechacek/qa_test_klp.
Based on v5.4-rc6. That is why I left 1/4 patch in the series despite
its presence in s390/features branch. The context of the rest of the
series could be preserved as such and changes in s390/fixes present in
v5.4-rc6 are taken into account.
Changes since v2:
- unwind_next_frame() is split to several functions to make it easier to
review and prepare it for additional changes
- unwind_next_frame_reliable() merged into the existing
unwind_next_frame()
Changes since v1 and questions follow:
- rebased on top of v5.4-rc5. It also meant rebasing on top of
ARCH_STACKWALK, which made the outcome nicer and addressed some of
Joe's remarks.
- sp alignment is checked in both _start and _next_frame cases
- ftrace_graph_ret_addr() calling cleanup
- I tried to use the existing infrastructure as much as possible with
one exception. I kept unwind_next_frame_reliable() next to the
ordinary unwind_next_frame(). I did not come up with a nice solution
how to integrate it. The reliable unwinding is executed on a task
stack only, which leads to a nice simplification. My integration
attempts only obfuscated the existing unwind_next_frame() which is
already not easy to read. Ideas are definitely welcome.
- although not nice (Josh mentioned it in his review), I kept checking
for kretprobe_trampoline in the main loop. It could go into _start and
_next_frame callbacks, but more changes would be required (changing a
function return type, ordinary unwinding does not need it). So I did
not think it was worth it. I could change it in v3, of course.
Miroslav Benes (4):
s390/unwind: drop unnecessary code around calling
ftrace_graph_ret_addr()
s390/unwind: split unwind_next_frame() to several functions
s390/unwind: prepare the unwinding interface for reliable stack traces
s390/livepatch: Implement reliable stack tracing for the consistency
model
arch/s390/Kconfig | 1 +
arch/s390/include/asm/stacktrace.h | 3 +-
arch/s390/include/asm/unwind.h | 18 +--
arch/s390/kernel/dumpstack.c | 16 ++-
arch/s390/kernel/perf_event.c | 2 +-
arch/s390/kernel/stacktrace.c | 48 ++++++-
arch/s390/kernel/unwind_bc.c | 194 ++++++++++++++++++++---------
arch/s390/oprofile/init.c | 2 +-
8 files changed, 208 insertions(+), 76 deletions(-)
--
2.23.0
The current code around calling ftrace_graph_ret_addr() is ifdeffed and
also tests if ftrace redirection is present on stack.
ftrace_graph_ret_addr() however performs the test internally and there
is a version for !CONFIG_FUNCTION_GRAPH_TRACER as well. The unnecessary
code can thus be dropped.
Signed-off-by: Miroslav Benes <[email protected]>
---
arch/s390/kernel/unwind_bc.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/arch/s390/kernel/unwind_bc.c b/arch/s390/kernel/unwind_bc.c
index a8204f952315..5a78deacb972 100644
--- a/arch/s390/kernel/unwind_bc.c
+++ b/arch/s390/kernel/unwind_bc.c
@@ -85,12 +85,8 @@ bool unwind_next_frame(struct unwind_state *state)
}
}
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- /* Decode any ftrace redirection */
- if (ip == (unsigned long) return_to_handler)
- ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
- ip, (void *) sp);
-#endif
+ ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
+ ip, (void *) sp);
/* Update unwind state */
state->sp = sp;
@@ -147,12 +143,8 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task,
reuse_sp = false;
}
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- /* Decode any ftrace redirection */
- if (ip == (unsigned long) return_to_handler)
- ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
- ip, NULL);
-#endif
+ ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
+ ip, NULL);
/* Update unwind state */
state->sp = sp;
--
2.23.0
Function unwind_next_frame() becomes less readable with each new
change. Split it to several functions to amend it and prepare for new
additions.
No functional change.
Suggested-by: Petr Mladek <[email protected]>
Signed-off-by: Miroslav Benes <[email protected]>
---
arch/s390/kernel/unwind_bc.c | 136 ++++++++++++++++++++++-------------
1 file changed, 88 insertions(+), 48 deletions(-)
diff --git a/arch/s390/kernel/unwind_bc.c b/arch/s390/kernel/unwind_bc.c
index 5a78deacb972..96da99ec7b59 100644
--- a/arch/s390/kernel/unwind_bc.c
+++ b/arch/s390/kernel/unwind_bc.c
@@ -36,55 +36,10 @@ static bool update_stack_info(struct unwind_state *state, unsigned long sp)
return true;
}
-bool unwind_next_frame(struct unwind_state *state)
+static bool unwind_update_state(struct unwind_state *state,
+ unsigned long sp, unsigned long ip,
+ struct pt_regs *regs, bool reliable)
{
- struct stack_info *info = &state->stack_info;
- struct stack_frame *sf;
- struct pt_regs *regs;
- unsigned long sp, ip;
- bool reliable;
-
- regs = state->regs;
- if (unlikely(regs)) {
- if (state->reuse_sp) {
- sp = state->sp;
- state->reuse_sp = false;
- } else {
- sp = READ_ONCE_NOCHECK(regs->gprs[15]);
- if (unlikely(outside_of_stack(state, sp))) {
- if (!update_stack_info(state, sp))
- goto out_err;
- }
- }
- sf = (struct stack_frame *) sp;
- ip = READ_ONCE_NOCHECK(sf->gprs[8]);
- reliable = false;
- regs = NULL;
- } else {
- sf = (struct stack_frame *) state->sp;
- sp = READ_ONCE_NOCHECK(sf->back_chain);
- if (likely(sp)) {
- /* Non-zero back-chain points to the previous frame */
- if (unlikely(outside_of_stack(state, sp))) {
- if (!update_stack_info(state, sp))
- goto out_err;
- }
- sf = (struct stack_frame *) sp;
- ip = READ_ONCE_NOCHECK(sf->gprs[8]);
- reliable = true;
- } else {
- /* No back-chain, look for a pt_regs structure */
- sp = state->sp + STACK_FRAME_OVERHEAD;
- if (!on_stack(info, sp, sizeof(struct pt_regs)))
- goto out_stop;
- regs = (struct pt_regs *) sp;
- if (READ_ONCE_NOCHECK(regs->psw.mask) & PSW_MASK_PSTATE)
- goto out_stop;
- ip = READ_ONCE_NOCHECK(regs->psw.addr);
- reliable = true;
- }
- }
-
ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
ip, (void *) sp);
@@ -94,13 +49,98 @@ bool unwind_next_frame(struct unwind_state *state)
state->regs = regs;
state->reliable = reliable;
return true;
+}
+
+static bool unwind_use_regs(struct unwind_state *state)
+{
+ struct stack_frame *sf;
+ unsigned long sp, ip;
+ struct pt_regs *regs = state->regs;
+
+ if (state->reuse_sp) {
+ sp = state->sp;
+ state->reuse_sp = false;
+ } else {
+ sp = READ_ONCE_NOCHECK(regs->gprs[15]);
+ if (unlikely(outside_of_stack(state, sp))) {
+ if (!update_stack_info(state, sp))
+ goto out_err;
+ }
+ }
+
+ sf = (struct stack_frame *) sp;
+ ip = READ_ONCE_NOCHECK(sf->gprs[8]);
+
+ return unwind_update_state(state, sp, ip, NULL, false);
+
+out_err:
+ state->error = true;
+ state->stack_info.type = STACK_TYPE_UNKNOWN;
+ return false;
+}
+
+static bool unwind_use_frame(struct unwind_state *state, unsigned long sp)
+{
+ struct stack_frame *sf;
+ unsigned long ip;
+
+ if (unlikely(outside_of_stack(state, sp))) {
+ if (!update_stack_info(state, sp))
+ goto out_err;
+ }
+
+ sf = (struct stack_frame *) sp;
+ ip = READ_ONCE_NOCHECK(sf->gprs[8]);
+
+ return unwind_update_state(state, sp, ip, NULL, true);
out_err:
state->error = true;
+ state->stack_info.type = STACK_TYPE_UNKNOWN;
+ return false;
+}
+
+static bool unwind_look_for_regs(struct unwind_state *state)
+{
+ struct stack_info *info = &state->stack_info;
+ struct pt_regs *regs;
+ unsigned long sp, ip;
+
+ sp = state->sp + STACK_FRAME_OVERHEAD;
+ if (!on_stack(info, sp, sizeof(struct pt_regs)))
+ goto out_stop;
+
+ regs = (struct pt_regs *) sp;
+ if (READ_ONCE_NOCHECK(regs->psw.mask) & PSW_MASK_PSTATE)
+ goto out_stop;
+
+ ip = READ_ONCE_NOCHECK(regs->psw.addr);
+
+ return unwind_update_state(state, sp, ip, regs, true);
+
out_stop:
state->stack_info.type = STACK_TYPE_UNKNOWN;
return false;
}
+
+bool unwind_next_frame(struct unwind_state *state)
+{
+ struct stack_frame *sf;
+ unsigned long sp;
+
+ if (unlikely(state->regs))
+ return unwind_use_regs(state);
+
+ sf = (struct stack_frame *) state->sp;
+ sp = READ_ONCE_NOCHECK(sf->back_chain);
+
+ /* Non-zero back-chain points to the previous frame */
+ if (likely(sp))
+ return unwind_use_frame(state, sp);
+
+ /* No back-chain, look for a pt_regs structure */
+ return unwind_look_for_regs(state);
+}
EXPORT_SYMBOL_GPL(unwind_next_frame);
void __unwind_start(struct unwind_state *state, struct task_struct *task,
--
2.23.0
The reliable stack traces support will require to perform more actions
and some tasks differently. Add a new parameter to respective functions
and propagate it where appropriate.
Signed-off-by: Miroslav Benes <[email protected]>
---
arch/s390/include/asm/stacktrace.h | 3 ++-
arch/s390/include/asm/unwind.h | 18 ++++++++++--------
arch/s390/kernel/dumpstack.c | 5 +++--
arch/s390/kernel/perf_event.c | 2 +-
arch/s390/kernel/stacktrace.c | 2 +-
arch/s390/kernel/unwind_bc.c | 19 +++++++++++--------
arch/s390/oprofile/init.c | 2 +-
7 files changed, 29 insertions(+), 22 deletions(-)
diff --git a/arch/s390/include/asm/stacktrace.h b/arch/s390/include/asm/stacktrace.h
index 0ae4bbf7779c..f033bb70a8db 100644
--- a/arch/s390/include/asm/stacktrace.h
+++ b/arch/s390/include/asm/stacktrace.h
@@ -21,7 +21,8 @@ struct stack_info {
const char *stack_type_name(enum stack_type type);
int get_stack_info(unsigned long sp, struct task_struct *task,
- struct stack_info *info, unsigned long *visit_mask);
+ struct stack_info *info, unsigned long *visit_mask,
+ bool unwind_reliable);
static inline bool on_stack(struct stack_info *info,
unsigned long addr, size_t len)
diff --git a/arch/s390/include/asm/unwind.h b/arch/s390/include/asm/unwind.h
index eaaefeceef6f..4c36e200404e 100644
--- a/arch/s390/include/asm/unwind.h
+++ b/arch/s390/include/asm/unwind.h
@@ -42,8 +42,9 @@ struct unwind_state {
};
void __unwind_start(struct unwind_state *state, struct task_struct *task,
- struct pt_regs *regs, unsigned long first_frame);
-bool unwind_next_frame(struct unwind_state *state);
+ struct pt_regs *regs, unsigned long first_frame,
+ bool unwind_reliable);
+bool unwind_next_frame(struct unwind_state *state, bool unwind_reliable);
unsigned long unwind_get_return_address(struct unwind_state *state);
static inline bool unwind_done(struct unwind_state *state)
@@ -59,10 +60,11 @@ static inline bool unwind_error(struct unwind_state *state)
static inline void unwind_start(struct unwind_state *state,
struct task_struct *task,
struct pt_regs *regs,
- unsigned long sp)
+ unsigned long sp,
+ bool unwind_reliable)
{
sp = sp ? : get_stack_pointer(task, regs);
- __unwind_start(state, task, regs, sp);
+ __unwind_start(state, task, regs, sp, unwind_reliable);
}
static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)
@@ -70,10 +72,10 @@ static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)
return unwind_done(state) ? NULL : state->regs;
}
-#define unwind_for_each_frame(state, task, regs, first_frame) \
- for (unwind_start(state, task, regs, first_frame); \
- !unwind_done(state); \
- unwind_next_frame(state))
+#define unwind_for_each_frame(state, task, regs, first_frame, unwind_reliable) \
+ for (unwind_start(state, task, regs, first_frame, unwind_reliable); \
+ !unwind_done(state); \
+ unwind_next_frame(state, unwind_reliable))
static inline void unwind_init(void) {}
static inline void unwind_module_init(struct module *mod, void *orc_ip,
diff --git a/arch/s390/kernel/dumpstack.c b/arch/s390/kernel/dumpstack.c
index 34bdc60c0b11..1ee19e6336cd 100644
--- a/arch/s390/kernel/dumpstack.c
+++ b/arch/s390/kernel/dumpstack.c
@@ -88,7 +88,8 @@ static bool in_restart_stack(unsigned long sp, struct stack_info *info)
}
int get_stack_info(unsigned long sp, struct task_struct *task,
- struct stack_info *info, unsigned long *visit_mask)
+ struct stack_info *info, unsigned long *visit_mask,
+ bool unwind_reliable)
{
if (!sp)
goto unknown;
@@ -130,7 +131,7 @@ void show_stack(struct task_struct *task, unsigned long *stack)
printk("Call Trace:\n");
if (!task)
task = current;
- unwind_for_each_frame(&state, task, NULL, (unsigned long) stack)
+ unwind_for_each_frame(&state, task, NULL, (unsigned long) stack, false)
printk(state.reliable ? " [<%016lx>] %pSR \n" :
"([<%016lx>] %pSR)\n",
state.ip, (void *) state.ip);
diff --git a/arch/s390/kernel/perf_event.c b/arch/s390/kernel/perf_event.c
index fcb6c2e92b07..7e81db0883e1 100644
--- a/arch/s390/kernel/perf_event.c
+++ b/arch/s390/kernel/perf_event.c
@@ -225,7 +225,7 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
{
struct unwind_state state;
- unwind_for_each_frame(&state, current, regs, 0)
+ unwind_for_each_frame(&state, current, regs, 0, false)
perf_callchain_store(entry, state.ip);
}
diff --git a/arch/s390/kernel/stacktrace.c b/arch/s390/kernel/stacktrace.c
index f8fc4f8aef9b..751c136172f7 100644
--- a/arch/s390/kernel/stacktrace.c
+++ b/arch/s390/kernel/stacktrace.c
@@ -16,7 +16,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
struct unwind_state state;
unsigned long addr;
- unwind_for_each_frame(&state, task, regs, 0) {
+ unwind_for_each_frame(&state, task, regs, 0, false) {
addr = unwind_get_return_address(&state);
if (!addr || !consume_entry(cookie, addr, false))
break;
diff --git a/arch/s390/kernel/unwind_bc.c b/arch/s390/kernel/unwind_bc.c
index 96da99ec7b59..092626f4e7c6 100644
--- a/arch/s390/kernel/unwind_bc.c
+++ b/arch/s390/kernel/unwind_bc.c
@@ -29,7 +29,7 @@ static bool update_stack_info(struct unwind_state *state, unsigned long sp)
unsigned long *mask = &state->stack_mask;
/* New stack pointer leaves the current stack */
- if (get_stack_info(sp, state->task, info, mask) != 0 ||
+ if (get_stack_info(sp, state->task, info, mask, false) != 0 ||
!on_stack(info, sp, sizeof(struct stack_frame)))
/* 'sp' does not point to a valid stack */
return false;
@@ -79,7 +79,8 @@ static bool unwind_use_regs(struct unwind_state *state)
return false;
}
-static bool unwind_use_frame(struct unwind_state *state, unsigned long sp)
+static bool unwind_use_frame(struct unwind_state *state, unsigned long sp,
+ bool unwind_reliable)
{
struct stack_frame *sf;
unsigned long ip;
@@ -100,7 +101,8 @@ static bool unwind_use_frame(struct unwind_state *state, unsigned long sp)
return false;
}
-static bool unwind_look_for_regs(struct unwind_state *state)
+static bool unwind_look_for_regs(struct unwind_state *state,
+ bool unwind_reliable)
{
struct stack_info *info = &state->stack_info;
struct pt_regs *regs;
@@ -123,7 +125,7 @@ static bool unwind_look_for_regs(struct unwind_state *state)
return false;
}
-bool unwind_next_frame(struct unwind_state *state)
+bool unwind_next_frame(struct unwind_state *state, bool unwind_reliable)
{
struct stack_frame *sf;
unsigned long sp;
@@ -136,15 +138,16 @@ bool unwind_next_frame(struct unwind_state *state)
/* Non-zero back-chain points to the previous frame */
if (likely(sp))
- return unwind_use_frame(state, sp);
+ return unwind_use_frame(state, sp, unwind_reliable);
/* No back-chain, look for a pt_regs structure */
- return unwind_look_for_regs(state);
+ return unwind_look_for_regs(state, unwind_reliable);
}
EXPORT_SYMBOL_GPL(unwind_next_frame);
void __unwind_start(struct unwind_state *state, struct task_struct *task,
- struct pt_regs *regs, unsigned long sp)
+ struct pt_regs *regs, unsigned long sp,
+ bool unwind_reliable)
{
struct stack_info *info = &state->stack_info;
unsigned long *mask = &state->stack_mask;
@@ -163,7 +166,7 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task,
}
/* Get current stack pointer and initialize stack info */
- if (get_stack_info(sp, task, info, mask) != 0 ||
+ if (get_stack_info(sp, task, info, mask, unwind_reliable) != 0 ||
!on_stack(info, sp, sizeof(struct stack_frame))) {
/* Something is wrong with the stack pointer */
info->type = STACK_TYPE_UNKNOWN;
diff --git a/arch/s390/oprofile/init.c b/arch/s390/oprofile/init.c
index 7441857df51b..59d736f46cbc 100644
--- a/arch/s390/oprofile/init.c
+++ b/arch/s390/oprofile/init.c
@@ -19,7 +19,7 @@ static void s390_backtrace(struct pt_regs *regs, unsigned int depth)
{
struct unwind_state state;
- unwind_for_each_frame(&state, current, regs, 0) {
+ unwind_for_each_frame(&state, current, regs, 0, false) {
if (depth-- == 0)
break;
oprofile_add_trace(state.ip);
--
2.23.0
On Wed, Nov 06, 2019 at 10:55:58AM +0100, Miroslav Benes wrote:
> The current code around calling ftrace_graph_ret_addr() is ifdeffed and
> also tests if ftrace redirection is present on stack.
> ftrace_graph_ret_addr() however performs the test internally and there
> is a version for !CONFIG_FUNCTION_GRAPH_TRACER as well. The unnecessary
> code can thus be dropped.
>
> Signed-off-by: Miroslav Benes <[email protected]>
> ---
> arch/s390/kernel/unwind_bc.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
This patch has been picked up from v2 already. It's in Linus tree.