On Thu 2016-12-08 12:08:26, Josh Poimboeuf wrote:
> For live patching and possibly other use cases, a stack trace is only
> useful if it can be assured that it's completely reliable. Add a new
> save_stack_trace_tsk_reliable() function to achieve that.
>
> Scenarios which indicate that a stack trace may be unreliable:
>
> - running task
It seems that this has to be enforced by save_stack_trace_tsk_reliable()
caller. It should be mentioned in the function description.
> - interrupt stack
I guess that it is detected by saved regs on the stack. And it covers
also dynamic changes like kprobes. Do I get it correctly, please?
What about ftrace? Is ftrace without regs safe and detected?
> - preemption
I wonder if some very active kthreads might almost always be
preempted using irq in preemptive kernel. Then they block
the conversion with the non-reliable stacks. Have you noticed
such problems, please?
> - corrupted stack data
> - stack grows the wrong way
This is detected in unwind_next_frame() and passed via state->error.
Am I right?
> - stack walk doesn't reach the bottom
> - user didn't provide a large enough entries array
>
> Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can
> determine at build time whether the function is implemented.
>
> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
> index 0653788..3e0cf5e 100644
> --- a/arch/x86/kernel/stacktrace.c
> +++ b/arch/x86/kernel/stacktrace.c
> @@ -74,6 +74,64 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
> }
> EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
>
> +#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
> +static int __save_stack_trace_reliable(struct stack_trace *trace,
> + struct task_struct *task)
> +{
> + struct unwind_state state;
> + struct pt_regs *regs;
> + unsigned long addr;
> +
> + for (unwind_start(&state, task, NULL, NULL); !unwind_done(&state);
> + unwind_next_frame(&state)) {
> +
> + regs = unwind_get_entry_regs(&state);
> + if (regs) {
> + /*
> + * Preemption and page faults on the stack can make
> + * frame pointers unreliable.
> + */
> + if (!user_mode(regs))
> + return -1;
By other words, it we find regs on the stack, it almost always mean
a non-reliable stack. The only exception is when we are in the
userspace mode. Do I get it correctly, please?
> +
> + /*
> + * This frame contains the (user mode) pt_regs at the
> + * end of the stack. Finish the unwind.
> + */
> + unwind_next_frame(&state);
> + break;
> + }
> +
> + addr = unwind_get_return_address(&state);
> + if (!addr || save_stack_address(trace, addr, false))
> + return -1;
> + }
> +
> + if (!unwind_done(&state) || unwind_error(&state))
> + return -1;
> +
> + if (trace->nr_entries < trace->max_entries)
> + trace->entries[trace->nr_entries++] = ULONG_MAX;
> +
> + return 0;
> +}
Great work! I am surprised that it looks so straightforward.
I still have to think and investigate it more. But it looks
very promissing.
Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
