netdev - Re: [PATCH bpf-next v4 2/6] bpf: introduce BPF dispatcher

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Dec 2019 08:51:47 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Björn Töpel <bjorn.topel@...el.com>,
        bpf <bpf@...r.kernel.org>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Edward Cree <ecree@...arflare.com>,
        Toke Høiland-Jørgensen <thoiland@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>
Subject: Re: [PATCH bpf-next v4 2/6] bpf: introduce BPF dispatcher

On Fri, 13 Dec 2019 at 06:30, Alexei Starovoitov
<alexei.starovoitov@...il.com> 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