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 <jpoim...@redhat.com>
> > > > > > Cc: Alexei Starovoitov <a...@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 <a...@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
