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

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:

quoted

On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:

quoted

On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:

quoted

On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:

quoted

On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:

quoted

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 <redacted>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: netdev@vger.kernel.org
---
 arch/x86/net/bpf_jit.S | 9 +++++++--

...

quoted

quoted

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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