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 [off-list ref] wrote:

On 9/25/20 5:42 PM, Daniel Borkmann wrote:

quoted

On 9/25/20 12:17 AM, Daniel Borkmann wrote:

quoted

On 9/24/20 10:53 PM, Andrii Nakryiko wrote:

quoted

On Thu, Sep 24, 2020 at 11:22 AM Daniel Borkmann [off-list ref] wrote:

quoted

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@iogearbox.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()?

quoted

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.

quoted

   [0] https://github.com/cilium/cilium/blob/master/bpf/include/bpf/builtins.h

Thanks,
Daniel