Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1307626 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-12 17:50 +0100
    Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Josh Poimboeuf <jpoimboe@redhat.com> - 2016-01-12 18:50 +0100
      Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-12 19:00 +0100
        Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Josh Poimboeuf <jpoimboe@redhat.com> - 2016-01-12 20:00 +0100
          Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-12 20:40 +0100
      Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Ingo Molnar <mingo@kernel.org> - 2016-01-13 12:00 +0100
        Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Josh Poimboeuf <jpoimboe@redhat.com> - 2016-01-15 07:10 +0100
          Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-15 11:50 +0100
            Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Ingo Molnar <mingo@kernel.org> - 2016-01-15 12:10 +0100
              Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-15 12:20 +0100
                Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Ingo Molnar <mingo@kernel.org> - 2016-01-15 12:20 +0100
              Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Josh Poimboeuf <jpoimboe@redhat.com> - 2016-01-20 06:50 +0100
                Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 07:00 +0100
                  Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Josh Poimboeuf <jpoimboe@redhat.com> - 2016-01-20 07:20 +0100
                Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-20 11:50 +0100
          Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Ingo Molnar <mingo@kernel.org> - 2016-01-15 12:00 +0100

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

On Fri, Dec 18, 2015 at 06:39:27AM -0600, Josh Poimboeuf wrote:
> stacktool reports a false positive warning for the ljmp instruction in
> machine_real_restart().  Normally, ljmp isn't allowed in a function, but
> this is a special case where it's jumping into real mode.
> 
> Add the jumps to a whitelist which tells stacktool to ignore them.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/kernel/reboot.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 02693dd..1ea1c5e 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -9,6 +9,7 @@
>  #include <linux/sched.h>
>  #include <linux/tboot.h>
>  #include <linux/delay.h>
> +#include <linux/stacktool.h>
>  #include <acpi/reboot.h>
>  #include <asm/io.h>
>  #include <asm/apic.h>
> @@ -97,11 +98,13 @@ void __noreturn machine_real_restart(unsigned int type)
>  
>  	/* Jump to the identity-mapped low memory code */
>  #ifdef CONFIG_X86_32
> -	asm volatile("jmpl *%0" : :
> +	asm volatile(STACKTOOL_IGNORE_INSN
> +		     "jmpl *%0;" : :
>  		     "rm" (real_mode_header->machine_real_restart_asm),
>  		     "a" (type));
>  #else
> -	asm volatile("ljmpl *%0" : :
> +	asm volatile(STACKTOOL_IGNORE_INSN
> +		     "ljmpl *%0" : :
>  		     "m" (real_mode_header->machine_real_restart_asm),
>  		     "D" (type));
>  #endif

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?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [next] | [standalone]

#1307683

On Tue, Jan 12, 2016 at 05:47:11PM +0100, Borislav Petkov wrote:
> On Fri, Dec 18, 2015 at 06:39:27AM -0600, Josh Poimboeuf wrote:
> > stacktool reports a false positive warning for the ljmp instruction in
> > machine_real_restart().  Normally, ljmp isn't allowed in a function, but
> > this is a special case where it's jumping into real mode.
> > 
> > Add the jumps to a whitelist which tells stacktool to ignore them.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  arch/x86/kernel/reboot.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > index 02693dd..1ea1c5e 100644
> > --- a/arch/x86/kernel/reboot.c
> > +++ b/arch/x86/kernel/reboot.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/tboot.h>
> >  #include <linux/delay.h>
> > +#include <linux/stacktool.h>
> >  #include <acpi/reboot.h>
> >  #include <asm/io.h>
> >  #include <asm/apic.h>
> > @@ -97,11 +98,13 @@ void __noreturn machine_real_restart(unsigned int type)
> >  
> >  	/* Jump to the identity-mapped low memory code */
> >  #ifdef CONFIG_X86_32
> > -	asm volatile("jmpl *%0" : :
> > +	asm volatile(STACKTOOL_IGNORE_INSN
> > +		     "jmpl *%0;" : :
> >  		     "rm" (real_mode_header->machine_real_restart_asm),
> >  		     "a" (type));
> >  #else
> > -	asm volatile("ljmpl *%0" : :
> > +	asm volatile(STACKTOOL_IGNORE_INSN
> > +		     "ljmpl *%0" : :
> >  		     "m" (real_mode_header->machine_real_restart_asm),
> >  		     "D" (type));
> >  #endif
> 
> 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."

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.

It's also more resilient to future code changes, e.g. if the offending
instruction gets moved or if the function gets renamed.

And it gives you the ability to more granularly whitelist instructions
rather than entire functions, which could cause other offending stack
violations in the function to get overlooked.

Another thing is that stacktool could be a nice general purpose tool for
finding stack issues in other code bases, and so I think requiring it to
have hard-coded knowledge about the code base would greatly limit its
general usefulness.  (Though maybe this problem could be remediated with
a user-provided whitelist file which lists functions to be ignored.)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1307689

On Tue, Jan 12, 2016 at 11:43:01AM -0600, Josh Poimboeuf wrote:
> 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."

Weird for whom?

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

I don't think so. All that unnecessary clutter just gets in the way
of actually writing code. Sure, those tools are all good and nice but
again, they should *not* have to touch the code. We want people to
concentrate on writing code, not paying attention to gazillion tools
breaking from their changes.

> It's also more resilient to future code changes, e.g. if the offending
> instruction gets moved or if the function gets renamed.

So make your tool parse vmlinux properly.

> And it gives you the ability to more granularly whitelist instructions
> rather than entire functions, which could cause other offending stack
> violations in the function to get overlooked.

No no no, the moment you have to *whitelist* something is already wrong.
Normal kernel code shouldn't have to whitelist anything - the tools
should strive to be smart, instead.

> Another thing is that stacktool could be a nice general purpose tool for
> finding stack issues in other code bases, and so I think requiring it to
> have hard-coded knowledge about the code base would greatly limit its
> general usefulness.  (Though maybe this problem could be remediated with
> a user-provided whitelist file which lists functions to be ignored.)

I can use the same argument for me: all those other code bases would
need annotating too.

Again, the onus should always be on the tool to do the right thing.
If it cannot, it should not say anything.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1307746

On Tue, Jan 12, 2016 at 06:55:40PM +0100, Borislav Petkov wrote:
> On Tue, Jan 12, 2016 at 11:43:01AM -0600, Josh Poimboeuf wrote:
> > 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."
> 
> Weird for whom?

Well, for example, here are the 3 places in the code where I needed to
use STACKTOOL_IGNORE:

- machine_real_restart() does an "ljmp" (far jump) into real mode and
  never returns

- xen_setup_gdt() does an "lret" (far return)

- xen_cpuid() inserts some fake xen-specific (non-x86) asm instructions

I'd say those are all weird things that C code should _normally_ not do
(especially emitting fake instructions!).  But yet they should be
allowed for those specific rare cases where we can manually verify that
they won't cause problems.

> > 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.
> 
> I don't think so. All that unnecessary clutter just gets in the way
> of actually writing code. Sure, those tools are all good and nice but
> again, they should *not* have to touch the code. We want people to
> concentrate on writing code, not paying attention to gazillion tools
> breaking from their changes.

Generally I agree (but I don't know what other tools you're talking
about which require adding clutter).  We don't want the tools to get in
the way.

I've tried *very* hard to make stacktool as unobtrusive as possible.
And I think I've done a good job at it.  As I said there's hopefully
only a handful of code locations which need the STACKTOOL_IGNORE stuff.

> > It's also more resilient to future code changes, e.g. if the offending
> > instruction gets moved or if the function gets renamed.
> 
> So make your tool parse vmlinux properly.
> 
> > And it gives you the ability to more granularly whitelist instructions
> > rather than entire functions, which could cause other offending stack
> > violations in the function to get overlooked.
> 
> No no no, the moment you have to *whitelist* something is already wrong.
> Normal kernel code shouldn't have to whitelist anything - the tools
> should strive to be smart, instead.

The tool *does* strive to be smart.  It disassembles every instruction
in both C and asm object files, follows every code path, understands the
"alternatives" hot-patching stuff, jump tables, exception tables, gcc
switch tables, tail function calls, etc, etc, etc.  That's why we're now
at v15 :-)

Taking a hard line of "no whitelists" is just not very realistic when
you take into account some of the special cases in the kernel.  For
example, how can it possibly grok a fake xen instruction without some
kind of a whitelist, either hard-coded in the tool or annotated some
other way?

If you know of some other way to avoid all false positives without
having to whitelist them, then I'm all for it.

> > Another thing is that stacktool could be a nice general purpose tool for
> > finding stack issues in other code bases, and so I think requiring it to
> > have hard-coded knowledge about the code base would greatly limit its
> > general usefulness.  (Though maybe this problem could be remediated with
> > a user-provided whitelist file which lists functions to be ignored.)
> 
> I can use the same argument for me: all those other code bases would
> need annotating too.

Maybe so, although the kernel is more "special" than most code bases and
is thus more likely to need annotations.  But again, we really try to
keep the annotations to a minimum.  The tradeoff is worth it IMO.

> Again, the onus should always be on the tool to do the right thing.
> If it cannot, it should not say anything.

Saying nothing at all in order to prevent false positives would also by
definition allow some false negatives, which would make stacktool
useless for its intended purpose of enabling reliable stack traces.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1307765

On Tue, Jan 12, 2016 at 12:56:06PM -0600, Josh Poimboeuf wrote:
> I'd say those are all weird things that C code should _normally_ not do
> (especially emitting fake instructions!).

The kernel does weird things, that's fine.

> Generally I agree (but I don't know what other tools you're talking
> about which require adding clutter).

kasan and kmemleak, for example.

> As I said there's hopefully only a handful of code locations which
> need the STACKTOOL_IGNORE stuff.

Can you get rid of them too?

> For example, how can it possibly grok a fake xen instruction without
> some kind of a whitelist, either hard-coded in the tool or annotated
> some other way?

I'd much prefer a whitelist which the tool parses, loads, etc, if you
don't want to hardcode it, to annotating kernel code.

> For example, how can it possibly grok a fake xen instruction without
> some kind of a whitelist, either hard-coded in the tool or annotated
> some other way?

Pointer to the place? I could take a look when I get a chance.

> Saying nothing at all in order to prevent false positives would also
> by definition allow some false negatives, which would make stacktool
> useless for its intended purpose of enabling reliable stack traces.

I'd take output from the tool anyday of the week which says something
like: "Looka here, this looks funny, you might want to do something
about it." than imposing annotations on code.

It might even move people into rewriting the code into tool-compliant
version.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1308285

* 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.

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

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1309883

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

[toc] | [prev] | [next] | [standalone]

#1310044

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

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

Could also be part of the "--kernel"-specific checking and you could
match the containing ELF symbol bpf_error...

> - __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.

I think if we put all those checks that under --kernel, the tool would
remain generic enough.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1310049

* Borislav Petkov <bp@alien8.de> 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'.)

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1310056

On Fri, Jan 15, 2016 at 12:00:00PM +0100, Ingo Molnar wrote:
> All of the cases Josh listed involve some sort of special case where we do 
> something non-standard. (Where 'standard' == 'regular kernel C function'.)

My only worry is that next time we have to do something non-standard,
we'll have to annotate it as well. bpf_jit case-in-point.

OTOH, I guess the generic, tool-agnostic annotation could be a step
in the (more-)right direction as we're saying "we're doing something
special here, and we're stating that fact with this here annotation."

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1310058

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jan 15, 2016 at 12:00:00PM +0100, Ingo Molnar wrote:
> > All of the cases Josh listed involve some sort of special case where we do 
> > something non-standard. (Where 'standard' == 'regular kernel C function'.)
> 
> My only worry is that next time we have to do something non-standard,
> we'll have to annotate it as well. bpf_jit case-in-point.

but that's generally OK: it's not a problem if something non-standard is visibly 
non-standard on the source code level as well.

> OTOH, I guess the generic, tool-agnostic annotation could be a step in the 
> (more-)right direction as we're saying "we're doing something special here, and 
> we're stating that fact with this here annotation."

Yeah, I think so too!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1312890

On Fri, Jan 15, 2016 at 12:00:00PM +0100, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp@alien8.de> 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

[toc] | [prev] | [next] | [standalone]

#1312898

On 01/19/16 21:42, Josh Poimboeuf wrote:
>>>>
>>>>   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.
>>>>   

Cute.  UD2 followed by three ASCII characters.  I guess that becomes
really fun when using UD2 + a string for error reporting.  Of course, no
software does that...

	-hpa

[toc] | [prev] | [next] | [standalone]

#1312912

On Tue, Jan 19, 2016 at 09:50:31PM -0800, H. Peter Anvin wrote:
> On 01/19/16 21:42, Josh Poimboeuf wrote:
> >>>>
> >>>>   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.
> >>>>   
> 
> Cute.  UD2 followed by three ASCII characters.  I guess that becomes
> really fun when using UD2 + a string for error reporting.  Of course, no
> software does that...

Ah, I completely missed the fact that they're ASCII.  And it spells
"xen" of course :-)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1313099

On Tue, Jan 19, 2016 at 11:42:56PM -0600, Josh Poimboeuf wrote:
> 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!

Oh wow! :-)

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

Sounds nice.

Thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1310048

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

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

Yeah.

So the worry people have is that once such annotations get in, the incentive to 
solve them at the tooling level decreases and stays on a TODO list for a long time 
;-)

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

Yeah, probably. Annotating Xen special code looks acceptable.

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

I think annotation is fine in this case.

> - __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.

That is obviously a special case too.

As long as no 'usual' C function gets annotated as doing something weird with the 
stack frame, I'm fine with this approach.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web