Re: Portability of bpf_tracing.h

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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