lkml.org 
[lkml]   [2019]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] s390/livepatch: Implement reliable stack tracing for the consistency model
On Wed, Jul 10, 2019 at 12:59:18PM +0200, Miroslav Benes wrote:
> 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 "zSeries ELF Application Binary Interface Supplement" says:
>
> "The stack pointer points to the first word of the lowest allocated
> stack frame. If the "back chain" is implemented this word will point to
> the previously allocated stack frame (towards higher addresses), except
> for the first stack frame, which shall have a back chain of zero (NULL).
> The stack shall grow downwards, in other words towards lower addresses."
>
> "back chain" is optional. GCC option -mbackchain enables it. Quoting
> Martin Schwidefsky [1]:
>
> "The compiler is called with the -mbackchain option, all normal C
> function will store the backchain in the function prologue. All
> functions written in assembler code should do the same, if you find one
> that does not we should fix that. The end result is that a task that
> *voluntarily* called schedule() should have a proper backchain at all
> times.
>
> Dependent on the use case this may or may not be enough. Asynchronous
> interrupts may stop the CPU at the beginning of a function, if kernel
> preemption is enabled we can end up with a broken backchain. The
> production kernels for IBM Z are all compiled *without* kernel
> preemption. So yes, we might get away without the objtool support.
>
> On a side-note, we do have a line item to implement the ORC unwinder for
> the kernel, that includes the objtool support. Once we have that we can
> drop the -mbackchain option for the kernel build. That gives us a nice
> little performance benefit. I hope that the change from backchain to the
> ORC unwinder will not be too hard to implement in the livepatch tools."
>
> Thus, the call chain backtracing should be currently ensured and objtool
> should not be necessary for livepatch purposes.

Hi Miroslav,

Should there be a CONFIG? dependency on -mbackchain and/or kernel
preemption, or does the following ensure that we don't need a explicit
build time checks?

