Portability of bpf_tracing.h

Portability of bpf_tracing.h

[Lorenz Bauer]

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

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

Re: Portability of bpf_tracing.h

[Andrii Nakryiko]

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

Re: Portability of bpf_tracing.h

[John Fastabend]

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

Re: Portability of bpf_tracing.h

[Andrii Nakryiko]

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

Re: Portability of bpf_tracing.h

[Lorenz Bauer]

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.

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

www.cloudflare.com

Re: Portability of bpf_tracing.h

[Andrii Nakryiko]

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

Re: Portability of bpf_tracing.h

[Lorenz Bauer]

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.

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

www.cloudflare.com

Re: Portability of bpf_tracing.h

[Andrii Nakryiko]

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

Re: Portability of bpf_tracing.h

[Lorenz Bauer]

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.

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

www.cloudflare.com

Re: Portability of bpf_tracing.h

[Alexei Starovoitov]

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.