netdev - Re: [PATCH bpf-next 4/6] bpf, libbpf: add bpf_tail_call

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 25 Sep 2020 09:50:03 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        john fastabend <john.fastabend@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next 4/6] bpf, libbpf: add bpf_tail_call_static helper
 for bpf programs

On Fri, Sep 25, 2020 at 8:52 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 9/25/20 5:42 PM, Daniel Borkmann wrote:
> > On 9/25/20 12:17 AM, Daniel Borkmann wrote:
> >> On 9/24/20 10:53 PM, Andrii Nakryiko wrote:
> >>> On Thu, Sep 24, 2020 at 11:22 AM Daniel Borkmann <daniel@...earbox.net> wrote:
> >>>>
> >>>> Port of tail_call_static() helper function from Cilium's BPF code base [0]
> >>>> to libbpf, so others can easily consume it as well. We've been using this
> >>>> in production code for some time now. The main idea is that we guarantee
> >>>> that the kernel's BPF infrastructure and JIT (here: x86_64) can patch the
> >>>> JITed BPF insns with direct jumps instead of having to fall back to using
> >>>> expensive retpolines. By using inline asm, we guarantee that the compiler
> >>>> won't merge the call from different paths with potentially different
> >>>> content of r2/r3.
> >>>>
> >>>> We're also using __throw_build_bug() macro in different places as a neat
> >>>> trick to trigger compilation errors when compiler does not remove code at
> >>>> compilation time. This works for the BPF backend as it does not implement
> >>>> the __builtin_trap().
> >>>>
> >>>>    [0] https://github.com/cilium/cilium/commit/f5537c26020d5297b70936c6b7d03a1e412a1035
> >>>>
> >>>> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> >>>> ---
> >>>>   tools/lib/bpf/bpf_helpers.h | 32 ++++++++++++++++++++++++++++++++
> >>>>   1 file changed, 32 insertions(+)
> >>>>
> >>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> >>>> index 1106777df00b..18b75a4c82e6 100644
> >>>> --- a/tools/lib/bpf/bpf_helpers.h
> >>>> +++ b/tools/lib/bpf/bpf_helpers.h
> >>>> @@ -53,6 +53,38 @@
> >>>>          })
> >>>>   #endif
> >>>>
> >>>> +/*
> >>>> + * Misc useful helper macros
> >>>> + */
> >>>> +#ifndef __throw_build_bug
> >>>> +# define __throw_build_bug()   __builtin_trap()
> >>>> +#endif
> >>>
> >>> this will become part of libbpf stable API, do we want/need to expose
> >>> it? If we want to expose it, then we should probably provide a better
> >>> description.
> >>>
> >>> But also curious, how is it better than _Static_assert() (see
> >>> test_cls_redirect.c), which also allows to provide a better error
> >>> message?
> >>
> >> Need to get back to you whether that has same semantics. We use the __throw_build_bug()
> >> also in __bpf_memzero() and friends [0] as a way to trigger a hard build bug if we hit
> >> a default switch-case [0], so we detect unsupported sizes which are not covered by the
> >> implementation yet. If _Static_assert (0, "foo") does the trick, we could also use that;
> >> will check with our code base.
> >
> > So _Static_assert() won't work here, for example consider:
> >
> >    # cat f1.c
> >    int main(void)
> >    {
> >      if (0)
> >          _Static_assert(0, "foo");
> >      return 0;
> >    }
> >    # clang -target bpf -Wall -O2 -c f1.c -o f1.o
> >    f1.c:4:3: error: expected expression
> >                  _Static_assert(0, "foo");
> >                  ^
> >    1 error generated.
>
> .. aaand it looks like I need some more coffee. ;-) But result is the same after all:
>
>    # clang -target bpf -Wall -O2 -c f1.c -o f1.o
>    f1.c:4:3: error: static_assert failed "foo"
>                  _Static_assert(0, "foo");
>                  ^              ~
>    1 error generated.
>
>    # cat f1.c
>    int main(void)
>    {
>         if (0) {
>                 _Static_assert(0, "foo");
>         }
>         return 0;
>    }

You need still more :-P. For you use case it will look like this:

$ cat test-bla.c
int bar(int x) {
       _Static_assert(!__builtin_constant_p(x), "not a constant!");
       return x;
}

int foo() {
        bar(123);
        return 0;
}
$ clang -target bpf -O2 -c test-bla.c -o test-bla.o
$ echo $?
0

But in general to ensure unreachable code it's probably useful anyway
to have this. How about calling it __bpf_build_error() or maybe even
__bpf_unreachable()?

>
> > In order for it to work as required form the use-case, the _Static_assert() must not trigger
> > here given the path is unreachable and will be optimized away. I'll add a comment to the
> > __throw_build_bug() helper. Given libbpf we should probably also prefix with bpf_. If you see
> > a better name that would fit, pls let me know.
> >
> >>    [0] https://github.com/cilium/cilium/blob/master/bpf/include/bpf/builtins.h
> > Thanks,
> > Daniel
>