Andrii Nakryiko wrote:

On Mon, May 24, 2021 at 8:05 AM Lorenz Bauer [off-list ref] wrote:

quoted

Hi Andrii,

A user of bpf2go [1] recently ran into the problem of PT_REGS not
being defined after including bpf_tracing.h. It turns out this is
because we by default compile for bpfel / bpfeb so the logic in the
header file doesn't kick in. I originally filed [2] as a quick fix,
but looking at the issue some more made me wonder how to fit this into
bpf2go better.

Basically, the convention of bpf2go is that the compiled BPF is
checked into the source code repository to facilitate distributing BPF
as part of Go packages (as opposed to libbpf tooling which doesn't
include generated source). To make this portable, bpf2go by default
generates both bpfel and bpfeb variants of the C.

However, code using bpf_tracing.h is inherently non-portable since the
fields of struct pt_regs differ in name between platforms. It seems
like this forces one to compile to each possible __TARGET_ARCH
separately. If that is correct, could we extend CO-RE somehow to cover
this as well?

If there are enums/types/fields that we can use to reliably detect the
platform, then yes, we can have a new set of helpers that would do
this with CO-RE. Someone will need to investigate how to do that for
all the platforms we have. It's all about finding something that's
already in the kernel and can server as a reliably indicator of a
target architecture.

quoted

If that isn't possible, I want to avoid compiling and shipping BPF for
each possible __TARGET_ARCH_xxx by default. Instead I would like to
achieve:
* Code that doesn't use bpf_tracing.h is distributed in bpfel and bpfeb variants
* Code that uses bpf_tracing.h has to explicitly opt into the
supported platforms via a flag to bpf2go

The latter point is because the go tooling has to know the target arch
to be able to generate the correct Go wrappers. How would you feel
about adding something like the following to bpf_tracing.h:

Well, obviously I'm not a fan of even more magic #defines. But I think
we can achieve a similar effect with a more "lazy" approach. I.e., if
user tries to use PT_REGS_xxx macros but doesn't specify the platform
-- only then it gets compilation errors. There is stuff in
bpf_tracing.h that doesn't need pt_regs, so we can't just outright do
#error unconditinally. But we can do something like this:

#else /* !bpf_target_defined */

#define PT_REGS_PARM1(x) _Pragma("GCC error \"blah blah something
user-facing\"")

... and so on for all macros

#endif

Thoughts?

I don't use bpf_tracing.h, but...

Can we do this similar to feature discovery and simply have the
user space tell us the platform? I at least do this fairly
frequently so have infra in place on my side for user space to
push down feature flags/fields. One of those could be platform then
we just need helpers,

  get_pt_regs_parm() {
    if (bpf_target_defined == is_x86)
     ...
    else if (bpf_target_defined == is_foo)
     ...
    else {
      hard_load_error()
    }
  }
    
I think we are suggesting the same thing? Then user would need to
have bpf_target_Defined set but that should be OK and the other
conditions should all look like dead code.

While on the topic, I've noticed that we added BPF_SEQ_PRINTF and
BPF_SNPRINTF into bpf_tracing.h, which seems like not the best fit
(definitely not for BPF_SNPRINTF). Florent, can you please help moving
them into bpf_helpers.h, as it's really more generic functionality
rather than low-level tracing primitives. I think it was put here
because we needed ___bpf_narg macros, but I'd rather copy/paste them
into bpf_helpers.h (they won't change at all) and put
BPF_SNPRINTF/BPF_SEQ_PRINTF into bpf_helpers.h.

quoted

--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h

@@ -25,6 +25,9 @@
        #define bpf_target_sparc
        #define bpf_target_defined
 #else
+       #if defined(BPF_REQUIRE_EXPLICIT_TARGET_ARCH)
+               #error BPF_REQUIRE_EXPLICIT_TARGET_ARCH set and no

target arch defined
+       #endif
        #undef bpf_target_defined
 #endif

bpf2go would always define BPF_REQUIRE_EXPLICIT_TARGET_ARCH. If the
user included bpf_tracing.h they get this error. They can then add
-target amd64, etc. and the tooling then defines __TARGET_ARCH_x86_64

1: https://pkg.go.dev/github.com/cilium/ebpf/cmd/bpf2go
2: https://github.com/cilium/ebpf/issues/305

--
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com