Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756017AbcDDR7D (ORCPT ); Mon, 4 Apr 2016 13:59:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57470 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755912AbcDDR7B (ORCPT ); Mon, 4 Apr 2016 13:59:01 -0400 Date: Mon, 4 Apr 2016 12:58:59 -0500 From: Josh Poimboeuf To: Petr Mladek Cc: Jiri Kosina , Jessica Yu , Miroslav Benes , linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, Vojtech Pavlik Subject: Re: [RFC PATCH v1.9 07/14] x86/stacktrace: add function for detecting reliable stack traces Message-ID: <20160404175859.hkaekiddirb5dnkl@treble.redhat.com> References: <1f8c648ed8b8eb49a75f5a6cacf8b7ca76f44fa9.1458933243.git.jpoimboe@redhat.com> <20160404155501.GR5522@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160404155501.GR5522@pathway.suse.cz> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2112 Lines: 56 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