* [PATCH 22/33] x86/asm/bpf: Annotate callable functions
2016-01-21 22:49 [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf
@ 2016-01-21 22:49 ` Josh Poimboeuf
2016-01-21 22:49 ` [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S Josh Poimboeuf
` (4 subsequent siblings)
5 siblings, 0 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2016-01-21 22:49 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
Cc: linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Jiri Slaby, Arnaldo Carvalho de Melo,
Alexei Starovoitov, netdev
bpf_jit.S has several functions which can be called from C code. Give
them proper ELF annotations.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: netdev@vger.kernel.org
---
arch/x86/net/bpf_jit.S | 39 ++++++++++++++++-----------------------
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
index 4093216..eb4a3bd 100644
--- a/arch/x86/net/bpf_jit.S
+++ b/arch/x86/net/bpf_jit.S
@@ -22,15 +22,16 @@
32 /* space for rbx,r13,r14,r15 */ + \
8 /* space for skb_copy_bits */)
-sk_load_word:
- .globl sk_load_word
+#define FUNC(name) \
+ .globl name; \
+ .type name, @function; \
+ name:
+FUNC(sk_load_word)
test %esi,%esi
js bpf_slow_path_word_neg
-sk_load_word_positive_offset:
- .globl sk_load_word_positive_offset
-
+FUNC(sk_load_word_positive_offset)
mov %r9d,%eax # hlen
sub %esi,%eax # hlen - offset
cmp $3,%eax
@@ -39,15 +40,11 @@ sk_load_word_positive_offset:
bswap %eax /* ntohl() */
ret
-sk_load_half:
- .globl sk_load_half
-
+FUNC(sk_load_half)
test %esi,%esi
js bpf_slow_path_half_neg
-sk_load_half_positive_offset:
- .globl sk_load_half_positive_offset
-
+FUNC(sk_load_half_positive_offset)
mov %r9d,%eax
sub %esi,%eax # hlen - offset
cmp $1,%eax
@@ -56,15 +53,11 @@ sk_load_half_positive_offset:
rol $8,%ax # ntohs()
ret
-sk_load_byte:
- .globl sk_load_byte
-
+FUNC(sk_load_byte)
test %esi,%esi
js bpf_slow_path_byte_neg
-sk_load_byte_positive_offset:
- .globl sk_load_byte_positive_offset
-
+FUNC(sk_load_byte_positive_offset)
cmp %esi,%r9d /* if (offset >= hlen) goto bpf_slow_path_byte */
jle bpf_slow_path_byte
movzbl (SKBDATA,%rsi),%eax
@@ -120,8 +113,8 @@ bpf_slow_path_byte:
bpf_slow_path_word_neg:
cmp SKF_MAX_NEG_OFF, %esi /* test range */
jl bpf_error /* offset lower -> error */
-sk_load_word_negative_offset:
- .globl sk_load_word_negative_offset
+
+FUNC(sk_load_word_negative_offset)
sk_negative_common(4)
mov (%rax), %eax
bswap %eax
@@ -130,8 +123,8 @@ sk_load_word_negative_offset:
bpf_slow_path_half_neg:
cmp SKF_MAX_NEG_OFF, %esi
jl bpf_error
-sk_load_half_negative_offset:
- .globl sk_load_half_negative_offset
+
+FUNC(sk_load_half_negative_offset)
sk_negative_common(2)
mov (%rax),%ax
rol $8,%ax
@@ -141,8 +134,8 @@ sk_load_half_negative_offset:
bpf_slow_path_byte_neg:
cmp SKF_MAX_NEG_OFF, %esi
jl bpf_error
-sk_load_byte_negative_offset:
- .globl sk_load_byte_negative_offset
+
+FUNC(sk_load_byte_negative_offset)
sk_negative_common(1)
movzbl (%rax), %eax
ret
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-21 22:49 [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf
2016-01-21 22:49 ` [PATCH 22/33] x86/asm/bpf: Annotate callable functions Josh Poimboeuf
@ 2016-01-21 22:49 ` Josh Poimboeuf
2016-01-22 2:44 ` Alexei Starovoitov
2016-01-21 22:49 ` [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist Josh Poimboeuf
` (3 subsequent siblings)
5 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-01-21 22:49 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
Cc: linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Jiri Slaby, Arnaldo Carvalho de Melo,
Alexei Starovoitov, netdev
bpf_jit.S has several callable non-leaf functions which don't honor
CONFIG_FRAME_POINTER, which can result in bad stack traces.
Create a stack frame before the call instructions when
CONFIG_FRAME_POINTER is enabled.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: netdev@vger.kernel.org
---
arch/x86/net/bpf_jit.S | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
index eb4a3bd..f2a7faf 100644
--- a/arch/x86/net/bpf_jit.S
+++ b/arch/x86/net/bpf_jit.S
@@ -8,6 +8,7 @@
* of the License.
*/
#include <linux/linkage.h>
+#include <asm/frame.h>
/*
* Calling convention :
@@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
/* rsi contains offset and can be scratched */
#define bpf_slow_path_common(LEN) \
+ lea -MAX_BPF_STACK + 32(%rbp), %rdx;\
+ FRAME_BEGIN; \
mov %rbx, %rdi; /* arg1 == skb */ \
push %r9; \
push SKBDATA; \
/* rsi already has offset */ \
mov $LEN,%ecx; /* len */ \
- lea - MAX_BPF_STACK + 32(%rbp),%rdx; \
call skb_copy_bits; \
test %eax,%eax; \
pop SKBDATA; \
- pop %r9;
+ pop %r9; \
+ FRAME_END
bpf_slow_path_word:
@@ -99,6 +102,7 @@ bpf_slow_path_byte:
ret
#define sk_negative_common(SIZE) \
+ FRAME_BEGIN; \
mov %rbx, %rdi; /* arg1 == skb */ \
push %r9; \
push SKBDATA; \
@@ -108,6 +112,7 @@ bpf_slow_path_byte:
test %rax,%rax; \
pop SKBDATA; \
pop %r9; \
+ FRAME_END; \
jz bpf_error
bpf_slow_path_word_neg:
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-21 22:49 ` [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S Josh Poimboeuf
@ 2016-01-22 2:44 ` Alexei Starovoitov
2016-01-22 3:55 ` Josh Poimboeuf
0 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2016-01-22 2:44 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> bpf_jit.S has several callable non-leaf functions which don't honor
> CONFIG_FRAME_POINTER, which can result in bad stack traces.
>
> Create a stack frame before the call instructions when
> CONFIG_FRAME_POINTER is enabled.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: netdev@vger.kernel.org
> ---
> arch/x86/net/bpf_jit.S | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
> index eb4a3bd..f2a7faf 100644
> --- a/arch/x86/net/bpf_jit.S
> +++ b/arch/x86/net/bpf_jit.S
> @@ -8,6 +8,7 @@
> * of the License.
> */
> #include <linux/linkage.h>
> +#include <asm/frame.h>
>
> /*
> * Calling convention :
> @@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
>
> /* rsi contains offset and can be scratched */
> #define bpf_slow_path_common(LEN) \
> + lea -MAX_BPF_STACK + 32(%rbp), %rdx;\
> + FRAME_BEGIN; \
> mov %rbx, %rdi; /* arg1 == skb */ \
> push %r9; \
> push SKBDATA; \
> /* rsi already has offset */ \
> mov $LEN,%ecx; /* len */ \
> - lea - MAX_BPF_STACK + 32(%rbp),%rdx; \
> call skb_copy_bits; \
> test %eax,%eax; \
> pop SKBDATA; \
> - pop %r9;
> + pop %r9; \
> + FRAME_END
I'm not sure what above is doing.
There is already 'push rbp; mov rbp,rsp' at the beginning of generated
code and with above the stack trace will show two function at the same ip?
since there were no calls between them?
I think the stack walker will get even more confused?
Also the JIT of bpf_call insn will emit variable number of push/pop
around the call and I definitely don't want to add extra push rbp
there, since it's the critical path and callee will do its own
push rbp.
Also there are push/pops emitted around div/mod
and there is indirect goto emitted as well for bpf_tail_call
that jumps into different function body without touching
current stack.
Also none of the JITed function are dwarf annotated.
I could be missing something. I think either this patch
is not need or you need to teach the tool to ignore
all JITed stuff. I don't think it's practical to annotate
everything. Different JITs do their own magic.
s390 JIT is even more fancy.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-22 2:44 ` Alexei Starovoitov
@ 2016-01-22 3:55 ` Josh Poimboeuf
2016-01-22 4:18 ` Alexei Starovoitov
0 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-01-22 3:55 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > bpf_jit.S has several callable non-leaf functions which don't honor
> > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> >
> > Create a stack frame before the call instructions when
> > CONFIG_FRAME_POINTER is enabled.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: netdev@vger.kernel.org
> > ---
> > arch/x86/net/bpf_jit.S | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
> > index eb4a3bd..f2a7faf 100644
> > --- a/arch/x86/net/bpf_jit.S
> > +++ b/arch/x86/net/bpf_jit.S
> > @@ -8,6 +8,7 @@
> > * of the License.
> > */
> > #include <linux/linkage.h>
> > +#include <asm/frame.h>
> >
> > /*
> > * Calling convention :
> > @@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
> >
> > /* rsi contains offset and can be scratched */
> > #define bpf_slow_path_common(LEN) \
> > + lea -MAX_BPF_STACK + 32(%rbp), %rdx;\
> > + FRAME_BEGIN; \
> > mov %rbx, %rdi; /* arg1 == skb */ \
> > push %r9; \
> > push SKBDATA; \
> > /* rsi already has offset */ \
> > mov $LEN,%ecx; /* len */ \
> > - lea - MAX_BPF_STACK + 32(%rbp),%rdx; \
> > call skb_copy_bits; \
> > test %eax,%eax; \
> > pop SKBDATA; \
> > - pop %r9;
> > + pop %r9; \
> > + FRAME_END
>
> I'm not sure what above is doing.
> There is already 'push rbp; mov rbp,rsp' at the beginning of generated
> code and with above the stack trace will show two function at the same ip?
> since there were no calls between them?
> I think the stack walker will get even more confused?
> Also the JIT of bpf_call insn will emit variable number of push/pop
> around the call and I definitely don't want to add extra push rbp
> there, since it's the critical path and callee will do its own
> push rbp.
> Also there are push/pops emitted around div/mod
> and there is indirect goto emitted as well for bpf_tail_call
> that jumps into different function body without touching
> current stack.
Hm, I'm not sure I follow. Let me try to explain my understanding.
As you mentioned, the generated code sets up the frame pointer. From
emit_prologue():
EMIT1(0x55); /* push rbp */
EMIT3(0x48, 0x89, 0xE5); /* mov rbp,rsp */
And then later, do_jit() can generate a call into the functions in
bpf_jit.S. For example:
func = CHOOSE_LOAD_FUNC(imm32, sk_load_word);
...
EMIT1_off32(0xE8, jmp_offset); /* call */
So the functions in bpf_jit.S are being called by the generated code.
They're not part of the generated code itself. So they're callees and
need to create their own stack frame before they call out to somewhere
else.
Or did I miss something?
> Also none of the JITed function are dwarf annotated.
But what does that have to do with frame pointers?
> I could be missing something. I think either this patch
> is not need or you need to teach the tool to ignore
> all JITed stuff. I don't think it's practical to annotate
> everything. Different JITs do their own magic.
> s390 JIT is even more fancy.
Well, but the point of these patches isn't to make the tool happy. It's
really to make sure that runtime stack traces can be made reliable.
Maybe I'm missing something but I don't see why JIT code can't honor
CONFIG_FRAME_POINTER just like any other code.
--
Josh
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-22 3:55 ` Josh Poimboeuf
@ 2016-01-22 4:18 ` Alexei Starovoitov
2016-01-22 7:36 ` Ingo Molnar
2016-01-22 15:58 ` Josh Poimboeuf
0 siblings, 2 replies; 38+ messages in thread
From: Alexei Starovoitov @ 2016-01-22 4:18 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > >
> > > Create a stack frame before the call instructions when
> > > CONFIG_FRAME_POINTER is enabled.
> > >
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > Cc: netdev@vger.kernel.org
> > > ---
> > > arch/x86/net/bpf_jit.S | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
> > > index eb4a3bd..f2a7faf 100644
> > > --- a/arch/x86/net/bpf_jit.S
> > > +++ b/arch/x86/net/bpf_jit.S
> > > @@ -8,6 +8,7 @@
> > > * of the License.
> > > */
> > > #include <linux/linkage.h>
> > > +#include <asm/frame.h>
> > >
> > > /*
> > > * Calling convention :
> > > @@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
> > >
> > > /* rsi contains offset and can be scratched */
> > > #define bpf_slow_path_common(LEN) \
> > > + lea -MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > + FRAME_BEGIN; \
> > > mov %rbx, %rdi; /* arg1 == skb */ \
> > > push %r9; \
> > > push SKBDATA; \
> > > /* rsi already has offset */ \
> > > mov $LEN,%ecx; /* len */ \
> > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx; \
> > > call skb_copy_bits; \
> > > test %eax,%eax; \
> > > pop SKBDATA; \
> > > - pop %r9;
> > > + pop %r9; \
> > > + FRAME_END
> >
> > I'm not sure what above is doing.
> > There is already 'push rbp; mov rbp,rsp' at the beginning of generated
> > code and with above the stack trace will show two function at the same ip?
> > since there were no calls between them?
> > I think the stack walker will get even more confused?
> > Also the JIT of bpf_call insn will emit variable number of push/pop
> > around the call and I definitely don't want to add extra push rbp
> > there, since it's the critical path and callee will do its own
> > push rbp.
> > Also there are push/pops emitted around div/mod
> > and there is indirect goto emitted as well for bpf_tail_call
> > that jumps into different function body without touching
> > current stack.
>
> Hm, I'm not sure I follow. Let me try to explain my understanding.
>
> As you mentioned, the generated code sets up the frame pointer. From
> emit_prologue():
>
> EMIT1(0x55); /* push rbp */
> EMIT3(0x48, 0x89, 0xE5); /* mov rbp,rsp */
>
> And then later, do_jit() can generate a call into the functions in
> bpf_jit.S. For example:
>
> func = CHOOSE_LOAD_FUNC(imm32, sk_load_word);
> ...
> EMIT1_off32(0xE8, jmp_offset); /* call */
>
> So the functions in bpf_jit.S are being called by the generated code.
> They're not part of the generated code itself. So they're callees and
> need to create their own stack frame before they call out to somewhere
> else.
>
> Or did I miss something?
yes. all correct.
This particular patch is ok, since it adds to
bpf_slow_path_common and as the name says it's slow and rare,
but wanted to make sure the rest of it is understood.
> > Also none of the JITed function are dwarf annotated.
>
> But what does that have to do with frame pointers?
nothing, but then why do you need
.type name, @function
annotations in another patch?
> > I could be missing something. I think either this patch
> > is not need or you need to teach the tool to ignore
> > all JITed stuff. I don't think it's practical to annotate
> > everything. Different JITs do their own magic.
> > s390 JIT is even more fancy.
>
> Well, but the point of these patches isn't to make the tool happy. It's
> really to make sure that runtime stack traces can be made reliable.
> Maybe I'm missing something but I don't see why JIT code can't honor
> CONFIG_FRAME_POINTER just like any other code.
It can if there is no performance cost added.
I can speak for x64 JIT, but the rest needs to be analyzed as well.
My point was that may be it's easier to ignore all JITed code and
just say that such call stacks may be unreliable?
live-patching is not applicable to JITed code anyway
or you want to livepatch the callees of it?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-22 4:18 ` Alexei Starovoitov
@ 2016-01-22 7:36 ` Ingo Molnar
2016-01-22 15:58 ` Josh Poimboeuf
1 sibling, 0 replies; 38+ messages in thread
From: Ingo Molnar @ 2016-01-22 7:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Jiri Slaby, Arnaldo Carvalho de Melo,
Alexei Starovoitov, netdev, daniel
* Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > I could be missing something. I think either this patch is not need or you
> > > need to teach the tool to ignore all JITed stuff. I don't think it's
> > > practical to annotate everything. Different JITs do their own magic. s390
> > > JIT is even more fancy.
> >
> > Well, but the point of these patches isn't to make the tool happy. It's
> > really to make sure that runtime stack traces can be made reliable. Maybe I'm
> > missing something but I don't see why JIT code can't honor
> > CONFIG_FRAME_POINTER just like any other code.
>
> It can if there is no performance cost added. I can speak for x64 JIT, but the
> rest needs to be analyzed as well. My point was that may be it's easier to
> ignore all JITed code and just say that such call stacks may be unreliable?
> live-patching is not applicable to JITed code anyway or you want to livepatch
> the callees of it?
So the rule is that if frame pointers are enabled all kernel code should have
correct stack frames - in case an IRQ (or NMI) hits it or it crashes.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-22 4:18 ` Alexei Starovoitov
2016-01-22 7:36 ` Ingo Molnar
@ 2016-01-22 15:58 ` Josh Poimboeuf
2016-01-22 17:18 ` Alexei Starovoitov
1 sibling, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-01-22 15:58 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:
> On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> > On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > >
> > > > Create a stack frame before the call instructions when
> > > > CONFIG_FRAME_POINTER is enabled.
> > > >
> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > Cc: netdev@vger.kernel.org
> > > > ---
> > > > arch/x86/net/bpf_jit.S | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
> > > > index eb4a3bd..f2a7faf 100644
> > > > --- a/arch/x86/net/bpf_jit.S
> > > > +++ b/arch/x86/net/bpf_jit.S
> > > > @@ -8,6 +8,7 @@
> > > > * of the License.
> > > > */
> > > > #include <linux/linkage.h>
> > > > +#include <asm/frame.h>
> > > >
> > > > /*
> > > > * Calling convention :
> > > > @@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
> > > >
> > > > /* rsi contains offset and can be scratched */
> > > > #define bpf_slow_path_common(LEN) \
> > > > + lea -MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > > + FRAME_BEGIN; \
> > > > mov %rbx, %rdi; /* arg1 == skb */ \
> > > > push %r9; \
> > > > push SKBDATA; \
> > > > /* rsi already has offset */ \
> > > > mov $LEN,%ecx; /* len */ \
> > > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx; \
> > > > call skb_copy_bits; \
> > > > test %eax,%eax; \
> > > > pop SKBDATA; \
> > > > - pop %r9;
> > > > + pop %r9; \
> > > > + FRAME_END
> > >
> > > I'm not sure what above is doing.
> > > There is already 'push rbp; mov rbp,rsp' at the beginning of generated
> > > code and with above the stack trace will show two function at the same ip?
> > > since there were no calls between them?
> > > I think the stack walker will get even more confused?
> > > Also the JIT of bpf_call insn will emit variable number of push/pop
> > > around the call and I definitely don't want to add extra push rbp
> > > there, since it's the critical path and callee will do its own
> > > push rbp.
> > > Also there are push/pops emitted around div/mod
> > > and there is indirect goto emitted as well for bpf_tail_call
> > > that jumps into different function body without touching
> > > current stack.
> >
> > Hm, I'm not sure I follow. Let me try to explain my understanding.
> >
> > As you mentioned, the generated code sets up the frame pointer. From
> > emit_prologue():
> >
> > EMIT1(0x55); /* push rbp */
> > EMIT3(0x48, 0x89, 0xE5); /* mov rbp,rsp */
> >
> > And then later, do_jit() can generate a call into the functions in
> > bpf_jit.S. For example:
> >
> > func = CHOOSE_LOAD_FUNC(imm32, sk_load_word);
> > ...
> > EMIT1_off32(0xE8, jmp_offset); /* call */
> >
> > So the functions in bpf_jit.S are being called by the generated code.
> > They're not part of the generated code itself. So they're callees and
> > need to create their own stack frame before they call out to somewhere
> > else.
> >
> > Or did I miss something?
>
> yes. all correct.
> This particular patch is ok, since it adds to
> bpf_slow_path_common and as the name says it's slow and rare,
> but wanted to make sure the rest of it is understood.
>
> > > Also none of the JITed function are dwarf annotated.
> >
> > But what does that have to do with frame pointers?
>
> nothing, but then why do you need
> .type name, @function
> annotations in another patch?
Those are ELF function annotations which are needed so that stacktool
can find and analyze all callable functions (and they're a good idea
anyway for the sake of other tooling).
Obviously we can't annotate the JIT functions which have no name, but
that's ok. As long as they're doing the right thing with respect to
frame pointers, the stack dump code will still be able to see their
frames (and that they're associated with generated code).
> > > I could be missing something. I think either this patch
> > > is not need or you need to teach the tool to ignore
> > > all JITed stuff. I don't think it's practical to annotate
> > > everything. Different JITs do their own magic.
> > > s390 JIT is even more fancy.
> >
> > Well, but the point of these patches isn't to make the tool happy. It's
> > really to make sure that runtime stack traces can be made reliable.
> > Maybe I'm missing something but I don't see why JIT code can't honor
> > CONFIG_FRAME_POINTER just like any other code.
>
> It can if there is no performance cost added.
CONFIG_FRAME_POINTER always adds a small performance cost but as you
mentioned it only affects the slow path here. And hopefully we'll soon
have an in-kernel DWARF unwinder on x86 so we can get rid of the need
for frame pointers.
> I can speak for x64 JIT, but the rest needs to be analyzed as well.
> My point was that may be it's easier to ignore all JITed code and
> just say that such call stacks may be unreliable?
> live-patching is not applicable to JITed code anyway
> or you want to livepatch the callees of it?
Right. We can't patch JIT code, but we might need to patch other
functions on the call stack (including callees and callers).
--
Josh
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-22 15:58 ` Josh Poimboeuf
@ 2016-01-22 17:18 ` Alexei Starovoitov
2016-01-22 17:36 ` Josh Poimboeuf
0 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2016-01-22 17:18 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Fri, Jan 22, 2016 at 09:58:04AM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:
> > On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> > > On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > > > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > > >
> > > > > Create a stack frame before the call instructions when
> > > > > CONFIG_FRAME_POINTER is enabled.
> > > > >
> > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > > Cc: netdev@vger.kernel.org
> > > > > ---
> > > > > arch/x86/net/bpf_jit.S | 9 +++++++--
...
> > > > > /* rsi contains offset and can be scratched */
> > > > > #define bpf_slow_path_common(LEN) \
> > > > > + lea -MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > > > + FRAME_BEGIN; \
> > > > > mov %rbx, %rdi; /* arg1 == skb */ \
> > > > > push %r9; \
> > > > > push SKBDATA; \
> > > > > /* rsi already has offset */ \
> > > > > mov $LEN,%ecx; /* len */ \
> > > > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx; \
> > > > > call skb_copy_bits; \
> > > > > test %eax,%eax; \
> > > > > pop SKBDATA; \
> > > > > - pop %r9;
> > > > > + pop %r9; \
> > > > > + FRAME_END
...
> > > Well, but the point of these patches isn't to make the tool happy. It's
> > > really to make sure that runtime stack traces can be made reliable.
> > > Maybe I'm missing something but I don't see why JIT code can't honor
> > > CONFIG_FRAME_POINTER just like any other code.
> >
> > It can if there is no performance cost added.
>
> CONFIG_FRAME_POINTER always adds a small performance cost but as you
> mentioned it only affects the slow path here. And hopefully we'll soon
> have an in-kernel DWARF unwinder on x86 so we can get rid of the need
> for frame pointers.
ok. fair enough.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-22 17:18 ` Alexei Starovoitov
@ 2016-01-22 17:36 ` Josh Poimboeuf
2016-01-22 17:40 ` Alexei Starovoitov
0 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-01-22 17:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Fri, Jan 22, 2016 at 09:18:23AM -0800, Alexei Starovoitov wrote:
> On Fri, Jan 22, 2016 at 09:58:04AM -0600, Josh Poimboeuf wrote:
> > On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:
> > > On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> > > > On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > > > > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > > > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > > > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > > > >
> > > > > > Create a stack frame before the call instructions when
> > > > > > CONFIG_FRAME_POINTER is enabled.
> > > > > >
> > > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > > > Cc: netdev@vger.kernel.org
> > > > > > ---
> > > > > > arch/x86/net/bpf_jit.S | 9 +++++++--
> ...
> > > > > > /* rsi contains offset and can be scratched */
> > > > > > #define bpf_slow_path_common(LEN) \
> > > > > > + lea -MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > > > > + FRAME_BEGIN; \
> > > > > > mov %rbx, %rdi; /* arg1 == skb */ \
> > > > > > push %r9; \
> > > > > > push SKBDATA; \
> > > > > > /* rsi already has offset */ \
> > > > > > mov $LEN,%ecx; /* len */ \
> > > > > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx; \
> > > > > > call skb_copy_bits; \
> > > > > > test %eax,%eax; \
> > > > > > pop SKBDATA; \
> > > > > > - pop %r9;
> > > > > > + pop %r9; \
> > > > > > + FRAME_END
> ...
> > > > Well, but the point of these patches isn't to make the tool happy. It's
> > > > really to make sure that runtime stack traces can be made reliable.
> > > > Maybe I'm missing something but I don't see why JIT code can't honor
> > > > CONFIG_FRAME_POINTER just like any other code.
> > >
> > > It can if there is no performance cost added.
> >
> > CONFIG_FRAME_POINTER always adds a small performance cost but as you
> > mentioned it only affects the slow path here. And hopefully we'll soon
> > have an in-kernel DWARF unwinder on x86 so we can get rid of the need
> > for frame pointers.
>
> ok. fair enough.
> Acked-by: Alexei Starovoitov <ast@kernel.org>
Thanks!
Can I assume your ack also applies to the previous patch which adds the
ELF annotations ("x86/asm/bpf: Annotate callable functions")?
--
Josh
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S
2016-01-22 17:36 ` Josh Poimboeuf
@ 2016-01-22 17:40 ` Alexei Starovoitov
0 siblings, 0 replies; 38+ messages in thread
From: Alexei Starovoitov @ 2016-01-22 17:40 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Fri, Jan 22, 2016 at 11:36:14AM -0600, Josh Poimboeuf wrote:
> On Fri, Jan 22, 2016 at 09:18:23AM -0800, Alexei Starovoitov wrote:
> > On Fri, Jan 22, 2016 at 09:58:04AM -0600, Josh Poimboeuf wrote:
> > > On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:
> > > > On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> > > > > On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > > > > > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > > > > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > > > > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > > > > >
> > > > > > > Create a stack frame before the call instructions when
> > > > > > > CONFIG_FRAME_POINTER is enabled.
> > > > > > >
> > > > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > > > > Cc: netdev@vger.kernel.org
> > > > > > > ---
> > > > > > > arch/x86/net/bpf_jit.S | 9 +++++++--
> > ...
> > > > > > > /* rsi contains offset and can be scratched */
> > > > > > > #define bpf_slow_path_common(LEN) \
> > > > > > > + lea -MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > > > > > + FRAME_BEGIN; \
> > > > > > > mov %rbx, %rdi; /* arg1 == skb */ \
> > > > > > > push %r9; \
> > > > > > > push SKBDATA; \
> > > > > > > /* rsi already has offset */ \
> > > > > > > mov $LEN,%ecx; /* len */ \
> > > > > > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx; \
> > > > > > > call skb_copy_bits; \
> > > > > > > test %eax,%eax; \
> > > > > > > pop SKBDATA; \
> > > > > > > - pop %r9;
> > > > > > > + pop %r9; \
> > > > > > > + FRAME_END
> > ...
> > > > > Well, but the point of these patches isn't to make the tool happy. It's
> > > > > really to make sure that runtime stack traces can be made reliable.
> > > > > Maybe I'm missing something but I don't see why JIT code can't honor
> > > > > CONFIG_FRAME_POINTER just like any other code.
> > > >
> > > > It can if there is no performance cost added.
> > >
> > > CONFIG_FRAME_POINTER always adds a small performance cost but as you
> > > mentioned it only affects the slow path here. And hopefully we'll soon
> > > have an in-kernel DWARF unwinder on x86 so we can get rid of the need
> > > for frame pointers.
> >
> > ok. fair enough.
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
>
> Thanks!
>
> Can I assume your ack also applies to the previous patch which adds the
> ELF annotations ("x86/asm/bpf: Annotate callable functions")?
Yes. Thanks.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
2016-01-21 22:49 [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf
2016-01-21 22:49 ` [PATCH 22/33] x86/asm/bpf: Annotate callable functions Josh Poimboeuf
2016-01-21 22:49 ` [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S Josh Poimboeuf
@ 2016-01-21 22:49 ` Josh Poimboeuf
2016-01-21 22:57 ` Daniel Borkmann
2016-01-22 2:55 ` Alexei Starovoitov
2016-01-22 17:43 ` [PATCH 00/33] Compile-time stack metadata validation Chris J Arges
` (2 subsequent siblings)
5 siblings, 2 replies; 38+ messages in thread
From: Josh Poimboeuf @ 2016-01-21 22:49 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
Cc: linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Jiri Slaby, Arnaldo Carvalho de Melo,
Alexei Starovoitov, netdev
stacktool reports the following false positive warnings:
stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
[...]
It's confused by the following dynamic jump instruction in
__bpf_prog_run()::
jmp *(%r12,%rax,8)
which corresponds to the following line in the C code:
goto *jumptable[insn->code];
There's no way for stacktool to deterministically find all possible
branch targets for a dynamic jump, so it can't verify this code.
In this case the jumps all stay within the function, and there's nothing
unusual going on related to the stack, so we can whitelist the function.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: netdev@vger.kernel.org
---
kernel/bpf/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 972d9a8..7108a96 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -27,6 +27,7 @@
#include <linux/random.h>
#include <linux/moduleloader.h>
#include <linux/bpf.h>
+#include <linux/stacktool.h>
#include <asm/unaligned.h>
@@ -649,6 +650,7 @@ load_byte:
WARN_RATELIMIT(1, "unknown opcode %02x\n", insn->code);
return 0;
}
+STACKTOOL_IGNORE_FUNC(__bpf_prog_run);
bool bpf_prog_array_compatible(struct bpf_array *array,
const struct bpf_prog *fp)
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
2016-01-21 22:49 ` [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist Josh Poimboeuf
@ 2016-01-21 22:57 ` Daniel Borkmann
2016-01-22 2:55 ` Alexei Starovoitov
1 sibling, 0 replies; 38+ messages in thread
From: Daniel Borkmann @ 2016-01-21 22:57 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
Cc: linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Jiri Slaby, Arnaldo Carvalho de Melo,
Alexei Starovoitov, netdev
On 01/21/2016 11:49 PM, Josh Poimboeuf wrote:
> stacktool reports the following false positive warnings:
>
> stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
> stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
> stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
> [...]
>
> It's confused by the following dynamic jump instruction in
> __bpf_prog_run()::
>
> jmp *(%r12,%rax,8)
>
> which corresponds to the following line in the C code:
>
> goto *jumptable[insn->code];
>
> There's no way for stacktool to deterministically find all possible
> branch targets for a dynamic jump, so it can't verify this code.
>
> In this case the jumps all stay within the function, and there's nothing
> unusual going on related to the stack, so we can whitelist the function.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: netdev@vger.kernel.org
Fine by me:
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
2016-01-21 22:49 ` [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist Josh Poimboeuf
2016-01-21 22:57 ` Daniel Borkmann
@ 2016-01-22 2:55 ` Alexei Starovoitov
2016-01-22 4:13 ` Josh Poimboeuf
1 sibling, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2016-01-22 2:55 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Thu, Jan 21, 2016 at 04:49:35PM -0600, Josh Poimboeuf wrote:
> stacktool reports the following false positive warnings:
>
> stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
> stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
> stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
> [...]
>
> It's confused by the following dynamic jump instruction in
> __bpf_prog_run()::
>
> jmp *(%r12,%rax,8)
>
> which corresponds to the following line in the C code:
>
> goto *jumptable[insn->code];
>
> There's no way for stacktool to deterministically find all possible
> branch targets for a dynamic jump, so it can't verify this code.
>
> In this case the jumps all stay within the function, and there's nothing
> unusual going on related to the stack, so we can whitelist the function.
well, few things are very unusual in this function.
did you see what JMP_CALL does? it's a call into a different function,
but not like typical indirect call. Will it be ok as well?
In general it's not possible for any tool to identify all possible
branch targets. bpf programs can be loaded on the fly and
jumping sequence will change.
So if this marking says 'don't bother analyzing this function because
it does sane stuff' that's probably not the case.
If this marking says 'don't bother analyzing, the stack may be crazy
from here on' then it's ok.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
2016-01-22 2:55 ` Alexei Starovoitov
@ 2016-01-22 4:13 ` Josh Poimboeuf
2016-01-22 17:19 ` Alexei Starovoitov
0 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-01-22 4:13 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Thu, Jan 21, 2016 at 06:55:41PM -0800, Alexei Starovoitov wrote:
> On Thu, Jan 21, 2016 at 04:49:35PM -0600, Josh Poimboeuf wrote:
> > stacktool reports the following false positive warnings:
> >
> > stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
> > stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
> > stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
> > [...]
> >
> > It's confused by the following dynamic jump instruction in
> > __bpf_prog_run()::
> >
> > jmp *(%r12,%rax,8)
> >
> > which corresponds to the following line in the C code:
> >
> > goto *jumptable[insn->code];
> >
> > There's no way for stacktool to deterministically find all possible
> > branch targets for a dynamic jump, so it can't verify this code.
> >
> > In this case the jumps all stay within the function, and there's nothing
> > unusual going on related to the stack, so we can whitelist the function.
>
> well, few things are very unusual in this function.
> did you see what JMP_CALL does? it's a call into a different function,
> but not like typical indirect call. Will it be ok as well?
>
> In general it's not possible for any tool to identify all possible
> branch targets. bpf programs can be loaded on the fly and
> jumping sequence will change.
> So if this marking says 'don't bother analyzing this function because
> it does sane stuff' that's probably not the case.
> If this marking says 'don't bother analyzing, the stack may be crazy
> from here on' then it's ok.
So the tool doesn't need to follow all possible call targets. Instead
it just verifies that all functions follow the frame pointer convention.
That way it doesn't matter *which* function is being called because they
all do the right thing.
But it *does* need to follow all jump targets, so that it can analyze
all possible code paths within the function itself. With a dynamic
jump, it can't do that.
So the JMP_CALL is fine, but the goto *jumptable[insn->code] isn't.
(And BTW that's the only occurrence of such a dynamic jump table in the
entire kernel.)
--
Josh
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
2016-01-22 4:13 ` Josh Poimboeuf
@ 2016-01-22 17:19 ` Alexei Starovoitov
0 siblings, 0 replies; 38+ messages in thread
From: Alexei Starovoitov @ 2016-01-22 17:19 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, Alexei Starovoitov, netdev,
daniel
On Thu, Jan 21, 2016 at 10:13:02PM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 21, 2016 at 06:55:41PM -0800, Alexei Starovoitov wrote:
> > On Thu, Jan 21, 2016 at 04:49:35PM -0600, Josh Poimboeuf wrote:
> > > stacktool reports the following false positive warnings:
> > >
> > > stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
> > > stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
> > > stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
> > > [...]
> > >
> > > It's confused by the following dynamic jump instruction in
> > > __bpf_prog_run()::
> > >
> > > jmp *(%r12,%rax,8)
> > >
> > > which corresponds to the following line in the C code:
> > >
> > > goto *jumptable[insn->code];
> > >
> > > There's no way for stacktool to deterministically find all possible
> > > branch targets for a dynamic jump, so it can't verify this code.
> > >
> > > In this case the jumps all stay within the function, and there's nothing
> > > unusual going on related to the stack, so we can whitelist the function.
> >
> > well, few things are very unusual in this function.
> > did you see what JMP_CALL does? it's a call into a different function,
> > but not like typical indirect call. Will it be ok as well?
> >
> > In general it's not possible for any tool to identify all possible
> > branch targets. bpf programs can be loaded on the fly and
> > jumping sequence will change.
> > So if this marking says 'don't bother analyzing this function because
> > it does sane stuff' that's probably not the case.
> > If this marking says 'don't bother analyzing, the stack may be crazy
> > from here on' then it's ok.
>
> So the tool doesn't need to follow all possible call targets. Instead
> it just verifies that all functions follow the frame pointer convention.
> That way it doesn't matter *which* function is being called because they
> all do the right thing.
>
> But it *does* need to follow all jump targets, so that it can analyze
> all possible code paths within the function itself. With a dynamic
> jump, it can't do that.
>
> So the JMP_CALL is fine, but the goto *jumptable[insn->code] isn't.
> (And BTW that's the only occurrence of such a dynamic jump table in the
> entire kernel.)
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-01-21 22:49 [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf
` (2 preceding siblings ...)
2016-01-21 22:49 ` [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist Josh Poimboeuf
@ 2016-01-22 17:43 ` Chris J Arges
[not found] ` <20160122174348.GB29221-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-02-12 10:36 ` Jiri Slaby
2016-02-23 8:14 ` Ingo Molnar
5 siblings, 1 reply; 38+ messages in thread
From: Chris J Arges @ 2016-01-22 17:43 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Andrew Morton, Jiri Slaby,
Arnaldo Carvalho de Melo, David Vrabel, Borislav Petkov,
Konrad Rzeszutek Wilk, Boris Ostrovsky, Jeremy Fitzhardinge,
Chris Wright, Alok
On Thu, Jan 21, 2016 at 04:49:04PM -0600, Josh Poimboeuf wrote:
> This is v16 of the compile-time stack metadata validation patch set,
> along with proposed fixes for most of the warnings it found. It's based
> on the tip/master branch.
>
Josh,
Looks good, with my config [1] I do still get a few warnings building
linux/linux-next.
Here are the warnings:
$ grep ^stacktool build.log | grep -v staging
stacktool: arch/x86/kvm/vmx.o: vmx_handle_external_intr()+0x67: call without frame pointer save/setup
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: return without frame pointer restore
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x89: duplicate frame pointer save
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x8a: duplicate frame pointer setup
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: frame pointer state mismatch
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x0: frame pointer state mismatch
stacktool: fs/reiserfs/ibalance.o: .text: unexpected end of section
stacktool: fs/reiserfs/tail_conversion.o: .text: unexpected end of section
For vmx_handle_external_intr, I'm wondering if ignoring this function is the
best option.
--
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e2951b6..d19dfb2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -33,6 +33,7 @@
#include <linux/slab.h>
#include <linux/tboot.h>
#include <linux/hrtimer.h>
+#include <linux/stacktool.h>
#include "kvm_cache_regs.h"
#include "x86.h"
@@ -8398,6 +8399,7 @@ static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
} else
local_irq_enable();
}
+STACKTOOL_IGNORE_FUNC(vmx_handle_external_intr);
static bool vmx_has_high_real_mode_segbase(void)
{
--chris
[1] http://paste.ubuntu.com/14599083/
> v15 can be found here:
>
> https://lkml.kernel.org/r/cover.1450442274.git.jpoimboe@redhat.com
>
> For more information about the motivation behind this patch set, and
> more details about what it does, see the first patch changelog and
> tools/stacktool/Documentation/stack-validation.txt.
>
> Patches 1-4 add stacktool and integrate it into the kernel build.
>
> Patches 5-28 are some proposed fixes for several of the warnings
> reported by stacktool. They've been compile-tested and boot-tested in a
> VM, but I haven't attempted any meaningful testing for many of them.
>
> Patches 29-33 add some directories, files, and functions to the
> stacktool whitelist in order to silence false positive warnings.
>
> v16:
> - fix all allyesconfig warnings, except for staging
> - get rid of STACKTOOL_IGNORE_INSN which is no longer needed
> - remove several whitelists in favor of automatically whitelisting any
> function with a special instruction like ljmp, lret, or vmrun
> - split up stacktool patch into 3 parts as suggested by Ingo
> - update the global noreturn function list
> - detect noreturn function fallthroughs
> - skip weak functions in noreturn call detection logic
> - add empty function check to noreturn logic
> - allow non-section rela symbols for __ex_table sections
> - support rare switch table case with jmpq *[addr](%rip)
> - don't warn on frame pointer restore without save
> - rearrange patch order a bit
>
> v15:
> - restructure code for a new cmdline interface "stacktool check" using
> the new subcommand framework in tools/lib/subcmd
> - fix 32 bit build fail (put __sp at end) in paravirt_types.h patch 10
> which was reported by 0day
>
> v14:
> - make tools/include/linux/list.h self-sufficient
> - create FRAME_OFFSET to allow 32-bit code to be able to access function
> arguments on the stack
> - add FRAME_OFFSET usage in crypto patch 14/24: "Create stack frames in
> aesni-intel_asm.S"
> - rename "index" -> "idx" to fix build with some compilers
>
> v13:
> - LDFLAGS order fix from Chris J Arges
> - new warning fix patches from Chris J Arges
> - "--frame-pointer" -> "--check-frame-pointer"
>
> v12:
> - rename "stackvalidate" -> "stacktool"
> - move from scripts/ to tools/:
> - makefile rework
> - make a copy of the x86 insn code (and warn if the code diverges)
> - use tools/include/linux/list.h
> - move warning macros to a new warn.h file
> - change wording: "stack validation" -> "stack metadata validation"
>
> v11:
> - attempt to answer the "why" question better in the documentation and
> commit message
> - s/FP_SAVE/FRAME_BEGIN/ in documentation
>
> v10:
> - add scripts/mod to directory ignores
> - remove circular dependencies for ignored objects which are built
> before stackvalidate
> - fix CONFIG_MODVERSIONS incompatibility
>
> v9:
> - rename FRAME/ENDFRAME -> FRAME_BEGIN/FRAME_END
> - fix jump table issue for when the original instruction is a jump
> - drop paravirt thunk alignment patch
> - add maintainers to CC for proposed warning fixes
>
> v8:
> - add proposed fixes for warnings
> - fix all memory leaks
> - process ignores earlier and add more ignore checks
> - always assume POPCNT alternative is enabled
> - drop hweight inline asm fix
> - drop __schedule() ignore patch
> - change .Ltemp_\@ to .Lstackvalidate_ignore_\@ in asm macro
> - fix CONFIG_* checks in asm macros
> - add C versions of ignore macros and frame macros
> - change ";" to "\n" in C macros
> - add ifdef CONFIG_STACK_VALIDATION checks in C ignore macros
> - use numbered label in C ignore macro
> - add missing break in switch case statement in arch-x86.c
>
> v7:
> - sibling call support
> - document proposed solution for inline asm() frame pointer issues
> - say "kernel entry/exit" instead of "context switch"
> - clarify the checking of switch statement jump tables
> - discard __stackvalidate_ignore_* sections in linker script
> - use .Ltemp_\@ to get a unique label instead of static 3-digit number
> - change STACKVALIDATE_IGNORE_FUNC variable to a static
> - move STACKVALIDATE_IGNORE_INSN to arch-specific .h file
>
> v6:
> - rename asmvalidate -> stackvalidate (again)
> - gcc-generated object file support
> - recursive branch state analysis
> - external jump support
> - fixup/exception table support
> - jump label support
> - switch statement jump table support
> - added documentation
> - detection of "noreturn" dead end functions
> - added a Kbuild mechanism for skipping files and dirs
> - moved frame pointer macros to arch/x86/include/asm/frame.h
> - moved ignore macros to include/linux/stackvalidate.h
>
> v5:
> - stackvalidate -> asmvalidate
> - frame pointers only required for non-leaf functions
> - check for the use of the FP_SAVE/RESTORE macros instead of manually
> analyzing code to detect frame pointer usage
> - additional checks to ensure each function doesn't leave its boundaries
> - make the macros simpler and more flexible
> - support for analyzing ALTERNATIVE macros
> - simplified the arch interfaces in scripts/asmvalidate/arch.h
> - fixed some asmvalidate warnings
> - rebased onto latest tip asm cleanups
> - many more small changes
>
> v4:
> - Changed the default to CONFIG_STACK_VALIDATION=n, until all the asm
> code can get cleaned up.
> - Fixed a stackvalidate error path exit code issue found by Michal
> Marek.
>
> v3:
> - Added a patch to make the push/pop CFI macros arch-independent, as
> suggested by H. Peter Anvin
>
> v2:
> - Fixed memory leaks reported by Petr Mladek
>
> Cc: linux-kernel@vger.kernel.org
> Cc: live-patching@vger.kernel.org
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Pedro Alves <palves@redhat.com>
> Cc: Namhyung Kim <namhyung@gmail.com>
> Cc: Bernd Petrovitsch <bernd@petrovitsch.priv.at>
> Cc: Chris J Arges <chris.j.arges@canonical.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>
> Chris J Arges (1):
> x86/uaccess: Add stack frame output operand in get_user inline asm
>
> Josh Poimboeuf (32):
> x86/stacktool: Compile-time stack metadata validation
> kbuild/stacktool: Add CONFIG_STACK_VALIDATION option
> x86/stacktool: Enable stacktool on x86_64
> x86/stacktool: Add STACKTOOL_IGNORE_FUNC macro
> x86/xen: Add stack frame dependency to hypercall inline asm calls
> x86/asm/xen: Set ELF function type for xen_adjust_exception_frame()
> x86/asm/xen: Create stack frames in xen-asm.S
> x86/paravirt: Add stack frame dependency to PVOP inline asm calls
> x86/paravirt: Create a stack frame in PV_CALLEE_SAVE_REGS_THUNK
> x86/amd: Set ELF function type for vide()
> x86/asm/crypto: Move .Lbswap_mask data to .rodata section
> x86/asm/crypto: Move jump_table to .rodata section
> x86/asm/crypto: Simplify stack usage in sha-mb functions
> x86/asm/crypto: Don't use rbp as a scratch register
> x86/asm/crypto: Create stack frames in crypto functions
> x86/asm/entry: Create stack frames in thunk functions
> x86/asm/acpi: Create a stack frame in do_suspend_lowlevel()
> x86/asm: Create stack frames in rwsem functions
> x86/asm/efi: Create a stack frame in efi_call()
> x86/asm/power: Create stack frames in hibernate_asm_64.S
> x86/asm/bpf: Annotate callable functions
> x86/asm/bpf: Create stack frames in bpf_jit.S
> x86/kprobes: Get rid of kretprobe_trampoline_holder()
> x86/kvm: Set ELF function type for fastop functions
> x86/kvm: Add stack frame dependency to test_cc() inline asm
> watchdog/hpwdt: Create stack frame in asminline_call()
> x86/locking: Create stack frame in PV unlock
> x86/stacktool: Add directory and file whitelists
> x86/xen: Add xen_cpuid() to stacktool whitelist
> bpf: Add __bpf_prog_run() to stacktool whitelist
> sched: Add __schedule() to stacktool whitelist
> x86/kprobes: Add kretprobe_trampoline() to stacktool whitelist
>
> MAINTAINERS | 6 +
> Makefile | 5 +-
> arch/Kconfig | 6 +
> arch/x86/Kconfig | 1 +
> arch/x86/boot/Makefile | 1 +
> arch/x86/boot/compressed/Makefile | 3 +-
> arch/x86/crypto/aesni-intel_asm.S | 75 +-
> arch/x86/crypto/camellia-aesni-avx-asm_64.S | 15 +
> arch/x86/crypto/camellia-aesni-avx2-asm_64.S | 15 +
> arch/x86/crypto/cast5-avx-x86_64-asm_64.S | 9 +
> arch/x86/crypto/cast6-avx-x86_64-asm_64.S | 13 +
> arch/x86/crypto/crc32c-pcl-intel-asm_64.S | 8 +-
> arch/x86/crypto/ghash-clmulni-intel_asm.S | 5 +
> arch/x86/crypto/serpent-avx-x86_64-asm_64.S | 13 +
> arch/x86/crypto/serpent-avx2-asm_64.S | 13 +
> arch/x86/crypto/sha-mb/sha1_mb_mgr_flush_avx2.S | 35 +-
> arch/x86/crypto/sha-mb/sha1_mb_mgr_submit_avx2.S | 36 +-
> arch/x86/crypto/twofish-avx-x86_64-asm_64.S | 13 +
> arch/x86/entry/Makefile | 4 +
> arch/x86/entry/thunk_64.S | 4 +
> arch/x86/entry/vdso/Makefile | 5 +-
> arch/x86/include/asm/paravirt.h | 9 +-
> arch/x86/include/asm/paravirt_types.h | 18 +-
> arch/x86/include/asm/qspinlock_paravirt.h | 4 +
> arch/x86/include/asm/uaccess.h | 5 +-
> arch/x86/include/asm/xen/hypercall.h | 5 +-
> arch/x86/kernel/Makefile | 5 +
> arch/x86/kernel/acpi/wakeup_64.S | 3 +
> arch/x86/kernel/cpu/amd.c | 5 +-
> arch/x86/kernel/kprobes/core.c | 59 +-
> arch/x86/kernel/vmlinux.lds.S | 5 +-
> arch/x86/kvm/emulate.c | 33 +-
> arch/x86/lib/rwsem.S | 11 +-
> arch/x86/net/bpf_jit.S | 48 +-
> arch/x86/platform/efi/Makefile | 2 +
> arch/x86/platform/efi/efi_stub_64.S | 3 +
> arch/x86/power/hibernate_asm_64.S | 7 +
> arch/x86/purgatory/Makefile | 2 +
> arch/x86/realmode/Makefile | 4 +-
> arch/x86/realmode/rm/Makefile | 3 +-
> arch/x86/xen/enlighten.c | 3 +-
> arch/x86/xen/xen-asm.S | 10 +-
> arch/x86/xen/xen-asm_64.S | 1 +
> drivers/firmware/efi/libstub/Makefile | 1 +
> drivers/watchdog/hpwdt.c | 8 +-
> include/linux/stacktool.h | 23 +
> kernel/bpf/core.c | 2 +
> kernel/sched/core.c | 2 +
> lib/Kconfig.debug | 12 +
> scripts/Makefile.build | 38 +-
> scripts/mod/Makefile | 2 +
> tools/Makefile | 14 +-
> tools/stacktool/.gitignore | 2 +
> tools/stacktool/Build | 13 +
> tools/stacktool/Documentation/stack-validation.txt | 333 +++++++
> tools/stacktool/Makefile | 60 ++
> tools/stacktool/arch.h | 44 +
> tools/stacktool/arch/x86/Build | 12 +
> tools/stacktool/arch/x86/decode.c | 172 ++++
> .../stacktool/arch/x86/insn/gen-insn-attr-x86.awk | 387 ++++++++
> tools/stacktool/arch/x86/insn/inat.c | 97 ++
> tools/stacktool/arch/x86/insn/inat.h | 221 +++++
> tools/stacktool/arch/x86/insn/inat_types.h | 29 +
> tools/stacktool/arch/x86/insn/insn.c | 594 ++++++++++++
> tools/stacktool/arch/x86/insn/insn.h | 201 +++++
> tools/stacktool/arch/x86/insn/x86-opcode-map.txt | 984 ++++++++++++++++++++
> tools/stacktool/builtin-check.c | 991 +++++++++++++++++++++
> tools/stacktool/builtin.h | 22 +
> tools/stacktool/elf.c | 403 +++++++++
> tools/stacktool/elf.h | 79 ++
> tools/stacktool/special.c | 193 ++++
> tools/stacktool/special.h | 42 +
> tools/stacktool/stacktool.c | 134 +++
> tools/stacktool/warn.h | 60 ++
> 74 files changed, 5516 insertions(+), 189 deletions(-)
> create mode 100644 include/linux/stacktool.h
> create mode 100644 tools/stacktool/.gitignore
> create mode 100644 tools/stacktool/Build
> create mode 100644 tools/stacktool/Documentation/stack-validation.txt
> create mode 100644 tools/stacktool/Makefile
> create mode 100644 tools/stacktool/arch.h
> create mode 100644 tools/stacktool/arch/x86/Build
> create mode 100644 tools/stacktool/arch/x86/decode.c
> create mode 100644 tools/stacktool/arch/x86/insn/gen-insn-attr-x86.awk
> create mode 100644 tools/stacktool/arch/x86/insn/inat.c
> create mode 100644 tools/stacktool/arch/x86/insn/inat.h
> create mode 100644 tools/stacktool/arch/x86/insn/inat_types.h
> create mode 100644 tools/stacktool/arch/x86/insn/insn.c
> create mode 100644 tools/stacktool/arch/x86/insn/insn.h
> create mode 100644 tools/stacktool/arch/x86/insn/x86-opcode-map.txt
> create mode 100644 tools/stacktool/builtin-check.c
> create mode 100644 tools/stacktool/builtin.h
> create mode 100644 tools/stacktool/elf.c
> create mode 100644 tools/stacktool/elf.h
> create mode 100644 tools/stacktool/special.c
> create mode 100644 tools/stacktool/special.h
> create mode 100644 tools/stacktool/stacktool.c
> create mode 100644 tools/stacktool/warn.h
>
> --
> 2.4.3
>
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-01-21 22:49 [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf
` (3 preceding siblings ...)
2016-01-22 17:43 ` [PATCH 00/33] Compile-time stack metadata validation Chris J Arges
@ 2016-02-12 10:36 ` Jiri Slaby
2016-02-12 10:41 ` Jiri Slaby
2016-02-12 14:45 ` Josh Poimboeuf
2016-02-23 8:14 ` Ingo Molnar
5 siblings, 2 replies; 38+ messages in thread
From: Jiri Slaby @ 2016-02-12 10:36 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
Cc: linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Arnaldo Carvalho de Melo, David Vrabel,
Borislav Petkov, Konrad Rzeszutek Wilk, Boris Ostrovsky,
Jeremy Fitzhardinge, Chris Wright, Alok Kataria, Rusty Russell,
Herbert Xu
On 01/21/2016, 11:49 PM, Josh Poimboeuf wrote:
> This is v16 of the compile-time stack metadata validation patch set,
> along with proposed fixes for most of the warnings it found. It's based
> on the tip/master branch.
Hi,
with this config:
https://github.com/openSUSE/kernel-source/blob/master/config/x86_64/vanilla
I am seeing a lot of functions in C which do not have frame pointer setup/cleanup:
stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x1: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/lo.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/nidstrings.o: cfs_print_nidlist()+0x220: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/peer.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/fid/fid_request.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/fld/lproc_fld.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_lock.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_mem.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x4: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x24: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1a: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1b: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x19: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/llite/../lclient/lcommon_misc.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/llite_mmap.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/lproc_llite.o: checksum_pages_store()+0x19e: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/rw.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x24: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/vvp_page.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/xattr_cache.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x1f: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/lmv/lmv_intent.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/lmv/lmv_obd.o: __lmv_fid_alloc()+0x185: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/lov/lov_io.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/lov/lovsub_dev.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/mdc/mdc_lib.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/mdc/mdc_locks.o: .text.unlikely: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/debug.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/genops.o: class_name2dev()+0xc7: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/obdclass/lustre_handles.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x4: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x1: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/osc/osc_dev.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/osc/osc_page.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/connection.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x6: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/ptlrpc/llog_net.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_extent.o: ldlm_extent_shift_kms()+0x93: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_bl_ast_lock()+0x156: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_cp_ast_lock()+0xda: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x6: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_bulk.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_config.o: .text: unexpected end of section
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup
For example do_profile_hits.isra.5:
0000000000003360 <hpsa_scsi_do_simple_cmd.constprop.106>:
3360: e8 00 00 00 00 callq 3365 <hpsa_scsi_do_simple_cmd.constprop.106+0x5>
3361: R_X86_64_PC32 __fentry__-0x4
3365: 65 ff 05 00 00 00 00 incl %gs:0x0(%rip) # 336c <hpsa_scsi_do_simple_cmd.constprop.106+0xc>
3368: R_X86_64_PC32 __preempt_count-0x4
336c: 65 8b 0d 00 00 00 00 mov %gs:0x0(%rip),%ecx # 3373 <hpsa_scsi_do_simple_cmd.constprop.106+0x13>
336f: R_X86_64_PC32 cpu_number-0x4
3373: 48 63 c9 movslq %ecx,%rcx
3376: 48 8b 87 b8 4b 00 00 mov 0x4bb8(%rdi),%rax
337d: 48 8b 0c cd 00 00 00 mov 0x0(,%rcx,8),%rcx
3384: 00
3381: R_X86_64_32S __per_cpu_offset
3385: 8b 04 01 mov (%rcx,%rax,1),%eax
3388: 65 ff 0d 00 00 00 00 decl %gs:0x0(%rip) # 338f <hpsa_scsi_do_simple_cmd.constprop.106+0x2f>
338b: R_X86_64_PC32 __preempt_count-0x4
338f: 74 48 je 33d9 <hpsa_scsi_do_simple_cmd.constprop.106+0x79>
3391: 85 c0 test %eax,%eax
3393: 75 4d jne 33e2 <hpsa_scsi_do_simple_cmd.constprop.106+0x82>
3395: 55 push %rbp
3396: 48 89 e5 mov %rsp,%rbp
3399: 53 push %rbx
339a: 48 8d 5d d8 lea -0x28(%rbp),%rbx
339e: 48 83 ec 20 sub $0x20,%rsp
33a2: c7 45 d8 00 00 00 00 movl $0x0,-0x28(%rbp)
33a9: c7 45 e0 00 00 00 00 movl $0x0,-0x20(%rbp)
33b0: 48 8d 43 10 lea 0x10(%rbx),%rax
33b4: 48 89 9e 54 02 00 00 mov %rbx,0x254(%rsi)
33bb: 48 89 45 e8 mov %rax,-0x18(%rbp)
33bf: 48 89 45 f0 mov %rax,-0x10(%rbp)
33c3: e8 f8 ce ff ff callq 2c0 <__enqueue_cmd_and_start_io>
33c8: 48 89 df mov %rbx,%rdi
33cb: e8 00 00 00 00 callq 33d0 <hpsa_scsi_do_simple_cmd.constprop.106+0x70>
33cc: R_X86_64_PC32 wait_for_completion_io-0x4
33d0: 48 83 c4 20 add $0x20,%rsp
33d4: 31 c0 xor %eax,%eax
33d6: 5b pop %rbx
33d7: 5d pop %rbp
33d8: c3 retq
33d9: e8 00 00 00 00 callq 33de <hpsa_scsi_do_simple_cmd.constprop.106+0x7e>
33da: R_X86_64_PC32 ___preempt_schedule-0x4
33de: 85 c0 test %eax,%eax
33e0: 74 b3 je 3395 <hpsa_scsi_do_simple_cmd.constprop.106+0x35>
33e2: 48 8b 86 38 02 00 00 mov 0x238(%rsi),%rax
33e9: ba ff ff ff ff mov $0xffffffff,%edx
33ee: 66 89 50 02 mov %dx,0x2(%rax)
33f2: 31 c0 xor %eax,%eax
33f4: c3 retq
33f5: 90 nop
33f6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
33fd: 00 00 00
It there some compilation flag missing? -f flags when compiling that file are:
-falign-jumps=1
-falign-loops=1
-fconserve-stack
-fno-asynchronous-unwind-tables
-fno-common
-fno-delete-null-pointer-checks
-fno-inline-functions-called-once
-fno-omit-frame-pointer
-fno-optimize-sibling-calls
-fno-strict-aliasing
-fno-strict-overflow
-fno-var-tracking-assignments
-fstack-protector
-funit-at-a-time
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-02-12 10:36 ` Jiri Slaby
@ 2016-02-12 10:41 ` Jiri Slaby
2016-02-12 14:45 ` Josh Poimboeuf
1 sibling, 0 replies; 38+ messages in thread
From: Jiri Slaby @ 2016-02-12 10:41 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
Cc: linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Arnaldo Carvalho de Melo, David Vrabel,
Borislav Petkov, Konrad Rzeszutek Wilk, Boris Ostrovsky,
Jeremy Fitzhardinge, Chris Wright, Alok Kataria, Rusty Russell,
Herbert Xu
On 02/12/2016, 11:36 AM, Jiri Slaby wrote:
> It there some compilation flag missing? -f flags when compiling that file are:
> -falign-jumps=1
> -falign-loops=1
> -fconserve-stack
> -fno-asynchronous-unwind-tables
> -fno-common
> -fno-delete-null-pointer-checks
> -fno-inline-functions-called-once
> -fno-omit-frame-pointer
> -fno-optimize-sibling-calls
> -fno-strict-aliasing
> -fno-strict-overflow
> -fno-var-tracking-assignments
> -fstack-protector
> -funit-at-a-time
Happens with:
gcc (SUSE Linux) 5.3.1 20151207 [gcc-5-branch revision 231355]
gcc-6 (SUSE Linux) 6.0.0 20160202 (experimental) [trunk revision 233076]
> thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-02-12 10:36 ` Jiri Slaby
2016-02-12 10:41 ` Jiri Slaby
@ 2016-02-12 14:45 ` Josh Poimboeuf
2016-02-12 17:10 ` Peter Zijlstra
1 sibling, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-02-12 14:45 UTC (permalink / raw)
To: Jiri Slaby
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Arnaldo Carvalho de Melo, David Vrabel, Borislav Petkov,
Konrad Rzeszutek Wilk, Boris Ostrovsky, Jeremy Fitzhardinge,
Chris Wright
On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> On 01/21/2016, 11:49 PM, Josh Poimboeuf wrote:
> > This is v16 of the compile-time stack metadata validation patch set,
> > along with proposed fixes for most of the warnings it found. It's based
> > on the tip/master branch.
>
> Hi,
>
> with this config:
> https://github.com/openSUSE/kernel-source/blob/master/config/x86_64/vanilla
>
> I am seeing a lot of functions in C which do not have frame pointer setup/cleanup:
Hi Jiri,
Thanks for testing.
> stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
This seems like a real frame pointer bug caused by the following line in
arch/x86/include/asm/preempt.h:
# define __preempt_schedule() asm ("call ___preempt_schedule")
The asm statement doesn't have the stack pointer as an output operand,
so gcc doesn't skips the frame pointer setup before calling.
However, I suspect the "bug" is intentional for optimization purposes.
> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x1: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/lo.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/nidstrings.o: cfs_print_nidlist()+0x220: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/peer.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/fid/fid_request.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/fld/lproc_fld.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_lock.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_mem.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x4: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x24: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1a: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1b: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x19: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/lcommon_misc.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/llite_mmap.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/lproc_llite.o: checksum_pages_store()+0x19e: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/rw.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x24: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/vvp_page.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/xattr_cache.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x1f: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/lmv/lmv_intent.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/lmv/lmv_obd.o: __lmv_fid_alloc()+0x185: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/lov/lov_io.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/lov/lovsub_dev.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/mdc/mdc_lib.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/mdc/mdc_locks.o: .text.unlikely: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/debug.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/genops.o: class_name2dev()+0xc7: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/obdclass/lustre_handles.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x4: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x1: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/osc/osc_dev.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/osc/osc_page.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/connection.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x6: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/ptlrpc/llog_net.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_extent.o: ldlm_extent_shift_kms()+0x93: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_bl_ast_lock()+0x156: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_cp_ast_lock()+0xda: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x6: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_bulk.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_config.o: .text: unexpected end of section
These staging driver issues are caused by stacktool getting confused by
gcc optimizations related to noreturn functions. I have it on the TODO
list to make the noreturn function detection more intelligent.
> stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
> stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
> stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
> stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
> stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
> stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
> stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
> stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
> stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup
These are all the same "call ___preempt_schedule" issue from above.
I'll need to look into it to figure out if it's a real bug or if it's a
"feature" we should ignore.
--
Josh
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-02-12 14:45 ` Josh Poimboeuf
@ 2016-02-12 17:10 ` Peter Zijlstra
2016-02-12 18:32 ` Josh Poimboeuf
0 siblings, 1 reply; 38+ messages in thread
From: Peter Zijlstra @ 2016-02-12 17:10 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Jiri Slaby, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-kernel, live-patching, Michal Marek, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Arnaldo Carvalho de Melo, David Vrabel, Borislav Petkov,
Konrad Rzeszutek Wilk, Boris Ostrovsky, Jeremy Fitzhardinge,
Chris Wright
On Fri, Feb 12, 2016 at 08:45:43AM -0600, Josh Poimboeuf wrote:
> On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
>
> This seems like a real frame pointer bug caused by the following line in
> arch/x86/include/asm/preempt.h:
>
> # define __preempt_schedule() asm ("call ___preempt_schedule")
The purpose there is that:
preempt_enable();
turns into:
decl __percpu_prefix:__preempt_count
jnz 1f:
call ___preempt_schedule
1:
See arch/x86/include/asm/preempt.h:__preempt_count_dec_and_test()
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-02-12 17:10 ` Peter Zijlstra
@ 2016-02-12 18:32 ` Josh Poimboeuf
[not found] ` <20160212183206.GB29004-8wJ5/zUtDR0XGNroddHbYwC/G2K4zDHf@public.gmane.org>
0 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-02-12 18:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jiri Slaby, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-kernel, live-patching, Michal Marek, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Arnaldo Carvalho de Melo, David Vrabel, Borislav Petkov,
Konrad Rzeszutek Wilk, Boris Ostrovsky, Jeremy Fitzhardinge,
Chris Wright
On Fri, Feb 12, 2016 at 06:10:37PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 12, 2016 at 08:45:43AM -0600, Josh Poimboeuf wrote:
> > On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> >
> > This seems like a real frame pointer bug caused by the following line in
> > arch/x86/include/asm/preempt.h:
> >
> > # define __preempt_schedule() asm ("call ___preempt_schedule")
>
> The purpose there is that:
>
> preempt_enable();
>
> turns into:
>
> decl __percpu_prefix:__preempt_count
> jnz 1f:
> call ___preempt_schedule
> 1:
>
> See arch/x86/include/asm/preempt.h:__preempt_count_dec_and_test()
Sorry, I'm kind of confused. Do you mean that's what preempt_enable()
would turn into *without* the above define?
What I actually see in the listing is:
decl __percpu_prefix:__preempt_count
je 1f:
....
1:
call ___preempt_schedule
So it puts the "call ___preempt_schedule" in the slow path.
I also don't see how that would be related to the use of the asm
statement in the __preempt_schedule() macro. Doesn't the use of
unlikely() in preempt_enable() put the call in the slow path?
#define preempt_enable() \
do { \
barrier(); \
if (unlikely(preempt_count_dec_and_test())) \
preempt_schedule(); \
} while (0)
Also, why is the thunk needed? Any reason why preempt_enable() can't be
called directly from C?
--
Josh
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-01-21 22:49 [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf
` (4 preceding siblings ...)
2016-02-12 10:36 ` Jiri Slaby
@ 2016-02-23 8:14 ` Ingo Molnar
[not found] ` <20160223081406.GA606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-02-23 15:01 ` Josh Poimboeuf
5 siblings, 2 replies; 38+ messages in thread
From: Ingo Molnar @ 2016-02-23 8:14 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, David Vrabel,
Borislav Petkov, Konrad Rzeszutek Wilk, Boris Ostrovsky,
Jeremy Fitzhardinge
So I tried out this latest stacktool series and it looks mostly good for an
upstream merge.
To help this effort move forward I've applied the preparatory/fix patches that are
part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've
propagated all the acks that the latest submission got into the changelogs.)
I have 5 (easy to address) observations that need to be addressed before we can
move on with the rest of the merge:
1)
Due to recent changes to x86 exception handling, I get a lot of bogus warnings
about exception table sizes:
stacktool: arch/x86/kernel/cpu/mcheck/mce.o: __ex_table size not a multiple of 24
stacktool: arch/x86/kernel/cpu/mtrr/generic.o: __ex_table size not a multiple of 24
stacktool: arch/x86/kernel/cpu/mtrr/cleanup.o: __ex_table size not a multiple of 24
this is due to:
548acf19234d x86/mm: Expand the exception table logic to allow new handling options
2)
The fact that 'stacktool' already checks about assembly details like __ex_table[]
shows that my review feedback early iterations of this series, that the
'stacktool' name is too specific, was correct.
We really need to rename it before it gets upstream and the situation gets worse.
__ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
Another suitable name would be 'asmtool' or 'objtool'. For example the following
would naturally express what it does:
objtool check kernel/built-in.o
the name expresses that the tool checks object files, independently of the
existing toolchain. Its primary purpose right now is the checking of stack layout
details, but the tool is not limited to that at all.
3)
There's quite a bit of overhead when running the tool on larger object files, most
prominently in cmd_check():
62.06% stacktool stacktool [.] cmd_check
6.72% stacktool stacktool [.] find_rela_by_dest_range
I added -g to the CFLAGS, which made it visible to annotated output of perf top:
0.00 : 40430d: lea 0x4(%rdx,%rax,1),%r13
: find_instruction():
: struct section *sec,
: unsigned long offset)
: {
: struct instruction *insn;
:
: list_for_each_entry(insn, &file->insns, list)
0.03 : 404312: mov 0x38(%rsp),%rax
0.00 : 404317: cmp %rbp,%rax
0.00 : 40431a: jne 404334 <cmd_check+0x5b4>
0.00 : 40431c: jmpq 4045ba <cmd_check+0x83a>
0.00 : 404321: nopl 0x0(%rax)
6.14 : 404328: mov (%rax),%rax
0.00 : 40432b: cmp %rbp,%rax
2.02 : 40432e: je 4045ba <cmd_check+0x83a>
: if (insn->sec == sec && insn->offset == offset)
0.55 : 404334: cmp %r12,0x10(%rax)
87.91 : 404338: jne 404328 <cmd_check+0x5a8>
0.00 : 40433a: cmp %r13,0x18(%rax)
3.36 : 40433e: jne 404328 <cmd_check+0x5a8>
: get_jump_destinations():
: * later in validate_functions().
: */
: continue;
: }
:
: insn->jump_dest = find_instruction(file, dest_sec, dest_off);
0.00 : 404340: mov %rax,0x48(%rbx)
0.00 : 404344: jmpq 4042b0 <cmd_check+0x530>
0.00 : 404349: nopl 0x0(%rax)
: fprintf():
:
: # ifdef __va_arg_pack
: __fortify_function int
: fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
: {
: return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
that looks like a linear list search? That would suck with thousands of entries.
(If this is non-trivial to improve then we can delay this optimization to a later
patch.)
4)
I think the various 'STACKTOOL' flags in the kernel source are a bit of a misnomer
- it's not the tool we want to name but the actual property of the code.
So instead of:
STACKTOOL_IGNORE_FUNC(__bpf_prog_run);
we should do something like:
STACK_FRAME_NON_STANDARD(__bpf_prog_run);
see how much more expressive it is? It becomes a function attribute independent of
the tooling that makes use of it.
Similarly, for the highest level 'don't check these directories' makefile flags,
I'd suggest, instead of using this rather opaque, tool dependent naming:
STACKTOOL := n
something more specific, like:
OBJECT_FILES_NON_STANDARD := y
which makes it clearer what's going on: these are special object files that are
not the typical kernel object files.
Stacktool (or objtool) would be one consumer of this annotation.
I think Boris made similar observations in past reviews.
5)
Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that
we do exception table checks as well - and it does not express all the other
things we may check in object files in the future.
Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text
would list all the things the tool is able to checks for at the moment.
-------------------
Please send followup iterations of the series against the tip:x86/debug tree (or
tip:master), to keep the size of the series down.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 38+ messages in thread[parent not found: <20160223081406.GA606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 00/33] Compile-time stack metadata validation
[not found] ` <20160223081406.GA606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2016-02-23 14:27 ` Arnaldo Carvalho de Melo
2016-02-23 15:07 ` Josh Poimboeuf
0 siblings, 1 reply; 38+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-02-23 14:27 UTC (permalink / raw)
To: Ingo Molnar
Cc: Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
x86-DgEjT+Ai2ygdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
live-patching-u79uwXL29TY76Z2rM5mHXA, Michal Marek,
Peter Zijlstra, Andy Lutomirski, Borislav Petkov, Linus Torvalds,
Andi Kleen, Pedro Alves, Namhyung Kim, Bernd Petrovitsch,
Chris J Arges, Andrew Morton, Jiri Slaby, David Vrabel,
Borislav Petkov, Konrad Rzeszutek Wilk, Boris Ostrovsky,
Jeremy Fitzhardinge, C
Em Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar escreveu:
> The fact that 'stacktool' already checks about assembly details like __ex_table[]
> shows that my review feedback early iterations of this series, that the
> 'stacktool' name is too specific, was correct.
>
> We really need to rename it before it gets upstream and the situation gets worse.
> __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
>
> Another suitable name would be 'asmtool' or 'objtool'. For example the following
> would naturally express what it does:
>
> objtool check kernel/built-in.o
>
> the name expresses that the tool checks object files, independently of the
> existing toolchain. Its primary purpose right now is the checking of stack layout
> details, but the tool is not limited to that at all.
Removing 'tool' from the tool name would be nice too :-) Making it
easily googlable would be good too, lotsa people complain about 'perf'
being too vague, see for a quick laugher:
http://www.brendangregg.com/perf.html
``Searching for just "perf" finds sites on the police, petroleum, weed
control, and a T-shirt. This is not an official perf page, for either
perf_events or the T-shirt.''
The T-shirt: http://www.brendangregg.com/perf_events/omg-so-perf.jpg
Maybe we should ask Linus to come with some other nice, short,
searchable, funny name like 'git'?
'chob' as in 'check object'?
- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-02-23 14:27 ` Arnaldo Carvalho de Melo
@ 2016-02-23 15:07 ` Josh Poimboeuf
2016-02-23 15:28 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-02-23 15:07 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Ingo Molnar, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Jiri Slaby, David Vrabel, Borislav Petkov,
Konrad Rzeszutek Wilk, Boris Ostrovsky, Jeremy Fitzhardinge,
Chris
On Tue, Feb 23, 2016 at 11:27:17AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar escreveu:
> > The fact that 'stacktool' already checks about assembly details like __ex_table[]
> > shows that my review feedback early iterations of this series, that the
> > 'stacktool' name is too specific, was correct.
> >
> > We really need to rename it before it gets upstream and the situation gets worse.
> > __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
> >
> > Another suitable name would be 'asmtool' or 'objtool'. For example the following
> > would naturally express what it does:
> >
> > objtool check kernel/built-in.o
> >
> > the name expresses that the tool checks object files, independently of the
> > existing toolchain. Its primary purpose right now is the checking of stack layout
> > details, but the tool is not limited to that at all.
>
> Removing 'tool' from the tool name would be nice too :-) Making it
> easily googlable would be good too, lotsa people complain about 'perf'
> being too vague, see for a quick laugher:
>
> http://www.brendangregg.com/perf.html
>
> ``Searching for just "perf" finds sites on the police, petroleum, weed
> control, and a T-shirt. This is not an official perf page, for either
> perf_events or the T-shirt.''
>
> The T-shirt: http://www.brendangregg.com/perf_events/omg-so-perf.jpg
Yeah, 'tool' in the name is kind of silly, but the above type of
situation is why I prefer 'objtool' over 'obj'.
Though I have to admit I like the idea of making a t-shirt for it ;-)
> Maybe we should ask Linus to come with some other nice, short,
> searchable, funny name like 'git'?
>
> 'chob' as in 'check object'?
I think 'objtool' is searchable enough. And I also like the fact that
its name at least gives you an idea of what it does (and eventually it
will do more than just "checking").
--
Josh
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-02-23 15:07 ` Josh Poimboeuf
@ 2016-02-23 15:28 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 38+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-02-23 15:28 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Ingo Molnar, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-kernel, live-patching, Michal Marek, Peter Zijlstra,
Andy Lutomirski, Borislav Petkov, Linus Torvalds, Andi Kleen,
Pedro Alves, Namhyung Kim, Bernd Petrovitsch, Chris J Arges,
Andrew Morton, Jiri Slaby, David Vrabel, Borislav Petkov,
Konrad Rzeszutek Wilk, Boris Ostrovsky, Jeremy Fitzhardinge,
Chris
Em Tue, Feb 23, 2016 at 09:07:55AM -0600, Josh Poimboeuf escreveu:
> I think 'objtool' is searchable enough. And I also like the fact that
Yeah, agreed, there is even documentation available for it already:
http://docs.bvstools.com/home/objtool
> its name at least gives you an idea of what it does (and eventually it
> will do more than just "checking").
:-)
- ARnaldo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-02-23 8:14 ` Ingo Molnar
[not found] ` <20160223081406.GA606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2016-02-23 15:01 ` Josh Poimboeuf
2016-02-24 7:40 ` Ingo Molnar
1 sibling, 1 reply; 38+ messages in thread
From: Josh Poimboeuf @ 2016-02-23 15:01 UTC (permalink / raw)
To: Ingo Molnar
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, David Vrabel,
Borislav Petkov, Konrad Rzeszutek Wilk, Boris Ostrovsky,
Jeremy Fitzhardinge
Hi Ingo,
On Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar wrote:
> So I tried out this latest stacktool series and it looks mostly good for an
> upstream merge.
>
> To help this effort move forward I've applied the preparatory/fix patches that are
> part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've
> propagated all the acks that the latest submission got into the changelogs.)
Thanks very much for your review and for applying the fixes!
A few issues relating to the merge:
- The tip:x86/debug branch fails to build because it depends on
ec5186557abb ("x86/asm: Add C versions of frame pointer macros") which
is in tip:x86/asm.
- As Pavel mentioned, the tip-bot seems to be spitting out garbage
emails from:
=?UTF-8?B?dGlwLWJvdCBmb3IgSm9zaCBQb2ltYm9ldWYgPHRpcGJvdEB6eXRvci5jb20+?=@zytor.com.
> I have 5 (easy to address) observations that need to be addressed before we can
> move on with the rest of the merge:
>
> 1)
>
> Due to recent changes to x86 exception handling, I get a lot of bogus warnings
> about exception table sizes:
>
> stacktool: arch/x86/kernel/cpu/mcheck/mce.o: __ex_table size not a multiple of 24
> stacktool: arch/x86/kernel/cpu/mtrr/generic.o: __ex_table size not a multiple of 24
> stacktool: arch/x86/kernel/cpu/mtrr/cleanup.o: __ex_table size not a multiple of 24
>
> this is due to:
>
> 548acf19234d x86/mm: Expand the exception table logic to allow new handling options
Ok, I'll fix it up.
> 2)
>
> The fact that 'stacktool' already checks about assembly details like __ex_table[]
> shows that my review feedback early iterations of this series, that the
> 'stacktool' name is too specific, was correct.
>
> We really need to rename it before it gets upstream and the situation gets worse.
> __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
>
> Another suitable name would be 'asmtool' or 'objtool'. For example the following
> would naturally express what it does:
>
> objtool check kernel/built-in.o
>
> the name expresses that the tool checks object files, independently of the
> existing toolchain. Its primary purpose right now is the checking of stack layout
> details, but the tool is not limited to that at all.
Fair enough. I'll rename it to objtool if there are no objections.
> 3)
>
> There's quite a bit of overhead when running the tool on larger object files, most
> prominently in cmd_check():
>
> 62.06% stacktool stacktool [.] cmd_check
> 6.72% stacktool stacktool [.] find_rela_by_dest_range
>
> I added -g to the CFLAGS, which made it visible to annotated output of perf top:
>
> 0.00 : 40430d: lea 0x4(%rdx,%rax,1),%r13
> : find_instruction():
> : struct section *sec,
> : unsigned long offset)
> : {
> : struct instruction *insn;
> :
> : list_for_each_entry(insn, &file->insns, list)
> 0.03 : 404312: mov 0x38(%rsp),%rax
> 0.00 : 404317: cmp %rbp,%rax
> 0.00 : 40431a: jne 404334 <cmd_check+0x5b4>
> 0.00 : 40431c: jmpq 4045ba <cmd_check+0x83a>
> 0.00 : 404321: nopl 0x0(%rax)
> 6.14 : 404328: mov (%rax),%rax
> 0.00 : 40432b: cmp %rbp,%rax
> 2.02 : 40432e: je 4045ba <cmd_check+0x83a>
> : if (insn->sec == sec && insn->offset == offset)
> 0.55 : 404334: cmp %r12,0x10(%rax)
> 87.91 : 404338: jne 404328 <cmd_check+0x5a8>
> 0.00 : 40433a: cmp %r13,0x18(%rax)
> 3.36 : 40433e: jne 404328 <cmd_check+0x5a8>
> : get_jump_destinations():
> : * later in validate_functions().
> : */
> : continue;
> : }
> :
> : insn->jump_dest = find_instruction(file, dest_sec, dest_off);
> 0.00 : 404340: mov %rax,0x48(%rbx)
> 0.00 : 404344: jmpq 4042b0 <cmd_check+0x530>
> 0.00 : 404349: nopl 0x0(%rax)
> : fprintf():
> :
> : # ifdef __va_arg_pack
> : __fortify_function int
> : fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
> : {
> : return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
>
> that looks like a linear list search? That would suck with thousands of entries.
>
> (If this is non-trivial to improve then we can delay this optimization to a later
> patch.)
Yeah, I agree that the linear list search isn't good. I still need to
think about the data structures a bit, so if it's ok with you, I'll fix
it with a later patch.
> 4)
>
> I think the various 'STACKTOOL' flags in the kernel source are a bit of a misnomer
> - it's not the tool we want to name but the actual property of the code.
>
> So instead of:
>
> STACKTOOL_IGNORE_FUNC(__bpf_prog_run);
>
> we should do something like:
>
> STACK_FRAME_NON_STANDARD(__bpf_prog_run);
>
> see how much more expressive it is? It becomes a function attribute independent of
> the tooling that makes use of it.
Ok, STACK_FRAME_NON_STANDARD sounds fine to me.
> Similarly, for the highest level 'don't check these directories' makefile flags,
> I'd suggest, instead of using this rather opaque, tool dependent naming:
>
> STACKTOOL := n
>
> something more specific, like:
>
> OBJECT_FILES_NON_STANDARD := y
>
> which makes it clearer what's going on: these are special object files that are
> not the typical kernel object files.
>
> Stacktool (or objtool) would be one consumer of this annotation.
>
> I think Boris made similar observations in past reviews.
Sounds reasonable. With this approach we could probably eventually get
rid of KASAN_SANITIZE.
I'll change it to "OBJECT_FILES_NON_STANDARD := y" if there are no
objections.
Note there are also per-object ignores like:
STACKTOOL_head_$(BITS).o := n
I can similarly change that to something like:
OBJECT_FILES_NON_STANDARD_head_$(BITS).o := n
> 5)
>
> Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that
> we do exception table checks as well - and it does not express all the other
> things we may check in object files in the future.
>
> Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text
> would list all the things the tool is able to checks for at the moment.
Hm, I'm not really sure about this. Yes, the tool could potentially do
other types of checks, but is it necessary to lump them all together
into a single config option? It does have subcommands after all ;-)
The exception table check reported above is very basic and doesn't serve
any useful purpose other than supporting the goal of validating the
stack.
However, I don't feel strong enough about it to hold up the merge any
longer, so I'll plan to make the change unless I hear back from you.
> Please send followup iterations of the series against the tip:x86/debug tree (or
> tip:master), to keep the size of the series down.
Will do, thanks!
--
Josh
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 00/33] Compile-time stack metadata validation
2016-02-23 15:01 ` Josh Poimboeuf
@ 2016-02-24 7:40 ` Ingo Molnar
[not found] ` <20160224074054.GA13199-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 38+ messages in thread
From: Ingo Molnar @ 2016-02-24 7:40 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
live-patching, Michal Marek, Peter Zijlstra, Andy Lutomirski,
Borislav Petkov, Linus Torvalds, Andi Kleen, Pedro Alves,
Namhyung Kim, Bernd Petrovitsch, Chris J Arges, Andrew Morton,
Jiri Slaby, Arnaldo Carvalho de Melo, David Vrabel,
Borislav Petkov, Konrad Rzeszutek Wilk, Boris Ostrovsky,
Jeremy Fitzhardinge
* Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> Hi Ingo,
>
> On Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar wrote:
> > So I tried out this latest stacktool series and it looks mostly good for an
> > upstream merge.
> >
> > To help this effort move forward I've applied the preparatory/fix patches that are
> > part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've
> > propagated all the acks that the latest submission got into the changelogs.)
>
> Thanks very much for your review and for applying the fixes!
>
> A few issues relating to the merge:
>
> - The tip:x86/debug branch fails to build because it depends on
> ec5186557abb ("x86/asm: Add C versions of frame pointer macros") which
> is in tip:x86/asm.
Indeed...
> - As Pavel mentioned, the tip-bot seems to be spitting out garbage
> emails from:
> =?UTF-8?B?dGlwLWJvdCBmb3IgSm9zaCBQb2ltYm9ldWYgPHRpcGJvdEB6eXRvci5jb20+?=@zytor.com.
Yeah, hpa fixed that meanwhile.
Due to the above bad base I rebased the tree (to a x86/asm base), so there will be
a new round of (hopefully readable) tip-bot notifications. I'll push it out after
a bit of testing.
> > 5)
> >
> > Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that
> > we do exception table checks as well - and it does not express all the other
> > things we may check in object files in the future.
> >
> > Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text
> > would list all the things the tool is able to checks for at the moment.
>
> Hm, I'm not really sure about this. Yes, the tool could potentially do
> other types of checks, but is it necessary to lump them all together
> into a single config option? It does have subcommands after all ;-)
lol ;-)
Ok, I'm fine with CONFIG_STACK_VALIDATION=y as well.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 38+ messages in thread
