| Message ID | CACAyw9-GQasDdE9m_f3qXCO1UrR49YuF_6K1tjGxyk+ZZGhM-Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | Portability of bpf_tracing.h | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, May 24, 2021 at 8:05 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > 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. > > 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? 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. > > --- 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
Andrii Nakryiko wrote: > On Mon, May 24, 2021 at 8:05 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > > > 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. > > > > > 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. > > > > > --- 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
On Mon, May 24, 2021 at 12:31 PM John Fastabend <john.fastabend@gmail.com> wrote: > > Andrii Nakryiko wrote: > > On Mon, May 24, 2021 at 8:05 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > > > > > 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. > > > > > > > > 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. It's almost what I propose, except I suggest to get rid of the need for a user to specify the arch they expect/need, and use CO-RE to detect this. What you propose would work, but it's usability is worse than the CO-RE-based variant and it requires to hard-code struct pt_regs exact definitions for each platform in bpf_tracing.h, which sucks. And it would be like a third way to achieve the same thing (with a different set of tradeoffs). > > > > > > > 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. > > > > > > > > --- 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 > >
On Mon, 24 May 2021 at 18:48, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > 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. Can you explain a bit more how this would work? Seems like leg work I could do. > 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? That would work for me, but it would change the behaviour for current users of the header, no? That's why I added the magic define in the first place.
On Wed, May 26, 2021 at 2:13 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > On Mon, 24 May 2021 at 18:48, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > 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. > > Can you explain a bit more how this would work? Seems like leg work I could do. > So I did a bit of investigation and gathered struct pt_regs definitions from all the "supported" architectures in bpf_tracing.h. I'll leave it here for further reference. i386 ==== struct pt_regs { unsigned long bx; unsigned long cx; unsigned long dx; unsigned long si; unsigned long di; unsigned long bp; unsigned long ax; unsigned short ds; unsigned short __dsh; unsigned short es; unsigned short __esh; unsigned short fs; unsigned short __fsh; unsigned short gs; unsigned short __gsh; unsigned long orig_ax; unsigned long ip; unsigned short cs; unsigned short __csh; unsigned long flags; unsigned long sp; unsigned short ss; unsigned short __ssh; }; x86-64 ====== struct pt_regs { unsigned long r15; unsigned long r14; unsigned long r13; unsigned long r12; unsigned long bp; unsigned long bx; unsigned long r11; unsigned long r10; unsigned long r9; unsigned long r8; unsigned long ax; unsigned long cx; unsigned long dx; unsigned long si; unsigned long di; unsigned long orig_ax; unsigned long ip; unsigned long cs; unsigned long flags; unsigned long sp; unsigned long ss; }; s390 ==== struct pt_regs { union { user_pt_regs user_regs; struct { unsigned long args[1]; psw_t psw; unsigned long gprs[NUM_GPRS]; }; }; unsigned long orig_gpr2; unsigned int int_code; unsigned int int_parm; unsigned long int_parm_long; unsigned long flags; unsigned long cr1; }; arm === struct pt_regs { unsigned long uregs[18]; }; arm64 ===== struct pt_regs { union { struct user_pt_regs user_regs; struct { u64 regs[31]; u64 sp; u64 pc; u64 pstate; }; }; u64 orig_x0; #ifdef __AARCH64EB__ u32 unused2; s32 syscallno; #else s32 syscallno; u32 unused2; #endif u64 sdei_ttbr1; u64 pmr_save; u64 stackframe[2]; u64 lockdep_hardirqs; u64 exit_rcu; }; mips ==== struct pt_regs { #ifdef CONFIG_32BIT unsigned long pad0[8]; #endif unsigned long regs[32]; unsigned long cp0_status; unsigned long hi; unsigned long lo; #ifdef CONFIG_CPU_HAS_SMARTMIPS unsigned long acx; #endif unsigned long cp0_badvaddr; unsigned long cp0_cause; unsigned long cp0_epc; #ifdef CONFIG_CPU_CAVIUM_OCTEON unsigned long long mpl[6]; /* MTM{0-5} */ unsigned long long mtp[6]; /* MTP{0-5} */ #endif unsigned long __last[0]; } __aligned(8); powerpc ======= struct pt_regs { union { struct user_pt_regs user_regs; struct { unsigned long gpr[32]; unsigned long nip; unsigned long msr; unsigned long orig_gpr3; unsigned long ctr; unsigned long link; unsigned long xer; unsigned long ccr; #ifdef CONFIG_PPC64 unsigned long softe; #else unsigned long mq; #endif unsigned long trap; unsigned long dar; unsigned long dsisr; unsigned long result; }; }; union { struct { #ifdef CONFIG_PPC64 unsigned long ppr; #endif union { #ifdef CONFIG_PPC_KUAP unsigned long kuap; #endif #ifdef CONFIG_PPC_PKEY unsigned long amr; #endif }; #ifdef CONFIG_PPC_PKEY unsigned long iamr; #endif }; unsigned long __pad[4]; /* Maintain 16 byte interrupt stack alignment */ }; }; sparc ===== struct pt_regs { unsigned long u_regs[16]; /* globals and ins */ unsigned long tstate; unsigned long tpc; unsigned long tnpc; unsigned int y; /* We encode a magic number, PT_REGS_MAGIC, along * with the %tt (trap type) register value at trap * entry time. The magic number allows us to identify * accurately a trap stack frame in the stack * unwinder, and the %tt value allows us to test * things like "in a system call" etc. for an arbitray * process. * * The PT_REGS_MAGIC is chosen such that it can be * loaded completely using just a sethi instruction. */ unsigned int magic; }; Now, note how each architecture has some uniquely named fields. Assuming we pick something that is not going to get renamed easily, we should be able to do something like this: struct pt_regs___x86 { unsigned long di; } __attribute__((preserve_access_index)); struct pt_regs___s390 { unsigned long gprs[NUM_GPRS]; } __attribute__((preserve_access_index)); struct pt_regs___powerpc { unsigned long gpr[32] } __attribute__((preserve_access_index)); /* and so on for all arches */ Then PT_REGS_PARM1 CO-RE equivalent would be implemented like this: #define ___arch_is_x86 (bpf_core_field_exists(((struct pt_regs___x86 *)0)->di)) #define ___arch_is_s390 (bpf_core_field_exists(((struct pt_regs___s390 *)0)->gprs)) #define ___arch_is_powerpc (bpf_core_field_exists(((struct pt_regs___powerpc *)0)->gpr)) static unsigned long bpf_pt_regs_parm1(const void *regs) { if (___arch_is_x86) return ((struct pt_regs___x86 *)regs)->di; else if (___arch_is_s390) return ((struct pt_regs___s390 *)regs)->gprs[2]; else if (___arch_is_powerpc) return ((struct pt_regs___powerpc *)regs)->gpr[3]; else while(1); /* need some better way to force BPF verification failure */ } And so on for other architectures and other helpers, you should get the idea from the above. As a shameless plug, if you'd like to see some more examples of using CO-RE for detecting kernel features, see [0] [0] https://nakryiko.com/posts/bpf-tips-printk/ > > 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? > > That would work for me, but it would change the behaviour for current > users of the header, no? That's why I added the magic define in the > first place. How so? If someone is using PT_REGS_PARM1 without setting target arch they should get compilation error about undefined macro. Here it will be the same thing, only if someone tries to use PT_REGS_PARM1() will they reach that _Pragma. Or am I missing something? > > -- > Lorenz Bauer | Systems Engineer > 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK > > www.cloudflare.com
On Wed, 26 May 2021 at 19:34, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > So I did a bit of investigation and gathered struct pt_regs > definitions from all the "supported" architectures in bpf_tracing.h. > I'll leave it here for further reference. > > static unsigned long bpf_pt_regs_parm1(const void *regs) > { > if (___arch_is_x86) > return ((struct pt_regs___x86 *)regs)->di; > else if (___arch_is_s390) > return ((struct pt_regs___s390 *)regs)->gprs[2]; > else if (___arch_is_powerpc) > return ((struct pt_regs___powerpc *)regs)->gpr[3]; > else > while(1); /* need some better way to force BPF verification failure */ > } > > And so on for other architectures and other helpers, you should get > the idea from the above. The idea of basing this on unique fields in types is neat, the downside I see is that we encode the logic in the BPF bitstream. If in the future struct pt_regs is changed, code breaks and we can't do much about it. What if instead we replace ___arch_is_x86, etc. with a .kconfig style constant load? The platform detection logic can then live in libbpf or cilium/ebpf and can be evolved if needed. Instead of while(1) we could use an illegal function call, like we do for poisoned CORE relocations. > > As a shameless plug, if you'd like to see some more examples of using > CO-RE for detecting kernel features, see [0] > > [0] https://nakryiko.com/posts/bpf-tips-printk/ > > > > 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? > > > > That would work for me, but it would change the behaviour for current > > users of the header, no? That's why I added the magic define in the > > first place. > > How so? If someone is using PT_REGS_PARM1 without setting target arch > they should get compilation error about undefined macro. Here it will > be the same thing, only if someone tries to use PT_REGS_PARM1() will > they reach that _Pragma. > > Or am I missing something? Right! Doing this makes sense regardless of the outcome of our discussion above.
On Fri, May 28, 2021 at 1:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > On Wed, 26 May 2021 at 19:34, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > So I did a bit of investigation and gathered struct pt_regs > > definitions from all the "supported" architectures in bpf_tracing.h. > > I'll leave it here for further reference. > > > > static unsigned long bpf_pt_regs_parm1(const void *regs) > > { > > if (___arch_is_x86) > > return ((struct pt_regs___x86 *)regs)->di; > > else if (___arch_is_s390) > > return ((struct pt_regs___s390 *)regs)->gprs[2]; > > else if (___arch_is_powerpc) > > return ((struct pt_regs___powerpc *)regs)->gpr[3]; > > else > > while(1); /* need some better way to force BPF verification failure */ > > } > > > > And so on for other architectures and other helpers, you should get > > the idea from the above. > > The idea of basing this on unique fields in types is neat, the > downside I see is that we encode the logic in the BPF bitstream. If in > the future struct pt_regs is changed, code breaks and we can't do much If pt_regs fields are renamed all PT_REGS-related stuff, provided by libbpf in bpf_tracing.h will break as well and will require re-compilation of BPF application. This piece of code is going to be part of the same bpf_tracing.h, so if something changes in newer kernel version, libbpf will accommodate that in the latest version. You'd still need to re-compile your BPF application, but I don't see how that's avoidable even with your proposal. > about it. What if instead we replace ___arch_is_x86, etc. with a > .kconfig style constant load? The platform detection logic can then > live in libbpf or cilium/ebpf and can be evolved if needed. Instead of That might be worthwhile to do (similarly to how we have a special LINUX_KERNEL_VERSION extern) regardless. But again, detection of the architecture is just one part. Once you know the architecture, you are still relying on knowing pt_regs field names to extract the data. So if anything changes about that, you'd need to update bpf_tracing.h and re-compile. > while(1) we could use an illegal function call, like we do for > poisoned CORE relocations. Yeah, I knew something like that should be possible with assembly, but was too lazy to search for or invent it. > > > > > As a shameless plug, if you'd like to see some more examples of using > > CO-RE for detecting kernel features, see [0] > > > > [0] https://nakryiko.com/posts/bpf-tips-printk/ > > > > > > 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? > > > > > > That would work for me, but it would change the behaviour for current > > > users of the header, no? That's why I added the magic define in the > > > first place. > > > > How so? If someone is using PT_REGS_PARM1 without setting target arch > > they should get compilation error about undefined macro. Here it will > > be the same thing, only if someone tries to use PT_REGS_PARM1() will > > they reach that _Pragma. > > > > Or am I missing something? > > Right! Doing this makes sense regardless of the outcome of our discussion above. Cool, feel free to send a patch with _Pragmas and no extra #defines ;) > > -- > Lorenz Bauer | Systems Engineer > 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK > > www.cloudflare.com
On Sun, 30 May 2021 at 01:51, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, May 28, 2021 at 1:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > > > On Wed, 26 May 2021 at 19:34, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > So I did a bit of investigation and gathered struct pt_regs > > > definitions from all the "supported" architectures in bpf_tracing.h. > > > I'll leave it here for further reference. > > > > > > static unsigned long bpf_pt_regs_parm1(const void *regs) > > > { > > > if (___arch_is_x86) > > > return ((struct pt_regs___x86 *)regs)->di; > > > else if (___arch_is_s390) > > > return ((struct pt_regs___s390 *)regs)->gprs[2]; > > > else if (___arch_is_powerpc) > > > return ((struct pt_regs___powerpc *)regs)->gpr[3]; > > > else > > > while(1); /* need some better way to force BPF verification failure */ > > > } > > > > > > And so on for other architectures and other helpers, you should get > > > the idea from the above. > > > > The idea of basing this on unique fields in types is neat, the > > downside I see is that we encode the logic in the BPF bitstream. If in > > the future struct pt_regs is changed, code breaks and we can't do much > > If pt_regs fields are renamed all PT_REGS-related stuff, provided by > libbpf in bpf_tracing.h will break as well and will require > re-compilation of BPF application. I'm thinking more along the lines of, if a PT_REGS definition changes so that the unique field isn't unique anymore. The BPF is still valid, but the logic that determines the platform isn't. > This piece of code is going to be > part of the same bpf_tracing.h, so if something changes in newer > kernel version, libbpf will accommodate that in the latest version. > You'd still need to re-compile your BPF application, but I don't see > how that's avoidable even with your proposal. > > > about it. What if instead we replace ___arch_is_x86, etc. with a > > .kconfig style constant load? The platform detection logic can then > > live in libbpf or cilium/ebpf and can be evolved if needed. Instead of > > That might be worthwhile to do (similarly to how we have a special > LINUX_KERNEL_VERSION extern) regardless. But again, detection of the > architecture is just one part. Once you know the architecture, you are > still relying on knowing pt_regs field names to extract the data. So > if anything changes about that, you'd need to update bpf_tracing.h and > re-compile. Yes. It'd be nice to fix that, but I don't see how to do that in a generic fashion. So I'd deal with it when it happens. > > > How so? If someone is using PT_REGS_PARM1 without setting target arch > > > they should get compilation error about undefined macro. Here it will > > > be the same thing, only if someone tries to use PT_REGS_PARM1() will > > > they reach that _Pragma. > > > > > > Or am I missing something? > > > > Right! Doing this makes sense regardless of the outcome of our discussion above. > > Cool, feel free to send a patch with _Pragmas and no extra #defines ;) I'll give it a shot.
On Thu, Jun 10, 2021 at 7:12 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > > The idea of basing this on unique fields in types is neat, the > > > downside I see is that we encode the logic in the BPF bitstream. If in > > > the future struct pt_regs is changed, code breaks and we can't do much > > > > If pt_regs fields are renamed all PT_REGS-related stuff, provided by > > libbpf in bpf_tracing.h will break as well and will require > > re-compilation of BPF application. > > I'm thinking more along the lines of, if a PT_REGS definition changes > so that the unique field isn't unique anymore. The BPF is still valid, > but the logic that determines the platform isn't. struct pt_regs is uapi on every arch. They cannot change. New registers can be added :) but the chance is close to zero.
--- 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