| Message ID | 20220421003152.339542-3-alobakin@pm.me (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: random unpopular userspace fixes (32 bit et al) | expand |
Hello, 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. Thanks Michal
From: Michal Suchánek <msuchanek@suse.de> 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. 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). > > Thanks > > Michal > Thanks, Olek
On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote: > From: Michal Suchánek <msuchanek@suse.de> > 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. Thanks Michal
On Fri, Apr 14, 2023 at 9:28 AM Michal Suchánek <msuchanek@suse.de> wrote: > > On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote: > > From: Michal Suchánek <msuchanek@suse.de> > > 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. > > Thanks > > Michal
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@suse.de> wrote: > > > > On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote: > > > From: Michal Suchánek <msuchanek@suse.de> > > > 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. Thanks Michal
On Fri, 21 Apr 2023 at 08:39, Michal Suchánek <msuchanek@suse.de> 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@suse.de> wrote: > > > > > > On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote: > > > > From: Michal Suchánek <msuchanek@suse.de> > > > > 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
On Wed, May 3, 2023 at 4:44 PM Quentin Monnet <quentin@isovalent.com> wrote: > > On Fri, 21 Apr 2023 at 08:39, Michal Suchánek <msuchanek@suse.de> 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@suse.de> wrote: > > > > > > > > On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote: > > > > > From: Michal Suchánek <msuchanek@suse.de> > > > > > 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. Yep, why not? In general, if using vmlinux.h makes life harder and there is just a small set of types and enums BPF program needs, for the sake of support of old kernels/distros it might be cleaner just to define relevant structs, enums, etc explicitly and add __attribute__((preserve_access_index)) to them to make the CO-RE-relocatable > > Thanks, > Quentin
Hello, On Thu, May 04, 2023 at 12:43:52AM +0100, Quentin Monnet wrote: > On Fri, 21 Apr 2023 at 08:39, Michal Suchánek <msuchanek@suse.de> 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@suse.de> wrote: > > > > > > > > On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote: > > > > > From: Michal Suchánek <msuchanek@suse.de> > > > > > 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, > }; That's the same as I did except I used simple define instead of this fake enum. The enum only has value when it is complete and the compiler can check that a switch uses only known values, and can confuse things when values are missing. Thanks Michal
On 5/4/23 1:18 AM, Michal Suchánek wrote: > Hello, > > On Thu, May 04, 2023 at 12:43:52AM +0100, Quentin Monnet wrote: >> On Fri, 21 Apr 2023 at 08:39, Michal Suchánek <msuchanek@suse.de> 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@suse.de> wrote: >>>>> >>>>> On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote: >>>>>> From: Michal Suchánek <msuchanek@suse.de> >>>>>> 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, >> }; > > That's the same as I did except I used simple define instead of this > fake enum. > > The enum only has value when it is complete and the compiler can check > that a switch uses only known values, and can confuse things when values > are missing. Currently, enum value CORE is done though a llvm builtin function. So if the enum value is used in switch cases like switch(...) case BPF_LINK_TYPE_PERF_EVENT: ... CORE relocation will not work in that case since the compiler expects BPF_LINK_TYPE_PERF_EVENT to be a constant. > Thanks > > Michal
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c index e2af8e5fb29e..3a4c4f7d83d8 100644 --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c @@ -15,6 +15,11 @@ enum bpf_obj_type { BPF_OBJ_BTF, }; +struct bpf_perf_link___local { + struct bpf_link link; + struct file *perf_file; +} __attribute__((preserve_access_index)); + struct perf_event___local { u64 bpf_cookie; } __attribute__((preserve_access_index)); @@ -45,10 +50,10 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type) /* could be used only with BPF_LINK_TYPE_PERF_EVENT links */ static __u64 get_bpf_cookie(struct bpf_link *link) { + struct bpf_perf_link___local *perf_link; struct perf_event___local *event; - struct bpf_perf_link *perf_link; - perf_link = container_of(link, struct bpf_perf_link, link); + perf_link = container_of(link, struct bpf_perf_link___local, link); event = BPF_CORE_READ(perf_link, perf_file, private_data); return BPF_CORE_READ(event, bpf_cookie); }
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") Suggested-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Alexander Lobakin <alobakin@pm.me> --- tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) -- 2.36.0