Re: [PATCH v10 03/20] x86/stackvalidate: Compile-time stack validation

[Ingo Molnar <mingo@kernel.org>]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Fri, Aug 21, 2015 at 09:54:49AM +0200, Ingo Molnar wrote:
> > 
> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > > +Why do we need stack validation?
> > > +--------------------------------
> > > +
> > > +Here are some of the benefits of validating stack metadata:
> > > +
> > > +a) More reliable stack traces for frame pointer enabled kernels
> > > +
> > > +   Frame pointers are used for debugging purposes.  They allow runtime
> > > +   code and debug tools to be able to walk the stack to determine the
> > > +   chain of function call sites that led to the currently executing
> > > +   code.
> > > +
> > > +   For some architectures, frame pointers are enabled by
> > > +   CONFIG_FRAME_POINTER.  For some other architectures they may be
> > > +   required by the ABI (sometimes referred to as "backchain pointers").
> > > +
> > > +   For C code, gcc automatically generates instructions for setting up
> > > +   frame pointers when the -fno-omit-frame-pointer option is used.
> > > +
> > > +   But for asm code, the frame setup instructions have to be written by
> > > +   hand, which most people don't do.  So the end result is that
> > > +   CONFIG_FRAME_POINTER is honored for C code but not for most asm code.
> > > +
> > > +   For stack traces based on frame pointers to be reliable, all
> > > +   functions which call other functions must first create a stack frame
> > > +   and update the frame pointer.  If a first function doesn't properly
> > > +   create a stack frame before calling a second function, the *caller*
> > > +   of the first function will be skipped on the stack trace.
> > > +
> > > +   The benefit of stackvalidate here is that it ensures that *all*
> > > +   functions honor CONFIG_FRAME_POINTER.  As a result, no functions will
> > > +   ever [*] be skipped on a stack trace.
> > > +
> > > +   [*] unless an interrupt or exception has occurred at the very
> > > +       beginning of a function before the stack frame has been created,
> > > +       or at the very end of the function after the stack frame has been
> > > +       destroyed.  This is an inherent limitation of frame pointers.
> > 
> > What this section does not point out is the actual effects of missing frame 
> > pointer annotations. I.e. how about quoting a before/after stack backtrace to 
> > demonstrate it?
> 
> How about this (on top of the last one):
> 
> ---8<---
> 
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH] stackvalidate: Add missing frame pointer example
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  Documentation/stack-validation.txt | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/stack-validation.txt b/Documentation/stack-validation.txt
> index 94dad40..87a5ab8 100644
> --- a/Documentation/stack-validation.txt
> +++ b/Documentation/stack-validation.txt
> @@ -53,9 +53,40 @@ a) More reliable stack traces for frame pointer enabled kernels
>     create a stack frame before calling a second function, the *caller*
>     of the first function will be skipped on the stack trace.
>  
> -   The benefit of stackvalidate here is that it ensures that *all*
> -   functions honor CONFIG_FRAME_POINTER.  As a result, no functions will
> -   ever [*] be skipped on a stack trace.
> +   For example, consider the following example backtrace with frame
> +   pointers enabled:
> +
> +     [<ffffffff81812584>] dump_stack+0x4b/0x63
> +     [<ffffffff812d6dc2>] cmdline_proc_show+0x12/0x30
> +     [<ffffffff8127f568>] seq_read+0x108/0x3e0
> +     [<ffffffff812cce62>] proc_reg_read+0x42/0x70
> +     [<ffffffff81256197>] __vfs_read+0x37/0x100
> +     [<ffffffff81256b16>] vfs_read+0x86/0x130
> +     [<ffffffff81257898>] SyS_read+0x58/0xd0
> +     [<ffffffff8181c1f2>] entry_SYSCALL_64_fastpath+0x12/0x76
> +
> +   It correctly shows that the caller of cmdline_proc_show() is
> +   seq_read().
> +
> +   If we remove the frame pointer logic from cmdline_proc_show() by
> +   replacing the frame pointer related instructions with nops, here's
> +   what it looks like instead:
> +
> +     [<ffffffff81812584>] dump_stack+0x4b/0x63
> +     [<ffffffff812d6dc2>] cmdline_proc_show+0x12/0x30
> +     [<ffffffff812cce62>] proc_reg_read+0x42/0x70
> +     [<ffffffff81256197>] __vfs_read+0x37/0x100
> +     [<ffffffff81256b16>] vfs_read+0x86/0x130
> +     [<ffffffff81257898>] SyS_read+0x58/0xd0
> +     [<ffffffff8181c1f2>] entry_SYSCALL_64_fastpath+0x12/0x76
> +
> +   Notice that cmdline_proc_show()'s caller, seq_read(), has been
> +   skipped.  Instead the stack trace seems to show that
> +   cmdline_proc_show() was called by proc_reg_read().
> +
> +   The benefit of stackvalidate here is that because it ensures that
> +   *all* functions honor CONFIG_FRAME_POINTER, no functions will ever[*]
> +   be skipped on a stack trace.

Ok, this sounds good to me!

Thanks,

	Ingo