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

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Wed, Jan 13, 2016 at 11:55:03AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > > Well, I can't say that I'm crazy about all those new tools adding markers to 
> > > unrelated kernel code.
> > > 
> > > Can't you teach stacktool to ignore the whole machine_real_restart() function 
> > > simply?
> > 
> > Well, these STACKTOOL_IGNORE whitelist markers are only needed in a handful of 
> > places, and only for code that does very weird things.  Yes, they're a bit ugly, 
> > but IMO they also communicate valuable information: "be careful, this code does 
> > something very weird."
> 
> How common are these markers? Like with lockdep, it all depends on magnitude:
> 
>  - If it's less than 10 I'd say it's OK.
> 
>  - If it's dozens then it's ho-hum.
> 
>  - If certain types of annotations can go over 100, then they are unacceptable.
> 
> all such in-code overhead has to be balanced against the utility of the tooling.

Sounds reasonable.  After looking at all the warnings on an allyesconfig
kernel,  I'm pretty sure it'll be less than 10.  Most of the cases are
mentioned below.

> > As for whether to put the whitelist info in the code vs hard-coding it in 
> > stacktool, I think it's clearer and less "magical" to put them directly in the 
> > code.
> 
> That's true - but I think Boris tried to ask something slightly different: can 
> stacktool be taught to detect weird signatures automatically, and to ignore them 
> automatically?
> 
> Stuff like 16-bit code sure wounds 'weird' and the tool could detect that?

Some of the 'weird' cases:

- Some functions use 'ljmp' or 'lret'.  After looking at this some more,
  I think it would be safe for stacktool to translate the use of these
  instructions to mean "I know what I'm doing" and just ignore the
  function.  100% of the functions are safe to ignore anyway.  So we can
  get rid of those markers and just make stacktool smarter.

- 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)

- The error path in arch/x86/net/bpf_jit.S uses 'leaveq' to do a double
  return so that it returns from its caller's context.  stacktool
  doesn't know how to distinguish this from a frame pointer programming
  bug.  I think the only way to avoid a whitelist marker here would be
  to rewrite the bpf code to conform with more traditional rbp usage
  (but I don't know if that would really be a good idea because it would
  probably result in slower/more code).

- __bpf_prog_run() uses a jump table:

    goto *jumptable[insn->code];

  stacktool doesn't have an x86 emulator, so it doesn't know how to
  deterministically follow all possible branches for a dynamic jump.

- schedule() mucks with the frame pointer which is normally not allowed.

-- 
Josh