Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool whitelist

[Josh Poimboeuf]

On Fri, Jan 15, 2016 at 12:00:00PM +0100, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp@xxxxxxxxx> wrote:
> 
> > On Fri, Jan 15, 2016 at 12:06:52AM -0600, Josh Poimboeuf wrote:
> > > - xen_cpuid() uses some custom xen instructions which start with
> > >   XEN_EMULATE_PREFIX.  It corresponds to the following x86 instructions:
> > > 
> > >     ffffffff8107e572:       0f 0b                   ud2
> > >     ffffffff8107e574:       78 65                   js ffffffff8107e5db <xen_get_debugreg+0xa>
> > >     ffffffff8107e576:       6e                      outsb %ds:(%rsi),(%dx)
> > > 
> > >   Apparently(?) xen treats the ud2 special when it's followed by "78 65
> > >   6e".  This is confusing for stacktool because ud2 is normally a dead
> > >   end, and it thinks the instructions after it will never run.
> > >   
> > >   (In theory stacktool could be taught to understand this hack, but
> > >   that's a bad idea IMO)
> > 
> > Why, because it is not generic enough?
> > 
> > Well, you could add a cmdline option "--kernel" which is supplied when
> > checking the kernel and such kernel "idiosyncrasies" are handled only
> > then and there. And since the tool is part of the kernel, changes to
> > XEN_EMULATE_PREFIX, will have to be updated in stacktool too...
> 
> So I think because we are talking about less than a dozen annotations, these are 
> technicalities - and it might in fact be better to have a single line of obvious 
> annotation in a function that does something weird (and arguably all of these 
> functions do something weird), than having dozens of lines of code on the tooling 
> side to avoid that single line on the kernel side.
> 
> That has a documentation value as well.
> 
> As long as the annotation itself is not stacktool specific, it should serve as 
> documentation as well - such as:
> 
>   __non_standard_stack_frame
> 
> or:
> 
>   __non_C_instructions
> 
> ?
> 
> All of the cases Josh listed involve some sort of special case where we do 
> something non-standard. (Where 'standard' == 'regular kernel C function'.)

I've now gotten the number of warnings down to 0 (except for a few
staging drivers), even with allyesconfig (with !CONFIG_GCOV).

I've also managed to make stacktool a little smarter such that the
in-code STACKTOOL_IGNORE_INSN markers are no longer needed, woot!

There's still a need for 4 STACKTOOL_IGNORE_FUNC(name) markers in the
entire tree, due to the weird cases I mentioned.  But they're placed
after the functions, so they're much less disruptive.

I'll be posting a v16 soon.

-- 
Josh