Re: [RFC PATCH v1.9 07/14] x86/stacktrace: add function for detecting reliable stack traces

[Josh Poimboeuf]

On Mon, Apr 04, 2016 at 05:55:01PM +0200, Petr Mladek wrote:
> On Fri 2016-03-25 14:34:54, Josh Poimboeuf wrote:
> > For live patching and possibly other use cases, a stack trace is only
> > useful if you 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 strace may be unreliable:
> > 
> > - interrupt stacks
> > - preemption
> > - corrupted stack data
> > - newly forked tasks
> > - running tasks
> > - the user didn't provide a large enough entries array
> > 
> > Also add a config option so arch-independent code can determine at build
> > time whether the function is implemented.
> > 
> > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> > index 3b10518..9c68bfc 100644
> > --- a/arch/x86/kernel/dumpstack.c
> > +++ b/arch/x86/kernel/dumpstack.c
> > @@ -145,6 +145,42 @@ int print_context_stack_bp(struct thread_info *tinfo,
> >  }
> >  EXPORT_SYMBOL_GPL(print_context_stack_bp);
> >  
> > +int print_context_stack_reliable(struct thread_info *tinfo,
> > +				 unsigned long *stack, unsigned long *bp,
> > +				 const struct stacktrace_ops *ops,
> > +				 void *data, unsigned long *end, int *graph)
> > +{
> > +	struct stack_frame *frame = (struct stack_frame *)*bp;
> > +	struct stack_frame *last_frame = frame;
> 
> I tried to debug why the patching never finishes as reported by Mirek.
> This initialization breaks the whole function, see below.
> I would initialize last_frame to NULL or maybe (void *)stack.
> 
> 
> > +	unsigned long *ret_addr = &frame->return_address;
> > +
> > +	if (test_ti_thread_flag(tinfo, TIF_FORK))
> > +		return -EINVAL;
> > +
> > +	while (valid_stack_ptr(tinfo, ret_addr, sizeof(*ret_addr), end)) {
> > +		unsigned long addr = *ret_addr;
> > +
> > +		if (frame <= last_frame || !__kernel_text_address(addr) ||
> 
> frame == last_frame in the very rist iteration, so we always return -EINVAL.

Ah, right, thanks for debugging it.  I actually did test an earlier
iteration of the function, but obviously didn't test this one.

-- 
Josh