> Regarding the second issue, stack corruptions and non-reliable states
> have to be recognized by the unwinder. Mainly it means to detect
> preemption or page faults, the end of the task stack must be reached,
> return addresses must be valid text addresses and hacks like function
> graph tracing and kretprobes must be properly detected.
>
> Unwinding a running task's stack is not a problem, because there is a
> livepatch requirement that every checked task is blocked, except for the
> current task. Due to that, the implementation can be much simpler
> compared to the existing non-reliable infrastructure. We can consider a
> task's kernel/thread stack only and skip the other stacks.
>
> Idle tasks are a bit special. Their final back chains point to no_dat
> stacks. See for reference CALL_ON_STACK() in smp_start_secondary()
> callback used in __cpu_up(). The unwinding is stopped there and it is
> not considered to be a stack corruption.
>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> ---
> - based on Linus' master
> - passes livepatch kselftests
> - passes tests from https://github.com/lpechacek/qa_test_klp, which
> stress the consistency model and the unwinder a bit more
>
> arch/s390/Kconfig | 1 +
> arch/s390/include/asm/stacktrace.h | 5 ++
> arch/s390/include/asm/unwind.h | 19 ++++++
> arch/s390/kernel/dumpstack.c | 28 +++++++++
> arch/s390/kernel/stacktrace.c | 78 +++++++++++++++++++++++++
> arch/s390/kernel/unwind_bc.c | 93 ++++++++++++++++++++++++++++++
> 6 files changed, 224 insertions(+)
>
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index fdb4246265a5..ea73e555063d 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -170,6 +170,7 @@ config S390
> select HAVE_PERF_EVENTS
> select HAVE_RCU_TABLE_FREE
> select HAVE_REGS_AND_STACK_ACCESS_API
> + select HAVE_RELIABLE_STACKTRACE
> select HAVE_RSEQ
> select HAVE_SYSCALL_TRACEPOINTS
> select HAVE_VIRT_CPU_ACCOUNTING
> diff --git a/arch/s390/include/asm/stacktrace.h b/arch/s390/include/asm/stacktrace.h
> index 0ae4bbf7779c..2b5c913c408f 100644
> --- a/arch/s390/include/asm/stacktrace.h
> +++ b/arch/s390/include/asm/stacktrace.h
> @@ -23,6 +23,11 @@ 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);
>
> +#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
> +int get_stack_info_reliable(unsigned long sp, struct task_struct *task,
> + struct stack_info *info);
> +#endif
> +
> 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 d827b5b9a32c..1cc96c54169c 100644
> --- a/arch/s390/include/asm/unwind.h
> +++ b/arch/s390/include/asm/unwind.h
> @@ -45,6 +45,25 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task,
> bool unwind_next_frame(struct unwind_state *state);
> unsigned long unwind_get_return_address(struct unwind_state *state);
>
> +#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
> +void __unwind_start_reliable(struct unwind_state *state,
> + struct task_struct *task, unsigned long sp);
> +bool unwind_next_frame_reliable(struct unwind_state *state);
> +
> +static inline void unwind_start_reliable(struct unwind_state *state,
> + struct task_struct *task)
> +{
> + unsigned long sp;
> +
> + if (task == current)
> + sp = current_stack_pointer();
> + else
> + sp = task->thread.ksp;
> +
> + __unwind_start_reliable(state, task, sp);
> +}
> +#endif
> +
> static inline bool unwind_done(struct unwind_state *state)
> {
> return state->stack_info.type == STACK_TYPE_UNKNOWN;
> diff --git a/arch/s390/kernel/dumpstack.c b/arch/s390/kernel/dumpstack.c
> index ac06c3949ab3..b21ef2a766ff 100644
> --- a/arch/s390/kernel/dumpstack.c
> +++ b/arch/s390/kernel/dumpstack.c
> @@ -127,6 +127,34 @@ int get_stack_info(unsigned long sp, struct task_struct *task,
> return -EINVAL;
> }
>
> +#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
> +int get_stack_info_reliable(unsigned long sp, struct task_struct *task,
> + struct stack_info *info)
> +{
> + if (!sp)
> + goto error;
> +
> + /* Sanity check: ABI requires SP to be aligned 8 bytes. */
> + if (sp & 0x7)
> + goto error;
> +

Does SP alignment only need to be checked for the initial frame, or
should it be verified everytime it's moved in
unwind_next_frame_reliable()?

> + if (!task)
> + goto error;
> +
> + /*
> + * The unwinding should not start on nodat_stack, async_stack or
> + * restart_stack. The task is either current or must be inactive.
> + */
> + if (!in_task_stack(sp, task, info))
> + goto error;
> +
> + return 0;
> +error:
> + info->type = STACK_TYPE_UNKNOWN;
> + return -EINVAL;
> +}
> +#endif
> +
> void show_stack(struct task_struct *task, unsigned long *stack)
> {
> struct unwind_state state;
> diff --git a/arch/s390/kernel/stacktrace.c b/arch/s390/kernel/stacktrace.c
> index f6a620f854e1..7d774a325163 100644
> --- a/arch/s390/kernel/stacktrace.c
> +++ b/arch/s390/kernel/stacktrace.c
> @@ -13,6 +13,7 @@
> #include <linux/export.h>
> #include <asm/stacktrace.h>
> #include <asm/unwind.h>
> +#include <asm/kprobes.h>
>
> void save_stack_trace(struct stack_trace *trace)
> {
> @@ -60,3 +61,80 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
> }
> }
> EXPORT_SYMBOL_GPL(save_stack_trace_regs);
> +
> +#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
> +/*
> + * This function returns an error if it detects any unreliable features of the
> + * stack. Otherwise it guarantees that the stack trace is reliable.
> + *
> + * If the task is not 'current', the caller *must* ensure the task is inactive.
> + */
> +static __always_inline int
> +__save_stack_trace_tsk_reliable(struct task_struct *tsk,
> + struct stack_trace *trace)
> +{
> + struct unwind_state state;
> +
> + for (unwind_start_reliable(&state, tsk);
> + !unwind_done(&state) && !unwind_error(&state);
> + unwind_next_frame_reliable(&state)) {
> +
> + if (!__kernel_text_address(state.ip))
> + return -EINVAL;
> +
> +#ifdef CONFIG_KPROBES
> + /*
> + * Mark stacktraces with kretprobed functions on them
> + * as unreliable.
> + */
> + if (state.ip == (unsigned long)kretprobe_trampoline)
> + return -EINVAL;
> +#endif
> +
> + if (trace->nr_entries >= trace->max_entries)
> + return -E2BIG;
> +
> + if (!trace->skip)
> + trace->entries[trace->nr_entries++] = state.ip;
> + else
> + trace->skip--;
> + }
> +
> + /* Check for stack corruption */
> + if (unwind_error(&state))
> + return -EINVAL;
> +
> + /* Store kernel_thread_starter, null for swapper/0 */
> + if (tsk->flags & (PF_KTHREAD | PF_IDLE)) {
> + if (trace->nr_entries >= trace->max_entries)
> + return -E2BIG;
> +
> + if (!trace->skip)
> + trace->entries[trace->nr_entries++] =
> + state.regs->psw.addr;
> + else
> + trace->skip--;

An idea for a follow up patch: stuff this into a function like
int save_trace_entry(struct stack_trace *trace, unsigned long entry);
which could one day make the trace->entries[] code generic across arches.

> + }
> +
> + return 0;
> +}
> +
> +int save_stack_trace_tsk_reliable(struct task_struct *tsk,
> + struct stack_trace *trace)
> +{
> + int ret;
> +
> + /*
> + * If the task doesn't have a stack (e.g., a zombie), the stack is
> + * "reliably" empty.
> + */
> + if (!try_get_task_stack(tsk))
> + return 0;
> +
> + ret = __save_stack_trace_tsk_reliable(tsk, trace);
> +
> + put_task_stack(tsk);
> +
> + return ret;
> +}
> +#endif
> diff --git a/arch/s390/kernel/unwind_bc.c b/arch/s390/kernel/unwind_bc.c
> index 3ce8a0808059..ada3a8538961 100644
> --- a/arch/s390/kernel/unwind_bc.c
> +++ b/arch/s390/kernel/unwind_bc.c
> @@ -153,3 +153,96 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task,
> state->reliable = reliable;
> }
> EXPORT_SYMBOL_GPL(__unwind_start);
> +
> +#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
> +void __unwind_start_reliable(struct unwind_state *state,
> + struct task_struct *task, unsigned long sp)
> +{
> + struct stack_info *info = &state->stack_info;
> + struct stack_frame *sf;
> + unsigned long ip;
> +
> + memset(state, 0, sizeof(*state));
> + state->task = task;
> +
> + /* Get current stack pointer and initialize stack info */
> + if (get_stack_info_reliable(sp, task, info) ||
> + !on_stack(info, sp, sizeof(struct stack_frame))) {
> + /* Something is wrong with the stack pointer */
> + info->type = STACK_TYPE_UNKNOWN;
> + state->error = true;
> + return;
> + }
> +
> + /* Get the instruction pointer from the stack frame */
> + sf = (struct stack_frame *) sp;
> + ip = READ_ONCE_NOCHECK(sf->gprs[8]);
> +
> +#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);
^^^^
double checking: we ignore the retp here and not in the next-frame case?

> +#endif
> +
> + /* Update unwind state */
> + state->sp = sp;
> + state->ip = ip;
> +}
> +
> +bool unwind_next_frame_reliable(struct unwind_state *state)
> +{
> + struct stack_info *info = &state->stack_info;
> + struct stack_frame *sf;
> + struct pt_regs *regs;
> + unsigned long sp, ip;
> +
> + sf = (struct stack_frame *) state->sp;
> + sp = READ_ONCE_NOCHECK(sf->back_chain);
> + /*
> + * Idle tasks are special. The final back-chain points to nodat_stack.
> + * See CALL_ON_STACK() in smp_start_secondary() callback used in
> + * __cpu_up(). We just accept it, go to else branch and look for
> + * pt_regs.
> + */
> + if (likely(sp && !(is_idle_task(state->task) &&
> + outside_of_stack(state, sp)))) {
> + /* Non-zero back-chain points to the previous frame */
> + if (unlikely(outside_of_stack(state, sp)))
> + goto out_err;
> +
> + sf = (struct stack_frame *) sp;
> + ip = READ_ONCE_NOCHECK(sf->gprs[8]);
> + } else {
> + /* No back-chain, look for a pt_regs structure */
> + sp = state->sp + STACK_FRAME_OVERHEAD;
> + regs = (struct pt_regs *) sp;
> + if ((unsigned long)regs != info->end - sizeof(struct pt_regs))
> + goto out_err;
> + if (!(state->task->flags & (PF_KTHREAD | PF_IDLE)) &&
> + !user_mode(regs))
> + goto out_err;
> +
> + state->regs = regs;
> + goto out_stop;
> + }
> +
> +#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
> +
> + /* Update unwind state */
> + state->sp = sp;
> + state->ip = ip;

minor nit: maybe the CONFIG_FUNCTION_GRAPH_TRACER and "Update unwind
state" logic could be combined into a function? (Not a big deal either
way.)

> + return true;
> +
> +out_err:
> + state->error = true;
> +out_stop:
> + state->stack_info.type = STACK_TYPE_UNKNOWN;
> + return false;
> +}
> +#endif
> --
> 2.22.0
>

I've tested the patch with positive results, however I didn't stress it
very hard (basically only selftests). The code logic seems
straightforward and correct by inspection.

On a related note, do you think it would be feasible to extend (in
another patchset) the reliable stack unwinding code a bit so that we
could feed it pre-baked stacks ... then we could verify that the code
was finding interesting scenarios. That was a passing thought I had
back when Nicolai and I were debugging the ppc64le exception frame
marker bug, but didn't think it worth the time/effort at the time.

One more note: Using READ_ONCE_NOCHECK is probably correct here, but
s390 happens to define a READ_ONCE_TASK_STACK macro which calls
READ_ONCE_NOCHECK when task != current. According to the code comments,
this "disables KASAN checking when reading a value from another task's
stack". Is there any scenario here where we would want to use the that
wrapper macro?

-- Joe

\
 
 \ /
  Last update: 2019-07-16 20:46    [W:0.104 / U:28.360 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site