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

On Fri, 21 Apr 2023 at 08:39, Michal Suchánek [off-list ref] wrote:

On Thu, Apr 20, 2023 at 04:07:38PM -0700, Andrii Nakryiko wrote:

quoted

On Fri, Apr 14, 2023 at 9:28 AM Michal Suchánek [off-list ref] wrote:

quoted

On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote:

quoted

From: Michal Suchánek <redacted>
Date: Fri, 14 Apr 2023 11:54:57 +0200

quoted

Hello,

Hey-hey,

quoted

On Thu, Apr 21, 2022 at 12:38:58AM +0000, Alexander Lobakin wrote:

quoted

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.

quoted

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