Re: [PATCH v6 2/4] x86/stackvalidate: Compile-time stack validation

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Jul 07, 2015 at 04:35:17PM -0700, Andy Lutomirski wrote:
> On Tue, Jul 7, 2015 at 4:29 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Tue, Jul 07, 2015 at 03:57:14PM -0700, Andy Lutomirski wrote:
> >> On Tue, Jul 7, 2015 at 7:54 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> >
> >> > It currently only supports x86_64.  I tried to make the code generic so
> >> > that support for other architectures can hopefully be plugged in
> >> > relatively easily.
> >> >
> >> > Currently with my Fedora config it's reporting over 1400 warnings, but
> >> > most of them are duplicates.  The warnings affect 37 .c files and 18 .S
> >> > files.  The C file warnings are generally due to inline assembly, which
> >> > doesn't seem to play nice with frame pointers.
> >>
> >> This issue might be worth bringing up on the gcc and binutils lists.
> >> If we need better toolchain support, let's ask for it.
> >
> > I think we found a good solution for this.  See my update at:
> >
> >   https://lkml.kernel.org/r/20150707223519.GA31294@treble.redhat.com
> 
> Does that force frame pointer generation?  If so, then once we have a
> real kernel unwinder, we might want a non-frame-pointer-forcing
> version for better code generation.  (That can wait, of course.)

I got some clarification on this solution of adding the stack pointer to
the output operand of the asm() statement.

If frame pointers are enabled, it forces frame pointer generation and
ensures the stack frame is set up before the asm() code.  If frame
pointers are disabled, it appears to have no effect on the generated
code.  So it's looking good for now and for the future.

> >> > +6. stackvalidate: asm_file.o: func()+0x26: jump to outside file from callable function
> >> > +   or
> >> > +   stackvalidate: asm_file.o: func()+0xd9: jump to dynamic address from callable function
> >> > +
> >> > +   These are constraints imposed by stackvalidate so that it can
> >> > +   properly analyze all jump targets.  Dynamic jump targets and jumps to
> >> > +   code in other object files aren't allowed.
> >>
> >> Does this not trigger due to optimized sibling calls to different files?
> >
> > This is a great point.  With CONFIG_FRAME_POINTER it's not a problem,
> > because it adds -fno-optimize-sibling-calls.
> >
> > Without it, I think stackvalidate would spit out a ton of "jump to
> > outside file" warnings.
> >
> > I haven't yet looked at the details of how exactly sibling calls work.
> > I'd assume they're disabled because they break frame pointers somehow.
> > Any idea if they'd also break DWARF CFI stack traces?
> 
> They'll certainly prevent unwinding from finding the pre-optimization
> caller, but the rest of unwinding should work.  I don't know why we
> turn it off, though.
> 
> You might want special-case jump-out-of-translation-unit to be okay if
> the stack frame is in its initial state.  That is:
> 
> func:
>      jmp elsewhere
> 
> could be considered okay, as could:
> 
> func:
>      push %rax
>      pop %rax
>      jmp elsewhere
> 
> and similar.

This approach works great for all sibling calls.  Wrapping up v7 now.

-- 
Josh