On Fri, 13 Dec 2019 at 06:30, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Dec 11, 2019 at 01:30:13PM +0100, Björn Töpel wrote:
> > +
> > +#define DEFINE_BPF_DISPATCHER(name) \
> > + unsigned int name##func( \
> > + const void *xdp_ctx, \
> > + const struct bpf_insn *insnsi, \
> > + unsigned int (*bpf_func)(const void *, \
> > + const struct bpf_insn *)) \
> > + { \
> > + return bpf_func(xdp_ctx, insnsi); \
> > + } \
> > + EXPORT_SYMBOL(name##func); \
> > + struct bpf_dispatcher name = BPF_DISPATCHER_INIT(name);
>
> The dispatcher function is a normal function. EXPORT_SYMBOL doesn't make it
> 'noinline'. struct bpf_dispatcher takes a pointer to it and that address is
> used for text_poke.
>
> In patch 3 __BPF_PROG_RUN calls dfunc() from two places.
> What stops compiler from inlining it? Or partially inlining it in one
> or the other place?
>
Good catch. No inlining for the XDP dispatcher is possible, since the
trampoline function is in a different compilation unit (filter.o),
than the users of bpf_prog_run_xdp(). Turning on LTO, this would no
longer be true. So, *not* having it marked as noinline is a bug.
> I guess it works, because your compiler didn't inline it?
> Could you share how asm looks for bpf_prog_run_xdp()
> (where __BPF_PROG_RUN is called) and asm for name##func() ?
>
Sure! bpf_prog_run_xdp() is always inlined, so let's look at:
net/bpf/test_run.c:bpf_test_run:
if (xdp)
*retval = bpf_prog_run_xdp(prog, ctx);
else
*retval = BPF_PROG_RUN(prog, ctx);
translates to:
0xffffffff8199f522 <+162>: nopl 0x0(%rax,%rax,1)
./include/linux/filter.h:
716 return __BPF_PROG_RUN(prog, xdp,
0xffffffff8199f527 <+167>: mov 0x30(%rbp),%rdx
0xffffffff8199f52b <+171>: mov %r14,%rsi
0xffffffff8199f52e <+174>: mov %r13,%rdi
0xffffffff8199f531 <+177>: callq 0xffffffff819586d0
<bpf_dispatcher_xdpfunc>
0xffffffff8199f536 <+182>: mov %eax,%ecx
net/bpf/test_run.c:
48 *retval = BPF_PROG_RUN(prog, ctx);
0xffffffff8199f538 <+184>: mov (%rsp),%rax
0xffffffff8199f53c <+188>: mov %ecx,(%rax)
...
net/bpf/test_run.c:
45 if (xdp)
0xffffffff8199f582 <+258>: test %r15b,%r15b
0xffffffff8199f585 <+261>: jne 0xffffffff8199f522 <bpf_test_run+162>
0xffffffff8199f587 <+263>: nopl 0x0(%rax,%rax,1)
./include/linux/bpf.h:
497 return bpf_func(ctx, insnsi);
0xffffffff8199f58c <+268>: mov 0x30(%rbp),%rax
0xffffffff8199f590 <+272>: mov %r14,%rsi
0xffffffff8199f593 <+275>: mov %r13,%rdi
0xffffffff8199f596 <+278>: callq 0xffffffff81e00eb0
<__x86_indirect_thunk_rax>
0xffffffff8199f59b <+283>: mov %eax,%ecx
0xffffffff8199f59d <+285>: jmp 0xffffffff8199f538 <bpf_test_run+184>
The "dfunc":
net/core/filter.c:
8944 DEFINE_BPF_DISPATCHER(bpf_dispatcher_xdp)
0xffffffff819586d0 <+0>: callq 0xffffffff81c01680 <__fentry__>
0xffffffff819586d5 <+5>: jmpq 0xffffffff81e00f10
<__x86_indirect_thunk_rdx>
> I hope my guess that compiler didn't inline it is correct. Then extra noinline
> will not hurt and that's the only thing needed to avoid the issue.
>
I'd say it's broken not marking it as noinline, and I was lucky. It
would break if other BPF entrypoints that are being called from
filter.o would appear. I'll wait for more comments, and respin a v5
after the weekend.
Thanks,
Björn
