Re: [PATCH v2 bpf 02/11] bpftool: define a local bpf_perf_link to fix accessing its fields

[Quentin Monnet]

On Fri, 21 Apr 2023 at 08:39, Michal Suchánek <msuchanek@xxxxxxx> wrote:
>
> On Thu, Apr 20, 2023 at 04:07:38PM -0700, Andrii Nakryiko wrote:
> > On Fri, Apr 14, 2023 at 9:28 AM Michal Suchánek <msuchanek@xxxxxxx> wrote:
> > >
> > > On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote:
> > > > From: Michal Suchánek <msuchanek@xxxxxxx>
> > > > Date: Fri, 14 Apr 2023 11:54:57 +0200
> > > >
> > > > > Hello,
> > > >
> > > > Hey-hey,
> > > >
> > > > >
> > > > > On Thu, Apr 21, 2022 at 12:38:58AM +0000, Alexander Lobakin wrote:
> > > > >> When building bpftool with !CONFIG_PERF_EVENTS:
> > > > >>
> > > > >> skeleton/pid_iter.bpf.c:47:14: error: incomplete definition of type 'struct bpf_perf_link'
> > > > >>         perf_link = container_of(link, struct bpf_perf_link, link);
> > > > >>                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:74:22: note: expanded from macro 'container_of'
> > > > >>                 ((type *)(__mptr - offsetof(type, member)));    \
> > > > >>                                    ^~~~~~~~~~~~~~~~~~~~~~
> > > > >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:68:60: note: expanded from macro 'offsetof'
> > > > >>  #define offsetof(TYPE, MEMBER)  ((unsigned long)&((TYPE *)0)->MEMBER)
> > > > >>                                                   ~~~~~~~~~~~^
> > > > >> skeleton/pid_iter.bpf.c:44:9: note: forward declaration of 'struct bpf_perf_link'
> > > > >>         struct bpf_perf_link *perf_link;
> > > > >>                ^
> > > > >>
> > > > >> &bpf_perf_link is being defined and used only under the ifdef.
> > > > >> Define struct bpf_perf_link___local with the `preserve_access_index`
> > > > >> attribute inside the pid_iter BPF prog to allow compiling on any
> > > > >> configs. CO-RE will substitute it with the real struct bpf_perf_link
> > > > >> accesses later on.
> > > > >> container_of() is not CO-REd, but it is a noop for
> > > > >> bpf_perf_link <-> bpf_link and the local copy is a full mirror of
> > > > >> the original structure.
> > > > >>
> > > > >> Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output")
> > > > >
> > > > > This does not solve the problem completely. Kernels that don't have
> > > > > CONFIG_PERF_EVENTS in the first place are also missing the enum value
> > > > > BPF_LINK_TYPE_PERF_EVENT which is used as the condition for handling the
> > > > > cookie.
> > > >
> > > > Sorry, I haven't been working with my home/private stuff for more than a
> > > > year already. I may get back to it some day when I'm tired of Lua (curse
> > > > words, sorry :D), but for now the series is "a bit" abandoned.
> > >
> > > This part still appllies and works for me with the caveat that
> > > BPF_LINK_TYPE_PERF_EVENT also needs to be defined.
> > >
> > > > I think there was alternative solution proposed there, which promised to
> > > > be more flexible. But IIRC it also doesn't touch the enum (was it added
> > > > recently? Because it was building just fine a year ago on config without
> > > > perf events).
> > >
> > > It was added in 5.15. Not sure there is a kernel.org LTS kernel usable
> > > for CO-RE that does not have it, technically 5.4 would work if it was
> > > built monolithic, it does not have module BTF, only kernel IIRC.
> > >
> > > Nonetheless, the approach to handling features completely missing in the
> > > running kernel should be figured out one way or another. I would be
> > > surprised if this was the last feature to be added that bpftool needs to
> > > know about.
> >
> > Are we talking about bpftool built from kernel sources or from Github?
> > Kernel source version should have access to latest UAPI headers and so
> > BPF_LINK_TYPE_PERF_EVENT should be available. Github version, if it
> > doesn't do that already, can use UAPI headers distributed (and used
> > for building) with libbpf through submodule.
>
> It does have a copy of the uapi headers but apparently does not use
> them. Using them directly might cause conflict with vmlinux.h, though.

Indeed, using the UAPI header here conflicts with vmlinux.h.

Looking again at some code I started last year but never finalised, I
used the following approach, redefining BPF_LINK_TYPE_PERF_EVENT with
CO-RE:

    enum bpf_link_type___local {
        BPF_LINK_TYPE_PERF_EVENT___local = 7,
    };

Then guarding accordingly in iter():

    [...]
    if (obj_type == BPF_OBJ_LINK &&
        bpf_core_enum_value_exists(enum bpf_link_type___local,
                       BPF_LINK_TYPE_PERF_EVENT___local)) {
        struct bpf_link *link = (struct bpf_link *) file->private_data;

        if (link->type == bpf_core_enum_value(enum bpf_link_type___local,
                  BPF_LINK_TYPE_PERF_EVENT___local)) {
            e.has_bpf_cookie = true;
            e.bpf_cookie = get_bpf_cookie(link);
        }
    }
    [...]

Would that approach make sense? I had a VM around with kernel 5.8, and
bpftool compiles there with that change. If I remember correctly, some
older kernel versions required yet more CO-RE work in pid_iter.bpf.c,
and at some point I was struggling, which is why I never submitted
this set.

If this approach looks correct to you Andrii, I can resubmit these
patches with my addition so we can at least fix the build on 5.8
onwards.

Thanks,
Quentin