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 <jpoimboe@xxxxxxxxxx>
> > > > > > Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> > > > > > Cc: netdev@xxxxxxxxxxxxxxx
> > > > > > ---
> > > > > >  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@xxxxxxxxxx>

Thanks!

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

-- 
Josh