| Message ID | 29d2a8c7-44cd-da42-5fed-f17ec0f8ccf2@synopsys.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/1] package/bpftool: revert bpf_cookie patch to allow building | expand |
ping!
Hi Shahab, On 15/06/2022 12:57, Shahab Vahedi wrote: > Building bpftool on Debian 11 (bullseye) with kernel v5.10 fails: > > -----------------------------------8<----------------------------------- > $ make > . > . > . > CLANG pid_iter.bpf.o > 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); > . > . > . > skeleton/pid_iter.bpf.c:49:30: error: no member named 'bpf_cookie' in > 'struct perf_event' > return BPF_CORE_READ(event, bpf_cookie); > . > . > . > 10 errors generated. > make: *** [Makefile:176: pid_iter.bpf.o] Error 1 > ----------------------------------->8----------------------------------- > > To be clearer about the setup, there is a clang (11) installed on this > system. bpftool's build process enables "co-re" feature (Compile-Once, > Run-Everywhere) if the clang compiler is new enough to support > BTF_KIND_VAR [1]. When that happens, bpftool will bootstrap itself: > build/bootstrap/libbpf.a. This bootstrap part is about those *.bpf.c > programs and are only built by invoking clang [2], irrespective of the > compiler used to build the rest of the bpftool. To sum it up, to > reproduce the issue, all you need is having the "co-re" feature > enabled and a kernel that does not provide all the data structures > expected. > > There are changes in bpftool v6.8.0 that assumes the existence of > particular data structures in generated vmlinux.h that is obtained > from the host machine. See [3] for further details. This commit adds > a patch to revert that additional change in v6.8.0. > > There's a patch series pending to be submitted upstream [4]. Hopefully, > those will take care of the problem if they land in the next release. Unfortunately, bpftool has been bumped two times since then and it looks like those patches are still not included... > > [1] Checking for the clang support > https://github.com/libbpf/bpftool/blob/d4469819d1d639ae2354f3d2ddd10377f081a576/src/Makefile.feature#L12 > > [2] Using clang to bootstrap > https://github.com/libbpf/bpftool/blob/d4469819d1d639ae2354f3d2ddd10377f081a576/src/Makefile#L175 > > [3] Question about the problem > https://lore.kernel.org/bpf/c47f732d-dba8-2c13-7c72-3a651bf72353@synopsys.com/t/#u > > [4] Pending patch series > https://lore.kernel.org/bpf/20220421003152.339542-1-alobakin@pm.me/ It's not clear to me why you did a revert of the cookie feature rather than using those patches. I think the first two or three should be sufficient to fix your issue? For the time being, I marked this as Changes Requested. If the upstream patches are not sufficient, please send a mail upstream (as a reply to that series) explaining the problem (again), and re-send this patch to Buildroot. If it does fix things, please send a mail upstream to ask to move the series forward (explaining the issue you have), and send a patch to Buildroot that adds the needed patches. Thanks! Regards, Arnout > > Signed-off-by: Shahab Vahedi <shahab@synopsys.com> > --- > > Changelog: > v1: Initial submission > v2: Use a full fledged git patch for bpftool > v3: Fix the snafu that resulted in malformed patch file > v4: Reword the commit message to make it clearer > > .../0002-revert-bpf_cookie-change.patch | 129 ++++++++++++++++++ > 1 file changed, 129 insertions(+) > create mode 100644 package/bpftool/0002-revert-bpf_cookie-change.patch > > diff --git a/package/bpftool/0002-revert-bpf_cookie-change.patch b/package/bpftool/0002-revert-bpf_cookie-change.patch > new file mode 100644 > index 0000000000..6f9579bd23 > --- /dev/null > +++ b/package/bpftool/0002-revert-bpf_cookie-change.patch > @@ -0,0 +1,129 @@ > +From d7c78d1e38cde73c85b491a833f0e6e3f0d62654 Mon Sep 17 00:00:00 2001 > +From: Shahab Vahedi <shahab@synopsys.com> > +Date: Tue, 14 Jun 2022 10:12:21 +0200 > +Subject: [PATCH] Revert commit "bpftool: Add bpf_cookie to link output" > + > +Building bpftool on a Debian bullseye with clang-11 fails [1]. > +This patch reverts the offending commit [2]. If clang-11 is not > +installed, then the "co-re" feature of bpf will not be enabled > +and the issue remains dormant. > + > +[1] Building release 6.8.0 on Debian 11 > +https://lore.kernel.org/bpf/c47f732d-dba8-2c13-7c72-3a651bf72353@synopsys.com/t/#u > + > +[2] bpftool: Add bpf_cookie to link output > +https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=cbdaf71f > + > +Signed-off-by: Shahab Vahedi <shahab@synopsys.com> > +--- > + src/main.h | 2 -- > + src/pids.c | 8 -------- > + src/skeleton/pid_iter.bpf.c | 22 ---------------------- > + src/skeleton/pid_iter.h | 2 -- > + 4 files changed, 34 deletions(-) > + > +diff --git a/src/main.h b/src/main.h > +index aa99ffa..2f2b638 100644 > +--- a/src/main.h > ++++ b/src/main.h > +@@ -111,9 +111,7 @@ struct obj_ref { > + > + struct obj_refs { > + int ref_cnt; > +- bool has_bpf_cookie; > + struct obj_ref *refs; > +- __u64 bpf_cookie; > + }; > + > + struct btf; > +diff --git a/src/pids.c b/src/pids.c > +index e2d00d3..57f0d1b 100644 > +--- a/src/pids.c > ++++ b/src/pids.c > +@@ -78,8 +78,6 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e) > + ref->pid = e->pid; > + memcpy(ref->comm, e->comm, sizeof(ref->comm)); > + refs->ref_cnt = 1; > +- refs->has_bpf_cookie = e->has_bpf_cookie; > +- refs->bpf_cookie = e->bpf_cookie; > + > + err = hashmap__append(map, u32_as_hash_field(e->id), refs); > + if (err) > +@@ -206,9 +204,6 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id, > + if (refs->ref_cnt == 0) > + break; > + > +- if (refs->has_bpf_cookie) > +- jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie); > +- > + jsonw_name(json_writer, "pids"); > + jsonw_start_array(json_writer); > + for (i = 0; i < refs->ref_cnt; i++) { > +@@ -238,9 +233,6 @@ void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix) > + if (refs->ref_cnt == 0) > + break; > + > +- if (refs->has_bpf_cookie) > +- printf("\n\tbpf_cookie %llu", (unsigned long long) refs->bpf_cookie); > +- > + printf("%s", prefix); > + for (i = 0; i < refs->ref_cnt; i++) { > + struct obj_ref *ref = &refs->refs[i]; > +diff --git a/src/skeleton/pid_iter.bpf.c b/src/skeleton/pid_iter.bpf.c > +index eb05ea5..f70702f 100644 > +--- a/src/skeleton/pid_iter.bpf.c > ++++ b/src/skeleton/pid_iter.bpf.c > +@@ -38,17 +38,6 @@ 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 *perf_link; > +- struct perf_event *event; > +- > +- perf_link = container_of(link, struct bpf_perf_link, link); > +- event = BPF_CORE_READ(perf_link, perf_file, private_data); > +- return BPF_CORE_READ(event, bpf_cookie); > +-} > +- > + SEC("iter/task_file") > + int iter(struct bpf_iter__task_file *ctx) > + { > +@@ -80,19 +69,8 @@ int iter(struct bpf_iter__task_file *ctx) > + if (file->f_op != fops) > + return 0; > + > +- __builtin_memset(&e, 0, sizeof(e)); > + e.pid = task->tgid; > + e.id = get_obj_id(file->private_data, obj_type); > +- > +- if (obj_type == BPF_OBJ_LINK) { > +- struct bpf_link *link = (struct bpf_link *) file->private_data; > +- > +- if (BPF_CORE_READ(link, type) == BPF_LINK_TYPE_PERF_EVENT) { > +- e.has_bpf_cookie = true; > +- e.bpf_cookie = get_bpf_cookie(link); > +- } > +- } > +- > + bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm), > + task->group_leader->comm); > + bpf_seq_write(ctx->meta->seq, &e, sizeof(e)); > +diff --git a/src/skeleton/pid_iter.h b/src/skeleton/pid_iter.h > +index bbb570d..5692cf2 100644 > +--- a/src/skeleton/pid_iter.h > ++++ b/src/skeleton/pid_iter.h > +@@ -6,8 +6,6 @@ > + struct pid_iter_entry { > + __u32 id; > + int pid; > +- __u64 bpf_cookie; > +- bool has_bpf_cookie; > + char comm[16]; > + }; > + > +-- > +2.30.2 > +
Hi Arnout, On 2/8/23 17:36, Arnout Vandecappelle wrote: > > It's not clear to me why you did a revert of the cookie feature rather than > using those patches. I think the first two or three should be sufficient to > fix your issue? Ideally, the correct way would be applying that pending series. But since there were comments that some of the patches might need reworking [1], I did not feel confident to add them. Therefore, I tried to keep my solution minimal and solve it by eliminating the problem. I know, it is not appealing. > For the time being, I marked this as Changes Requested. If the upstream > patches are not sufficient, please send a mail upstream (as a reply to that > series) explaining the problem (again), and re-send this patch to > Buildroot. If it does fix things, please send a mail upstream to ask to > move the series forward (explaining the issue you have), and send a patch > to Buildroot that adds the needed patches. They (re)confirmed the issue to me in the past [2] and are aware that its solution is missing from all the releases onward (see the first point of "Known bugs" in the release page [3]). [1] https://lore.kernel.org/bpf/20220421003152.339542-3-alobakin@pm.me/T/#rb9ce86e9f25b2d097b88d87af8ffdfa401d63c68 [2] https://lore.kernel.org/bpf/b1a604e1-be09-ac0b-ff22-b194ae9ce886@isovalent.com/ [3] https://github.com/libbpf/bpftool/releases -- Shahab
On 09/02/2023 13:23, Shahab Vahedi wrote: > Hi Arnout, > > On 2/8/23 17:36, Arnout Vandecappelle wrote: >> >> It's not clear to me why you did a revert of the cookie feature rather than >> using those patches. I think the first two or three should be sufficient to >> fix your issue? > > Ideally, the correct way would be applying that pending series. But since > there were comments that some of the patches might need reworking [1], I did I don't see anything in that thread about rework being needed, only some complaints about some mail gateway encrypting messages so they can't be read by some recipients... > not feel confident to add them. Therefore, I tried to keep my solution minimal > and solve it by eliminating the problem. I know, it is not appealing. The problem with your patch is that it is very difficult to decide if it should be kept or not when bumping the version. Plus, some people (or other packages) may rely on the feature that you remove. Regards, Arnout > >> For the time being, I marked this as Changes Requested. If the upstream >> patches are not sufficient, please send a mail upstream (as a reply to that >> series) explaining the problem (again), and re-send this patch to >> Buildroot. If it does fix things, please send a mail upstream to ask to >> move the series forward (explaining the issue you have), and send a patch >> to Buildroot that adds the needed patches. > > They (re)confirmed the issue to me in the past [2] and are aware that its > solution is missing from all the releases onward (see the first point of > "Known bugs" in the release page [3]). > > [1] > https://lore.kernel.org/bpf/20220421003152.339542-3-alobakin@pm.me/T/#rb9ce86e9f25b2d097b88d87af8ffdfa401d63c68 > > [2] > https://lore.kernel.org/bpf/b1a604e1-be09-ac0b-ff22-b194ae9ce886@isovalent.com/ > > [3] > https://github.com/libbpf/bpftool/releases > > -- > Shahab > >
On 2/9/23 21:27, Arnout Vandecappelle wrote: > > On 09/02/2023 13:23, Shahab Vahedi wrote: >> >> On 2/8/23 17:36, Arnout Vandecappelle wrote: >>> >>> It's not clear to me why you did a revert of the cookie feature rather than >>> using those patches. I think the first two or three should be sufficient to >>> fix your issue? >> >> Ideally, the correct way would be applying that pending series. But since >> there were comments that some of the patches might need reworking [1], I did > > I don't see anything in that thread about rework being needed, only some > complaints about some mail gateway encrypting messages so they can't be > read by some recipients... See this [1] and the response here [2]. Having read it again, apparently it is not such a big deal. Alexander, the author, mentioned that he will redo that part in v3 of the patch the way David suggested. So either, I can make that change when submitting to Buildroot, or take it as it is. Your call. [1] https://lore.kernel.org/bpf/20220421003152.339542-3-alobakin@pm.me/T/#m42db58567febf12040f6ba0478e1a6148c284c55 [2] https://lore.kernel.org/bpf/20220421003152.339542-3-alobakin@pm.me/T/#m293896646128262621c83e8c15c4d977f92ad0d2 >> not feel confident to add them. Therefore, I tried to keep my solution minimal >> and solve it by eliminating the problem. I know, it is not appealing. > > The problem with your patch is that it is very difficult to decide if it > should be kept or not when bumping the version. Plus, some people (or other > packages) may rely on the feature that you remove. I can try to apply that patch series on top of bpftool-v7.1.0-br1.tar.gz in buildroot. If it can be done fairly easily, I will submit it on a different thread.
On 10/02/2023 10:17, Shahab Vahedi wrote: > On 2/9/23 21:27, Arnout Vandecappelle wrote: >> >> On 09/02/2023 13:23, Shahab Vahedi wrote: >>> >>> On 2/8/23 17:36, Arnout Vandecappelle wrote: >>>> >>>> It's not clear to me why you did a revert of the cookie feature rather than >>>> using those patches. I think the first two or three should be sufficient to >>>> fix your issue? >>> >>> Ideally, the correct way would be applying that pending series. But since >>> there were comments that some of the patches might need reworking [1], I did >> >> I don't see anything in that thread about rework being needed, only some >> complaints about some mail gateway encrypting messages so they can't be >> read by some recipients... > See this [1] and the response here [2]. Having read it again, apparently it is Ah, but I guessed that that patch was not relevant for the issue you are facing. I had the impression only the first two or three patches of the series are relevant. > not such a big deal. Alexander, the author, mentioned that he will redo that > part in v3 of the patch the way David suggested. So either, I can make that > change when submitting to Buildroot, or take it as it is. Your call. > > [1] > https://lore.kernel.org/bpf/20220421003152.339542-3-alobakin@pm.me/T/#m42db58567febf12040f6ba0478e1a6148c284c55 > > [2] > https://lore.kernel.org/bpf/20220421003152.339542-3-alobakin@pm.me/T/#m293896646128262621c83e8c15c4d977f92ad0d2 > >>> not feel confident to add them. Therefore, I tried to keep my solution minimal >>> and solve it by eliminating the problem. I know, it is not appealing. >> >> The problem with your patch is that it is very difficult to decide if it >> should be kept or not when bumping the version. Plus, some people (or other >> packages) may rely on the feature that you remove. > > I can try to apply that patch series on top of bpftool-v7.1.0-br1.tar.gz in > buildroot. If it can be done fairly easily, I will submit it on a different > thread. Thanks! Regards, Arnout
diff --git a/package/bpftool/0002-revert-bpf_cookie-change.patch b/package/bpftool/0002-revert-bpf_cookie-change.patch new file mode 100644 index 0000000000..6f9579bd23 --- /dev/null +++ b/package/bpftool/0002-revert-bpf_cookie-change.patch @@ -0,0 +1,129 @@ +From d7c78d1e38cde73c85b491a833f0e6e3f0d62654 Mon Sep 17 00:00:00 2001 +From: Shahab Vahedi <shahab@synopsys.com> +Date: Tue, 14 Jun 2022 10:12:21 +0200 +Subject: [PATCH] Revert commit "bpftool: Add bpf_cookie to link output" + +Building bpftool on a Debian bullseye with clang-11 fails [1]. +This patch reverts the offending commit [2]. If clang-11 is not +installed, then the "co-re" feature of bpf will not be enabled +and the issue remains dormant. + +[1] Building release 6.8.0 on Debian 11 +https://lore.kernel.org/bpf/c47f732d-dba8-2c13-7c72-3a651bf72353@synopsys.com/t/#u + +[2] bpftool: Add bpf_cookie to link output +https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=cbdaf71f + +Signed-off-by: Shahab Vahedi <shahab@synopsys.com> +--- + src/main.h | 2 -- + src/pids.c | 8 -------- + src/skeleton/pid_iter.bpf.c | 22 ---------------------- + src/skeleton/pid_iter.h | 2 -- + 4 files changed, 34 deletions(-) + +diff --git a/src/main.h b/src/main.h +index aa99ffa..2f2b638 100644 +--- a/src/main.h ++++ b/src/main.h +@@ -111,9 +111,7 @@ struct obj_ref { + + struct obj_refs { + int ref_cnt; +- bool has_bpf_cookie; + struct obj_ref *refs; +- __u64 bpf_cookie; + }; + + struct btf; +diff --git a/src/pids.c b/src/pids.c +index e2d00d3..57f0d1b 100644 +--- a/src/pids.c ++++ b/src/pids.c +@@ -78,8 +78,6 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e) + ref->pid = e->pid; + memcpy(ref->comm, e->comm, sizeof(ref->comm)); + refs->ref_cnt = 1; +- refs->has_bpf_cookie = e->has_bpf_cookie; +- refs->bpf_cookie = e->bpf_cookie; + + err = hashmap__append(map, u32_as_hash_field(e->id), refs); + if (err) +@@ -206,9 +204,6 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id, + if (refs->ref_cnt == 0) + break; + +- if (refs->has_bpf_cookie) +- jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie); +- + jsonw_name(json_writer, "pids"); + jsonw_start_array(json_writer); + for (i = 0; i < refs->ref_cnt; i++) { +@@ -238,9 +233,6 @@ void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix) + if (refs->ref_cnt == 0) + break; + +- if (refs->has_bpf_cookie) +- printf("\n\tbpf_cookie %llu", (unsigned long long) refs->bpf_cookie); +- + printf("%s", prefix); + for (i = 0; i < refs->ref_cnt; i++) { + struct obj_ref *ref = &refs->refs[i]; +diff --git a/src/skeleton/pid_iter.bpf.c b/src/skeleton/pid_iter.bpf.c +index eb05ea5..f70702f 100644 +--- a/src/skeleton/pid_iter.bpf.c ++++ b/src/skeleton/pid_iter.bpf.c +@@ -38,17 +38,6 @@ 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 *perf_link; +- struct perf_event *event; +- +- perf_link = container_of(link, struct bpf_perf_link, link); +- event = BPF_CORE_READ(perf_link, perf_file, private_data); +- return BPF_CORE_READ(event, bpf_cookie); +-} +- + SEC("iter/task_file") + int iter(struct bpf_iter__task_file *ctx) + { +@@ -80,19 +69,8 @@ int iter(struct bpf_iter__task_file *ctx) + if (file->f_op != fops) + return 0; + +- __builtin_memset(&e, 0, sizeof(e)); + e.pid = task->tgid; + e.id = get_obj_id(file->private_data, obj_type); +- +- if (obj_type == BPF_OBJ_LINK) { +- struct bpf_link *link = (struct bpf_link *) file->private_data; +- +- if (BPF_CORE_READ(link, type) == BPF_LINK_TYPE_PERF_EVENT) { +- e.has_bpf_cookie = true; +- e.bpf_cookie = get_bpf_cookie(link); +- } +- } +- + bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm), + task->group_leader->comm); + bpf_seq_write(ctx->meta->seq, &e, sizeof(e)); +diff --git a/src/skeleton/pid_iter.h b/src/skeleton/pid_iter.h +index bbb570d..5692cf2 100644 +--- a/src/skeleton/pid_iter.h ++++ b/src/skeleton/pid_iter.h +@@ -6,8 +6,6 @@ + struct pid_iter_entry { + __u32 id; + int pid; +- __u64 bpf_cookie; +- bool has_bpf_cookie; + char comm[16]; + }; + +-- +2.30.2 +
Building bpftool on Debian 11 (bullseye) with kernel v5.10 fails: -----------------------------------8<----------------------------------- $ make . . . CLANG pid_iter.bpf.o 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); . . . skeleton/pid_iter.bpf.c:49:30: error: no member named 'bpf_cookie' in 'struct perf_event' return BPF_CORE_READ(event, bpf_cookie); . . . 10 errors generated. make: *** [Makefile:176: pid_iter.bpf.o] Error 1 ----------------------------------->8----------------------------------- To be clearer about the setup, there is a clang (11) installed on this system. bpftool's build process enables "co-re" feature (Compile-Once, Run-Everywhere) if the clang compiler is new enough to support BTF_KIND_VAR [1]. When that happens, bpftool will bootstrap itself: build/bootstrap/libbpf.a. This bootstrap part is about those *.bpf.c programs and are only built by invoking clang [2], irrespective of the compiler used to build the rest of the bpftool. To sum it up, to reproduce the issue, all you need is having the "co-re" feature enabled and a kernel that does not provide all the data structures expected. There are changes in bpftool v6.8.0 that assumes the existence of particular data structures in generated vmlinux.h that is obtained from the host machine. See [3] for further details. This commit adds a patch to revert that additional change in v6.8.0. There's a patch series pending to be submitted upstream [4]. Hopefully, those will take care of the problem if they land in the next release. [1] Checking for the clang support https://github.com/libbpf/bpftool/blob/d4469819d1d639ae2354f3d2ddd10377f081a576/src/Makefile.feature#L12 [2] Using clang to bootstrap https://github.com/libbpf/bpftool/blob/d4469819d1d639ae2354f3d2ddd10377f081a576/src/Makefile#L175 [3] Question about the problem https://lore.kernel.org/bpf/c47f732d-dba8-2c13-7c72-3a651bf72353@synopsys.com/t/#u [4] Pending patch series https://lore.kernel.org/bpf/20220421003152.339542-1-alobakin@pm.me/ Signed-off-by: Shahab Vahedi <shahab@synopsys.com> --- Changelog: v1: Initial submission v2: Use a full fledged git patch for bpftool v3: Fix the snafu that resulted in malformed patch file v4: Reword the commit message to make it clearer .../0002-revert-bpf_cookie-change.patch | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 package/bpftool/0002-revert-bpf_cookie-change.patch