Message-ID: <bdddb94c-555c-3833-f82e-a53f83b1a77e@iogearbox.net>
Date:   Fri, 25 Sep 2020 21:52:27 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
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 9/25/20 6:50 PM, Andrii Nakryiko wrote:
> 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

Right, but that won't work for example for the use case to detect switch cases which fall
into default case as mentioned with the mem* optimizations earlier in this thread.

> 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()?

I think the __bpf_unreachable() sounds best to me, will use that.

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

Powered by blists - more mailing lists