Re: Portability of bpf_tracing.h

[Lorenz Bauer <lmb@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.

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

www.cloudflare.com