Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

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 
> > > > > > > Cc: Alexei Starovoitov 
> > > > > > > Cc: net...@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;\
> > > > > > >   pushSKBDATA;\
> > > > > > >  /* rsi already has offset */ \
> > > > > > >   mov $LEN,%ecx;  /* len */   \
> > > > > > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx;
> > > > > > > \
> > > > > > >   callskb_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 
> 
> 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.

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

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 
> > > > > > Cc: Alexei Starovoitov 
> > > > > > Cc: net...@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;\
> > > > > > pushSKBDATA;\
> > > > > >  /* rsi already has offset */   \
> > > > > > mov $LEN,%ecx;  /* len */   \
> > > > > > -   lea - MAX_BPF_STACK + 32(%rbp),%rdx;
> > > > > > \
> > > > > > callskb_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 

Thanks!

Can I assume your ack also applies to the previous patch which adds the
ELF annotations ("x86/asm/bpf: Annotate callable functions")?

-- 
Josh

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

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 
> > > > > Cc: Alexei Starovoitov 
> > > > > Cc: net...@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;\
> > > > >   pushSKBDATA;\
> > > > >  /* rsi already has offset */ \
> > > > >   mov $LEN,%ecx;  /* len */   \
> > > > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx;
> > > > > \
> > > > >   callskb_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 

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

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 
> > > > Cc: Alexei Starovoitov 
> > > > Cc: net...@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 
> > > > +#include 
> > > >  
> > > >  /*
> > > >   * 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;\
> > > > pushSKBDATA;\
> > > >  /* rsi already has offset */   \
> > > > mov $LEN,%ecx;  /* len */   \
> > > > -   lea - MAX_BPF_STACK + 32(%rbp),%rdx;
> > > > \
> > > > callskb_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 

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

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 
> > > > > Cc: Alexei Starovoitov 
> > > > > Cc: net...@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;\
> > > > >   pushSKBDATA;\
> > > > >  /* rsi already has offset */ \
> > > > >   mov $LEN,%ecx;  /* len */   \
> > > > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx;
> > > > > \
> > > > >   callskb_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 

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

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 
> > > > Cc: Alexei Starovoitov 
> > > > Cc: net...@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 
> > > > +#include 
> > > >  
> > > >  /*
> > > >   * 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;\
> > > > pushSKBDATA;\
> > > >  /* rsi already has offset */   \
> > > > mov $LEN,%ecx;  /* len */   \
> > > > -   lea - MAX_BPF_STACK + 32(%rbp),%rdx;
> > > > \
> > > > callskb_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
> > 

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

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 
> > > > > > Cc: Alexei Starovoitov 
> > > > > > Cc: net...@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;\
> > > > > > pushSKBDATA;\
> > > > > >  /* rsi already has offset */   \
> > > > > > mov $LEN,%ecx;  /* len */   \
> > > > > > -   lea - MAX_BPF_STACK + 32(%rbp),%rdx;
> > > > > > \
> > > > > > callskb_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 

Thanks!

Can I assume your ack also applies to the previous patch which adds the
ELF annotations ("x86/asm/bpf: Annotate callable functions")?

-- 
Josh

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

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 
> > > > > > > Cc: Alexei Starovoitov 
> > > > > > > Cc: net...@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;\
> > > > > > >   pushSKBDATA;\
> > > > > > >  /* rsi already has offset */ \
> > > > > > >   mov $LEN,%ecx;  /* len */   \
> > > > > > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx;
> > > > > > > \
> > > > > > >   callskb_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 
> 
> 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.

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Ingo Molnar]

* Alexei Starovoitov  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

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

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 
> > > Cc: Alexei Starovoitov 
> > > Cc: net...@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 
> > > +#include 
> > >  
> > >  /*
> > >   * 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;\
> > >   pushSKBDATA;\
> > >  /* rsi already has offset */ \
> > >   mov $LEN,%ecx;  /* len */   \
> > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx;\
> > >   callskb_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?

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

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 
> > Cc: Alexei Starovoitov 
> > Cc: net...@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 
> > +#include 
> >  
> >  /*
> >   * 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;\
> > pushSKBDATA;\
> >  /* rsi already has offset */   \
> > mov $LEN,%ecx;  /* len */   \
> > -   lea - MAX_BPF_STACK + 32(%rbp),%rdx;\
> > callskb_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

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

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 
> Cc: Alexei Starovoitov 
> Cc: net...@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 
> +#include 
>  
>  /*
>   * 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;\
>   pushSKBDATA;\
>  /* rsi already has offset */ \
>   mov $LEN,%ecx;  /* len */   \
> - lea - MAX_BPF_STACK + 32(%rbp),%rdx;\
>   callskb_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.

[PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

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 
Cc: Alexei Starovoitov 
Cc: net...@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 
+#include 
 
 /*
  * 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;\
pushSKBDATA;\
 /* rsi already has offset */   \
mov $LEN,%ecx;  /* len */   \
-   lea - MAX_BPF_STACK + 32(%rbp),%rdx;\
callskb_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;\
pushSKBDATA;\
@@ -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

[PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

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 
Cc: Alexei Starovoitov 
Cc: net...@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 
+#include 
 
 /*
  * 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;\
pushSKBDATA;\
 /* rsi already has offset */   \
mov $LEN,%ecx;  /* len */   \
-   lea - MAX_BPF_STACK + 32(%rbp),%rdx;\
callskb_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;\
pushSKBDATA;\
@@ -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

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

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 
> > > Cc: Alexei Starovoitov 
> > > Cc: net...@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 
> > > +#include 
> > >  
> > >  /*
> > >   * 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;\
> > >   pushSKBDATA;\
> > >  /* rsi already has offset */ \
> > >   mov $LEN,%ecx;  /* len */   \
> > > - lea - MAX_BPF_STACK + 32(%rbp),%rdx;\
> > >   callskb_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?

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

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 
> Cc: Alexei Starovoitov 
> Cc: net...@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 
> +#include 
>  
>  /*
>   * 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;\
>   pushSKBDATA;\
>  /* rsi already has offset */ \
>   mov $LEN,%ecx;  /* len */   \
> - lea - MAX_BPF_STACK + 32(%rbp),%rdx;\
>   callskb_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.

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

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 
> > Cc: Alexei Starovoitov 
> > Cc: net...@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 
> > +#include 
> >  
> >  /*
> >   * 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;\
> > pushSKBDATA;\
> >  /* rsi already has offset */   \
> > mov $LEN,%ecx;  /* len */   \
> > -   lea - MAX_BPF_STACK + 32(%rbp),%rdx;\
> > callskb_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

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Ingo Molnar]

* Alexei Starovoitov  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