Message ID | daa546903aa74cf844452b7f80788d67c15b42ea.1586547735.git.rdna@fb.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | libbpf: Fix loading cgroup_skb/egress with ret in [2, 3] | expand |
On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@fb.com> wrote: > > Initially BPF_CGROUP_INET_EGRESS hook didn't require specifying > expected_attach_type at loading time, but commit > > 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") > > changed it so that expected_attach_type must be specified if program can > return either 2 or 3 (before it was either 0 or 1) to communicate > congestion notification to caller. > > At the same time loading w/o expected_attach_type is still supported for > backward compatibility if program retval is in tnum_range(0, 1). > > Though libbpf currently supports guessing prog/attach/expected_attach > types only for "old" mode (retval in [0; 1]). And if cgroup_skb egress > program stars returning e.g. 2 (corresponds to NET_XMIT_CN), then > guessing breaks and, e.g. bpftool can't load an object with such a > program anymore: > > # bpftool prog loadall tools/testing/selftests/bpf/test_skb.o /sys/fs/bpf/test_skb > libbpf: load bpf program failed: Invalid argument > libbpf: -- BEGIN DUMP LOG --- > libbpf: > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > 0: (85) call pc+5 > > ... skip ... > > from 87 to 1: R0_w=invP2 R10=fp0 > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > 1: (bc) w1 = w0 > 2: (b4) w0 = 1 > 3: (16) if w1 == 0x0 goto pc+1 > 4: (b4) w0 = 2 > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > 5: (95) exit > At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1) > processed 96 insns (limit 1000000) max_states_per_insn 1 total_states 10 peak_states 10 mark_read 2 > > libbpf: -- END LOG -- > libbpf: failed to load program 'cgroup_skb/egress' > libbpf: failed to load object 'tools/testing/selftests/bpf/test_skb.o' > Error: failed to load object file > > Fix it by introducing another entry in libbpf section_defs that makes the load > happens with expected_attach_type: cgroup_skb/egress/expected > > That name may not be ideal, but I don't have a better option. That's a really bad name :) But maybe instead of having another section_def, turn existing section def into the one that does specify expected_attach_type? Seems like kernels accept expected_attach_type for a while now, so it might be ok backwards compatibility-wise? Otherwise, we can teach libbpf to retry program load without expected attach type for cgroup_skb/egress? > > Strictly speaking this is not a fix but rather a missing feature, that's > why there is no Fixes tag. But it still seems to be a good idea to merge > it to stable tree to fix loading programs that use a feature available > for almost a year. > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > --- > tools/lib/bpf/libbpf.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index ff9174282a8c..c909352f894d 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -6330,6 +6330,8 @@ static const struct bpf_sec_def section_defs[] = { > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, > BPF_CGROUP_INET_INGRESS), > + BPF_EAPROG_SEC("cgroup_skb/egress/expected", BPF_PROG_TYPE_CGROUP_SKB, > + BPF_CGROUP_INET_EGRESS), > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, > BPF_CGROUP_INET_EGRESS), > BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB), > -- > 2.24.1 >
On Fri, Apr 10, 2020 at 01:39:03PM -0700, Andrii Nakryiko wrote: > On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Initially BPF_CGROUP_INET_EGRESS hook didn't require specifying > > expected_attach_type at loading time, but commit > > > > 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") > > > > changed it so that expected_attach_type must be specified if program can > > return either 2 or 3 (before it was either 0 or 1) to communicate > > congestion notification to caller. > > > > At the same time loading w/o expected_attach_type is still supported for > > backward compatibility if program retval is in tnum_range(0, 1). > > > > Though libbpf currently supports guessing prog/attach/expected_attach > > types only for "old" mode (retval in [0; 1]). And if cgroup_skb egress > > program stars returning e.g. 2 (corresponds to NET_XMIT_CN), then > > guessing breaks and, e.g. bpftool can't load an object with such a > > program anymore: > > > > # bpftool prog loadall tools/testing/selftests/bpf/test_skb.o /sys/fs/bpf/test_skb > > libbpf: load bpf program failed: Invalid argument > > libbpf: -- BEGIN DUMP LOG --- > > libbpf: > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > 0: (85) call pc+5 > > > > ... skip ... > > > > from 87 to 1: R0_w=invP2 R10=fp0 > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > 1: (bc) w1 = w0 > > 2: (b4) w0 = 1 > > 3: (16) if w1 == 0x0 goto pc+1 > > 4: (b4) w0 = 2 > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > 5: (95) exit > > At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1) > > processed 96 insns (limit 1000000) max_states_per_insn 1 total_states 10 peak_states 10 mark_read 2 > > > > libbpf: -- END LOG -- > > libbpf: failed to load program 'cgroup_skb/egress' > > libbpf: failed to load object 'tools/testing/selftests/bpf/test_skb.o' > > Error: failed to load object file > > > > Fix it by introducing another entry in libbpf section_defs that makes the load > > happens with expected_attach_type: cgroup_skb/egress/expected > > > > That name may not be ideal, but I don't have a better option. > > That's a really bad name :) But maybe instead of having another > section_def, turn existing section def into the one that does specify > expected_attach_type? Seems like kernels accept expected_attach_type > for a while now, so it might be ok backwards compatibility-wise? > Otherwise, we can teach libbpf to retry program load without expected > attach type for cgroup_skb/egress? > > > > > Strictly speaking this is not a fix but rather a missing feature, that's > > why there is no Fixes tag. But it still seems to be a good idea to merge > > it to stable tree to fix loading programs that use a feature available > > for almost a year. > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > --- > > tools/lib/bpf/libbpf.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index ff9174282a8c..c909352f894d 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -6330,6 +6330,8 @@ static const struct bpf_sec_def section_defs[] = { > > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), > > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, > > BPF_CGROUP_INET_INGRESS), > > + BPF_EAPROG_SEC("cgroup_skb/egress/expected", BPF_PROG_TYPE_CGROUP_SKB, > > + BPF_CGROUP_INET_EGRESS), > > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, > > BPF_CGROUP_INET_EGRESS), are you saying that when bpf prog has SEC("cgroup_skb/egress",.. libbpf actually _not_ passing BPF_CGROUP_INET_EGRESS as expected_attach to the kernel? I think it's a libbpf bug and not something to workaround with retries.
On Fri, Apr 10, 2020 at 2:14 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Apr 10, 2020 at 01:39:03PM -0700, Andrii Nakryiko wrote: > > On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > Initially BPF_CGROUP_INET_EGRESS hook didn't require specifying > > > expected_attach_type at loading time, but commit > > > > > > 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") > > > > > > changed it so that expected_attach_type must be specified if program can > > > return either 2 or 3 (before it was either 0 or 1) to communicate > > > congestion notification to caller. > > > > > > At the same time loading w/o expected_attach_type is still supported for > > > backward compatibility if program retval is in tnum_range(0, 1). > > > > > > Though libbpf currently supports guessing prog/attach/expected_attach > > > types only for "old" mode (retval in [0; 1]). And if cgroup_skb egress > > > program stars returning e.g. 2 (corresponds to NET_XMIT_CN), then > > > guessing breaks and, e.g. bpftool can't load an object with such a > > > program anymore: > > > > > > # bpftool prog loadall tools/testing/selftests/bpf/test_skb.o /sys/fs/bpf/test_skb > > > libbpf: load bpf program failed: Invalid argument > > > libbpf: -- BEGIN DUMP LOG --- > > > libbpf: > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > 0: (85) call pc+5 > > > > > > ... skip ... > > > > > > from 87 to 1: R0_w=invP2 R10=fp0 > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > 1: (bc) w1 = w0 > > > 2: (b4) w0 = 1 > > > 3: (16) if w1 == 0x0 goto pc+1 > > > 4: (b4) w0 = 2 > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > 5: (95) exit > > > At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1) > > > processed 96 insns (limit 1000000) max_states_per_insn 1 total_states 10 peak_states 10 mark_read 2 > > > > > > libbpf: -- END LOG -- > > > libbpf: failed to load program 'cgroup_skb/egress' > > > libbpf: failed to load object 'tools/testing/selftests/bpf/test_skb.o' > > > Error: failed to load object file > > > > > > Fix it by introducing another entry in libbpf section_defs that makes the load > > > happens with expected_attach_type: cgroup_skb/egress/expected > > > > > > That name may not be ideal, but I don't have a better option. > > > > That's a really bad name :) But maybe instead of having another > > section_def, turn existing section def into the one that does specify > > expected_attach_type? Seems like kernels accept expected_attach_type > > for a while now, so it might be ok backwards compatibility-wise? > > Otherwise, we can teach libbpf to retry program load without expected > > attach type for cgroup_skb/egress? > > > > > > > > Strictly speaking this is not a fix but rather a missing feature, that's > > > why there is no Fixes tag. But it still seems to be a good idea to merge > > > it to stable tree to fix loading programs that use a feature available > > > for almost a year. > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > --- > > > tools/lib/bpf/libbpf.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > index ff9174282a8c..c909352f894d 100644 > > > --- a/tools/lib/bpf/libbpf.c > > > +++ b/tools/lib/bpf/libbpf.c > > > @@ -6330,6 +6330,8 @@ static const struct bpf_sec_def section_defs[] = { > > > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), > > > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, > > > BPF_CGROUP_INET_INGRESS), > > > + BPF_EAPROG_SEC("cgroup_skb/egress/expected", BPF_PROG_TYPE_CGROUP_SKB, > > > + BPF_CGROUP_INET_EGRESS), > > > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, > > > BPF_CGROUP_INET_EGRESS), > > are you saying that when bpf prog has SEC("cgroup_skb/egress",.. libbpf actually > _not_ passing BPF_CGROUP_INET_EGRESS as expected_attach to the kernel? Yes, that seems to be the difference between BPF_EAPROG_SEC and BPF_APROG_SEC. > I think it's a libbpf bug and not something to workaround with retries. This predates me, but I assume it's a backwards-compatibility move. Because older kernels might know about expected_attach_type, but still allow ingress/egress programs to be attached. I'm fine with dropping that (I actually had to work around this problem in bpf_program__attach_cgroup), but if anyone is feeling strongly about tiny chance of breaking something, we'll have to teach libbpf to retry load without expected_attach_type, if that one fails (which fails in its own way, so I'd rather not do it).
Andrii Nakryiko <andrii.nakryiko@gmail.com> [Fri, 2020-04-10 13:39 -0700]: > On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Initially BPF_CGROUP_INET_EGRESS hook didn't require specifying > > expected_attach_type at loading time, but commit > > > > 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") > > > > changed it so that expected_attach_type must be specified if program can > > return either 2 or 3 (before it was either 0 or 1) to communicate > > congestion notification to caller. > > > > At the same time loading w/o expected_attach_type is still supported for > > backward compatibility if program retval is in tnum_range(0, 1). > > > > Though libbpf currently supports guessing prog/attach/expected_attach > > types only for "old" mode (retval in [0; 1]). And if cgroup_skb egress > > program stars returning e.g. 2 (corresponds to NET_XMIT_CN), then > > guessing breaks and, e.g. bpftool can't load an object with such a > > program anymore: > > > > # bpftool prog loadall tools/testing/selftests/bpf/test_skb.o /sys/fs/bpf/test_skb > > libbpf: load bpf program failed: Invalid argument > > libbpf: -- BEGIN DUMP LOG --- > > libbpf: > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > 0: (85) call pc+5 > > > > ... skip ... > > > > from 87 to 1: R0_w=invP2 R10=fp0 > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > 1: (bc) w1 = w0 > > 2: (b4) w0 = 1 > > 3: (16) if w1 == 0x0 goto pc+1 > > 4: (b4) w0 = 2 > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > 5: (95) exit > > At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1) > > processed 96 insns (limit 1000000) max_states_per_insn 1 total_states 10 peak_states 10 mark_read 2 > > > > libbpf: -- END LOG -- > > libbpf: failed to load program 'cgroup_skb/egress' > > libbpf: failed to load object 'tools/testing/selftests/bpf/test_skb.o' > > Error: failed to load object file > > > > Fix it by introducing another entry in libbpf section_defs that makes the load > > happens with expected_attach_type: cgroup_skb/egress/expected > > > > That name may not be ideal, but I don't have a better option. > > That's a really bad name :) But maybe instead of having another > section_def, turn existing section def into the one that does specify > expected_attach_type? Unfortunately, unless I'm missing something, it'll break loading on older kernels. Specifically before commit 5e43f899b03a ("bpf: Check attach type at prog load time") BPF_PROG_LOAD_LAST_FIELD was prog_ifindex what means that on kernels before that commit all bytes in bpf_attr have to be zero at loading time, otherwise the check in bpf_prog_load: if (CHECK_ATTR(BPF_PROG_LOAD)) return -EINVAL; will fail. If libbpf starts loading with expected_attach_type set on those kernels, that load will fail. That's why I didn't converted existing BPF_APROG_SEC to BPF_EAPROG_SEC. > Seems like kernels accept expected_attach_type > for a while now, so it might be ok backwards compatibility-wise? Sure, that commit is from 2018, but I guess backward compatibility should still be maintained in this case? That's a question to maintainers though. If simply changing BPF_APROG_SEC to BPF_EAPROG_SEC is an option that works for me. > Otherwise, we can teach libbpf to retry program load without expected > attach type for cgroup_skb/egress? Looks like a lot of work compared to simply adding a new section name (or changing existing section if backward compatibility is not a concern here). But that work may work may outweigh inconvenience on user side so no strong preferences. If this is what you were going to do anyway, that may work as well. > > Strictly speaking this is not a fix but rather a missing feature, that's > > why there is no Fixes tag. But it still seems to be a good idea to merge > > it to stable tree to fix loading programs that use a feature available > > for almost a year. > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > --- > > tools/lib/bpf/libbpf.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index ff9174282a8c..c909352f894d 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -6330,6 +6330,8 @@ static const struct bpf_sec_def section_defs[] = { > > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), > > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, > > BPF_CGROUP_INET_INGRESS), > > + BPF_EAPROG_SEC("cgroup_skb/egress/expected", BPF_PROG_TYPE_CGROUP_SKB, > > + BPF_CGROUP_INET_EGRESS), > > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, > > BPF_CGROUP_INET_EGRESS), > > BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB), > > -- > > 2.24.1 > >
Andrii Nakryiko <andrii.nakryiko@gmail.com> [Fri, 2020-04-10 14:52 -0700]: > On Fri, Apr 10, 2020 at 2:14 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Apr 10, 2020 at 01:39:03PM -0700, Andrii Nakryiko wrote: > > > On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > Initially BPF_CGROUP_INET_EGRESS hook didn't require specifying > > > > expected_attach_type at loading time, but commit > > > > > > > > 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") > > > > > > > > changed it so that expected_attach_type must be specified if program can > > > > return either 2 or 3 (before it was either 0 or 1) to communicate > > > > congestion notification to caller. > > > > > > > > At the same time loading w/o expected_attach_type is still supported for > > > > backward compatibility if program retval is in tnum_range(0, 1). > > > > > > > > Though libbpf currently supports guessing prog/attach/expected_attach > > > > types only for "old" mode (retval in [0; 1]). And if cgroup_skb egress > > > > program stars returning e.g. 2 (corresponds to NET_XMIT_CN), then > > > > guessing breaks and, e.g. bpftool can't load an object with such a > > > > program anymore: > > > > > > > > # bpftool prog loadall tools/testing/selftests/bpf/test_skb.o /sys/fs/bpf/test_skb > > > > libbpf: load bpf program failed: Invalid argument > > > > libbpf: -- BEGIN DUMP LOG --- > > > > libbpf: > > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > > 0: (85) call pc+5 > > > > > > > > ... skip ... > > > > > > > > from 87 to 1: R0_w=invP2 R10=fp0 > > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > > 1: (bc) w1 = w0 > > > > 2: (b4) w0 = 1 > > > > 3: (16) if w1 == 0x0 goto pc+1 > > > > 4: (b4) w0 = 2 > > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > > 5: (95) exit > > > > At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1) > > > > processed 96 insns (limit 1000000) max_states_per_insn 1 total_states 10 peak_states 10 mark_read 2 > > > > > > > > libbpf: -- END LOG -- > > > > libbpf: failed to load program 'cgroup_skb/egress' > > > > libbpf: failed to load object 'tools/testing/selftests/bpf/test_skb.o' > > > > Error: failed to load object file > > > > > > > > Fix it by introducing another entry in libbpf section_defs that makes the load > > > > happens with expected_attach_type: cgroup_skb/egress/expected > > > > > > > > That name may not be ideal, but I don't have a better option. > > > > > > That's a really bad name :) But maybe instead of having another > > > section_def, turn existing section def into the one that does specify > > > expected_attach_type? Seems like kernels accept expected_attach_type > > > for a while now, so it might be ok backwards compatibility-wise? > > > Otherwise, we can teach libbpf to retry program load without expected > > > attach type for cgroup_skb/egress? > > > > > > > > > > > Strictly speaking this is not a fix but rather a missing feature, that's > > > > why there is no Fixes tag. But it still seems to be a good idea to merge > > > > it to stable tree to fix loading programs that use a feature available > > > > for almost a year. > > > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > > --- > > > > tools/lib/bpf/libbpf.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > > index ff9174282a8c..c909352f894d 100644 > > > > --- a/tools/lib/bpf/libbpf.c > > > > +++ b/tools/lib/bpf/libbpf.c > > > > @@ -6330,6 +6330,8 @@ static const struct bpf_sec_def section_defs[] = { > > > > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), > > > > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, > > > > BPF_CGROUP_INET_INGRESS), > > > > + BPF_EAPROG_SEC("cgroup_skb/egress/expected", BPF_PROG_TYPE_CGROUP_SKB, > > > > + BPF_CGROUP_INET_EGRESS), > > > > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, > > > > BPF_CGROUP_INET_EGRESS), > > > > are you saying that when bpf prog has SEC("cgroup_skb/egress",.. libbpf actually > > _not_ passing BPF_CGROUP_INET_EGRESS as expected_attach to the kernel? > > Yes, that seems to be the difference between BPF_EAPROG_SEC and BPF_APROG_SEC. Yeah, "EA" version adds expected_attach_type ("E" stands for "expected") at load time and "A" version only specifies attach type to use at attach time (stands for "attach"). > > > I think it's a libbpf bug and not something to workaround with retries. > > This predates me, but I assume it's a backwards-compatibility move. > Because older kernels might know about expected_attach_type, but still > allow ingress/egress programs to be attached. I'm fine with dropping Only egress. ingress doesn't have that problem. > that (I actually had to work around this problem in > bpf_program__attach_cgroup), but if anyone is feeling strongly about > tiny chance of breaking something, we'll have to teach libbpf to retry > load without expected_attach_type, if that one fails (which fails in > its own way, so I'd rather not do it). Answered backward compatibility point in the previous e-mail.
On Fri, Apr 10, 2020 at 3:57 PM Andrey Ignatov <rdna@fb.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Fri, 2020-04-10 13:39 -0700]: > > On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > Initially BPF_CGROUP_INET_EGRESS hook didn't require specifying > > > expected_attach_type at loading time, but commit > > > > > > 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") > > > > > > changed it so that expected_attach_type must be specified if program can > > > return either 2 or 3 (before it was either 0 or 1) to communicate > > > congestion notification to caller. > > > > > > At the same time loading w/o expected_attach_type is still supported for > > > backward compatibility if program retval is in tnum_range(0, 1). > > > > > > Though libbpf currently supports guessing prog/attach/expected_attach > > > types only for "old" mode (retval in [0; 1]). And if cgroup_skb egress > > > program stars returning e.g. 2 (corresponds to NET_XMIT_CN), then > > > guessing breaks and, e.g. bpftool can't load an object with such a > > > program anymore: > > > > > > # bpftool prog loadall tools/testing/selftests/bpf/test_skb.o /sys/fs/bpf/test_skb > > > libbpf: load bpf program failed: Invalid argument > > > libbpf: -- BEGIN DUMP LOG --- > > > libbpf: > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > 0: (85) call pc+5 > > > > > > ... skip ... > > > > > > from 87 to 1: R0_w=invP2 R10=fp0 > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > 1: (bc) w1 = w0 > > > 2: (b4) w0 = 1 > > > 3: (16) if w1 == 0x0 goto pc+1 > > > 4: (b4) w0 = 2 > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > 5: (95) exit > > > At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1) > > > processed 96 insns (limit 1000000) max_states_per_insn 1 total_states 10 peak_states 10 mark_read 2 > > > > > > libbpf: -- END LOG -- > > > libbpf: failed to load program 'cgroup_skb/egress' > > > libbpf: failed to load object 'tools/testing/selftests/bpf/test_skb.o' > > > Error: failed to load object file > > > > > > Fix it by introducing another entry in libbpf section_defs that makes the load > > > happens with expected_attach_type: cgroup_skb/egress/expected > > > > > > That name may not be ideal, but I don't have a better option. > > > > That's a really bad name :) But maybe instead of having another > > section_def, turn existing section def into the one that does specify > > expected_attach_type? > > Unfortunately, unless I'm missing something, it'll break loading on > older kernels. > > Specifically before commit 5e43f899b03a ("bpf: Check attach type at prog > load time") BPF_PROG_LOAD_LAST_FIELD was prog_ifindex what means that on > kernels before that commit all bytes in bpf_attr have to be zero at > loading time, otherwise the check in bpf_prog_load: > > if (CHECK_ATTR(BPF_PROG_LOAD)) > return -EINVAL; > > will fail. If libbpf starts loading with expected_attach_type set on > those kernels, that load will fail. > > That's why I didn't converted existing BPF_APROG_SEC to BPF_EAPROG_SEC. I understand all that :) Seems like 4.10 through 4.16 will be affected. On the other hand, for them the easy work-around would be bpf_program__set_expected_attach_type(prog, BPF_CGROUP_INET_INGRESS), so not the end of the world... But a new section definition just for this is the worst option out of three possible ones, IMO. > > > Seems like kernels accept expected_attach_type > > for a while now, so it might be ok backwards compatibility-wise? > > Sure, that commit is from 2018, but I guess backward compatibility > should still be maintained in this case? That's a question to > maintainers though. If simply changing BPF_APROG_SEC to BPF_EAPROG_SEC > is an option that works for me. > > > > Otherwise, we can teach libbpf to retry program load without expected > > attach type for cgroup_skb/egress? > > Looks like a lot of work compared to simply adding a new section name > (or changing existing section if backward compatibility is not a concern > here). > > But that work may work may outweigh inconvenience on user side so no > strong preferences. If this is what you were going to do anyway, that > may work as well. Usability trumps extra code in libbpf :) > > > > > Strictly speaking this is not a fix but rather a missing feature, that's > > > why there is no Fixes tag. But it still seems to be a good idea to merge > > > it to stable tree to fix loading programs that use a feature available > > > for almost a year. > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > --- > > > tools/lib/bpf/libbpf.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > index ff9174282a8c..c909352f894d 100644 > > > --- a/tools/lib/bpf/libbpf.c > > > +++ b/tools/lib/bpf/libbpf.c > > > @@ -6330,6 +6330,8 @@ static const struct bpf_sec_def section_defs[] = { > > > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), > > > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, > > > BPF_CGROUP_INET_INGRESS), > > > + BPF_EAPROG_SEC("cgroup_skb/egress/expected", BPF_PROG_TYPE_CGROUP_SKB, > > > + BPF_CGROUP_INET_EGRESS), > > > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, > > > BPF_CGROUP_INET_EGRESS), > > > BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB), > > > -- > > > 2.24.1 > > > > > -- > Andrey Ignatov
On Fri, Apr 10, 2020 at 03:57:39PM -0700, Andrey Ignatov wrote: > > > Seems like kernels accept expected_attach_type > > for a while now, so it might be ok backwards compatibility-wise? > > Sure, that commit is from 2018, but I guess backward compatibility > should still be maintained in this case? That's a question to > maintainers though. If simply changing BPF_APROG_SEC to BPF_EAPROG_SEC > is an option that works for me. > > > > Otherwise, we can teach libbpf to retry program load without expected > > attach type for cgroup_skb/egress? > > Looks like a lot of work compared to simply adding a new section name > (or changing existing section if backward compatibility is not a concern > here). I still don't understand that backward compatiblity concern. Fixing libbpf to do BPF_EAPROG_SEC("cgroup_skb/egress" will make egress progs to fail at load time if they use > 1 return value on old kernels and fail at load time for > 3 return value on new kernels. Without libbpf fix such progs would rely on old and new kernels internal implementation details. Since on the latest kernel with current libbpf behavior the egress prog will get loaded as ingress type with return value 4 and then gets attached at egress. Argh. One really need to deep dive into kernel sources to figure out what kernel will do with such return value. Such behavior is undefined and broken. Did I misunderstand the whole issue?
Alexei Starovoitov <alexei.starovoitov@gmail.com> [Sat, 2020-04-11 15:42 -0700]: > On Fri, Apr 10, 2020 at 03:57:39PM -0700, Andrey Ignatov wrote: > > > > > Seems like kernels accept expected_attach_type > > > for a while now, so it might be ok backwards compatibility-wise? > > > > Sure, that commit is from 2018, but I guess backward compatibility > > should still be maintained in this case? That's a question to > > maintainers though. If simply changing BPF_APROG_SEC to BPF_EAPROG_SEC > > is an option that works for me. > > > > > > > Otherwise, we can teach libbpf to retry program load without expected > > > attach type for cgroup_skb/egress? > > > > Looks like a lot of work compared to simply adding a new section name > > (or changing existing section if backward compatibility is not a concern > > here). > > I still don't understand that backward compatiblity concern. > Fixing libbpf to do BPF_EAPROG_SEC("cgroup_skb/egress" > will make egress progs to fail at load time if they use > 1 return value on old > kernels No. Changing BPF_APROG_SEC to BPF_EAPROG_SEC will fail loading programs on old kernels with any return value, incl. 0 and 1. That's the point. > and fail at load time for > 3 return value on new kernels. Without > libbpf fix such progs would rely on old and new kernels internal implementation > details. Since on the latest kernel with current libbpf behavior the egress > prog will get loaded as ingress type with return value 4 and then gets attached > at egress. Argh. One really need to deep dive into kernel sources to figure out > what kernel will do with such return value. Such behavior is undefined and broken. > Did I misunderstand the whole issue? Let me try to explain with an example. I patched libbpf and built bpftool with this patch: 17:00:43 0 rdna@dev082.prn2:~/bpf-next$>git di tools/lib/bpf/ diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index ff9174282a8c..cc4d5b74e64a 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -6330,7 +6330,7 @@ static const struct bpf_sec_def section_defs[] = { BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_INGRESS), - BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, + BPF_EAPROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_EGRESS), BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB), BPF_APROG_SEC("cgroup/sock", BPF_PROG_TYPE_CGROUP_SOCK, Wrote a simple cgroup_skb/egress program that returns 1 and built it: 17:00:59 0 rdna@dev082.prn2:~/bpf-next$>cat tools/testing/selftests/bpf/progs/skb_ret1.c #include <linux/bpf.h> #include <bpf/bpf_helpers.h> int _version SEC("version") = 1; char _license[] SEC("license") = "GPL"; SEC("cgroup_skb/egress") int skb_ret1(struct __sk_buff *skb) { return 1; } Made sure that it can be loaded on new kernel: 17:00:39 0 rdna@dev082.prn2:~/bpf-next$>uname -srm Linux 5.2.9-93_fbk11_rc1_3610_g6a108f4f4a2b x86_64 17:01:10 0 rdna@dev082.prn2:~/bpf-next$>sudo ./tools/bpf/bpftool/bpftool p load tools/testing/selftests/bpf/skb_ret1.o /sys/fs/bpf/skb_ret1 17:01:25 0 rdna@dev082.prn2:~/bpf-next$>sudo ./tools/bpf/bpftool/bpftool p l pinned /sys/fs/bpf/skb_ret1 87347: cgroup_skb name skb_ret1 tag 9bf8e2a8bee581f5 gpl loaded_at 2020-04-11T17:01:25-0700 uid 0 xlated 16B jited 40B memlock 4096B btf_id 4952 Then copied both bpftool and the program to a server with old kernel (that doesn't have 5e43f899b03a ("bpf: Check attach type at prog load time")) and tried same thing (note "./bpftool"): [root@<some_host> ~]# uname -srm Linux 4.11.3-45_fbk11_3602_gd67c71c x86_64 [root@<some_host> ~]# ./bpftool p load ./skb_ret1.o /sys/fs/bpf/skb_ret1 libbpf: Error loading BTF: Invalid argument(22) libbpf: Error loading .BTF into kernel: -22. libbpf: load bpf program failed: Invalid argument libbpf: failed to load program 'cgroup_skb/egress' libbpf: failed to load object './skb_ret1.o' Error: failed to load object file [root@<some_host> ~]# echo $? 255 [root@<some_host> ~]# ./bpftool p l pinned /sys/fs/bpf/skb_ret1 Error: bpf obj get (/sys/fs/bpf/skb_ret1): No such file or directory As you can see it fails to load (BTF errors are not relevant). Then I tried to load same program on same old kernel but with prod bpftool (w/o my patch) just to make sure BTF is not a problem and it loads fine: [root@<some_host> ~]# bpftool p load ./skb_ret1.o /sys/fs/bpf/skb_ret1 libbpf: Error loading BTF: Invalid argument(22) libbpf: Error loading .BTF into kernel: -22. [root@<some_host> ~]# echo $? 0 [root@<some_host> ~]# bpftool p l pinned /sys/fs/bpf/skb_ret1 23944: cgroup_skb name skb_ret1 tag 9bf8e2a8bee581f5 loaded_at 2020-04-11T17:12:07-0700 uid 0 xlated 16B jited 64B memlock 4096B [root@<some_host> ~]# That's expected becase the error at loading time happens long before running verifier and doesn't have anything to do with what program returns. It fails on `if (CHECK_ATTR(BPF_PROG_LOAD))` in kernel's bpf_prog_load() -- the very first check that happens in that function. Old kernel checks that passed by user-space bpf_attr has all bytes after bpf_attr.prog_ifindex (the BPF_PROG_LOAD_LAST_FIELD known to that kernel) zero, but finds that there is non-zero unknown field, that is expected_attach_type, and fails. So changing BPF_APROG_SEC to BPF_EAPROG_SEC will break loading cgroup skb egress progs on any kernel before 5e43f899b03a ("bpf: Check attach type at prog load time"), no matter what those programs do. That's why I think it's not a good idea. Does it clarify?
On Fri, Apr 10, 2020 at 5:09 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Apr 10, 2020 at 3:57 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Fri, 2020-04-10 13:39 -0700]: > > > On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > Initially BPF_CGROUP_INET_EGRESS hook didn't require specifying > > > > expected_attach_type at loading time, but commit > > > > > > > > 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") > > > > > > > > changed it so that expected_attach_type must be specified if program can > > > > return either 2 or 3 (before it was either 0 or 1) to communicate > > > > congestion notification to caller. > > > > > > > > At the same time loading w/o expected_attach_type is still supported for > > > > backward compatibility if program retval is in tnum_range(0, 1). > > > > > > > > Though libbpf currently supports guessing prog/attach/expected_attach > > > > types only for "old" mode (retval in [0; 1]). And if cgroup_skb egress > > > > program stars returning e.g. 2 (corresponds to NET_XMIT_CN), then > > > > guessing breaks and, e.g. bpftool can't load an object with such a > > > > program anymore: > > > > > > > > # bpftool prog loadall tools/testing/selftests/bpf/test_skb.o /sys/fs/bpf/test_skb > > > > libbpf: load bpf program failed: Invalid argument > > > > libbpf: -- BEGIN DUMP LOG --- > > > > libbpf: > > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > > 0: (85) call pc+5 > > > > > > > > ... skip ... > > > > > > > > from 87 to 1: R0_w=invP2 R10=fp0 > > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > > 1: (bc) w1 = w0 > > > > 2: (b4) w0 = 1 > > > > 3: (16) if w1 == 0x0 goto pc+1 > > > > 4: (b4) w0 = 2 > > > > ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; > > > > 5: (95) exit > > > > At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1) > > > > processed 96 insns (limit 1000000) max_states_per_insn 1 total_states 10 peak_states 10 mark_read 2 > > > > > > > > libbpf: -- END LOG -- > > > > libbpf: failed to load program 'cgroup_skb/egress' > > > > libbpf: failed to load object 'tools/testing/selftests/bpf/test_skb.o' > > > > Error: failed to load object file > > > > > > > > Fix it by introducing another entry in libbpf section_defs that makes the load > > > > happens with expected_attach_type: cgroup_skb/egress/expected > > > > > > > > That name may not be ideal, but I don't have a better option. > > > > > > That's a really bad name :) But maybe instead of having another > > > section_def, turn existing section def into the one that does specify > > > expected_attach_type? > > > > Unfortunately, unless I'm missing something, it'll break loading on > > older kernels. > > > > Specifically before commit 5e43f899b03a ("bpf: Check attach type at prog > > load time") BPF_PROG_LOAD_LAST_FIELD was prog_ifindex what means that on > > kernels before that commit all bytes in bpf_attr have to be zero at > > loading time, otherwise the check in bpf_prog_load: > > > > if (CHECK_ATTR(BPF_PROG_LOAD)) > > return -EINVAL; > > > > will fail. If libbpf starts loading with expected_attach_type set on > > those kernels, that load will fail. > > > > That's why I didn't converted existing BPF_APROG_SEC to BPF_EAPROG_SEC. > > I understand all that :) Seems like 4.10 through 4.16 will be affected. > > On the other hand, for them the easy work-around would be > bpf_program__set_expected_attach_type(prog, BPF_CGROUP_INET_INGRESS), > so not the end of the world... But a new section definition just for > this is the worst option out of three possible ones, IMO. > > > > > > Seems like kernels accept expected_attach_type > > > for a while now, so it might be ok backwards compatibility-wise? > > > > Sure, that commit is from 2018, but I guess backward compatibility > > should still be maintained in this case? That's a question to > > maintainers though. If simply changing BPF_APROG_SEC to BPF_EAPROG_SEC > > is an option that works for me. > > > > > > > Otherwise, we can teach libbpf to retry program load without expected > > > attach type for cgroup_skb/egress? > > > > Looks like a lot of work compared to simply adding a new section name > > (or changing existing section if backward compatibility is not a concern > > here). > > > > But that work may work may outweigh inconvenience on user side so no > > strong preferences. If this is what you were going to do anyway, that > > may work as well. > > Usability trumps extra code in libbpf :) [0] should fix this issue without requiring unnecessary new section definitions. Please take a look and let me know if that won't work for some reason. [0] https://patchwork.ozlabs.org/patch/1269400/ > > > > > > > > > Strictly speaking this is not a fix but rather a missing feature, that's > > > > why there is no Fixes tag. But it still seems to be a good idea to merge > > > > it to stable tree to fix loading programs that use a feature available > > > > for almost a year. > > > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > > --- > > > > tools/lib/bpf/libbpf.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > > index ff9174282a8c..c909352f894d 100644 > > > > --- a/tools/lib/bpf/libbpf.c > > > > +++ b/tools/lib/bpf/libbpf.c > > > > @@ -6330,6 +6330,8 @@ static const struct bpf_sec_def section_defs[] = { > > > > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), > > > > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, > > > > BPF_CGROUP_INET_INGRESS), > > > > + BPF_EAPROG_SEC("cgroup_skb/egress/expected", BPF_PROG_TYPE_CGROUP_SKB, > > > > + BPF_CGROUP_INET_EGRESS), > > > > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, > > > > BPF_CGROUP_INET_EGRESS), > > > > BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB), > > > > -- > > > > 2.24.1 > > > > > > > > -- > > Andrey Ignatov
Andrii Nakryiko <andrii.nakryiko@gmail.com> [Sat, 2020-04-11 23:01 -0700]: > On Fri, Apr 10, 2020 at 5:09 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Fri, Apr 10, 2020 at 3:57 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Fri, 2020-04-10 13:39 -0700]: > > > > On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@fb.com> wrote: ... > > > > Otherwise, we can teach libbpf to retry program load without expected > > > > attach type for cgroup_skb/egress? > > > > > > Looks like a lot of work compared to simply adding a new section name > > > (or changing existing section if backward compatibility is not a concern > > > here). > > > > > > But that work may work may outweigh inconvenience on user side so no > > > strong preferences. If this is what you were going to do anyway, that > > > may work as well. > > > > Usability trumps extra code in libbpf :) > > [0] should fix this issue without requiring unnecessary new section > definitions. Please take a look and let me know if that won't work for > some reason. > > [0] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1269400_&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=3jAokpHyGuCuJ834j-tttQ&m=gwtRFCwg1r2VaTv-GpXKX0e2c-HWZADFO3Ikynunse0&s=eFectzkso2WgfqSeWStlhCk3cEKaJ0_kjcnXhJ8tAFU&e= Thanks Andrii. This looks like a good option to me. I left a few comments on the implementation.
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index ff9174282a8c..c909352f894d 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -6330,6 +6330,8 @@ static const struct bpf_sec_def section_defs[] = { BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL), BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_INGRESS), + BPF_EAPROG_SEC("cgroup_skb/egress/expected", BPF_PROG_TYPE_CGROUP_SKB, + BPF_CGROUP_INET_EGRESS), BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_EGRESS), BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB),
Initially BPF_CGROUP_INET_EGRESS hook didn't require specifying expected_attach_type at loading time, but commit 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") changed it so that expected_attach_type must be specified if program can return either 2 or 3 (before it was either 0 or 1) to communicate congestion notification to caller. At the same time loading w/o expected_attach_type is still supported for backward compatibility if program retval is in tnum_range(0, 1). Though libbpf currently supports guessing prog/attach/expected_attach types only for "old" mode (retval in [0; 1]). And if cgroup_skb egress program stars returning e.g. 2 (corresponds to NET_XMIT_CN), then guessing breaks and, e.g. bpftool can't load an object with such a program anymore: # bpftool prog loadall tools/testing/selftests/bpf/test_skb.o /sys/fs/bpf/test_skb libbpf: load bpf program failed: Invalid argument libbpf: -- BEGIN DUMP LOG --- libbpf: ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; 0: (85) call pc+5 ... skip ... from 87 to 1: R0_w=invP2 R10=fp0 ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; 1: (bc) w1 = w0 2: (b4) w0 = 1 3: (16) if w1 == 0x0 goto pc+1 4: (b4) w0 = 2 ; return tc_prog(skb) == TC_ACT_OK ? 1 : 2 /* NET_XMIT_CN */; 5: (95) exit At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1) processed 96 insns (limit 1000000) max_states_per_insn 1 total_states 10 peak_states 10 mark_read 2 libbpf: -- END LOG -- libbpf: failed to load program 'cgroup_skb/egress' libbpf: failed to load object 'tools/testing/selftests/bpf/test_skb.o' Error: failed to load object file Fix it by introducing another entry in libbpf section_defs that makes the load happens with expected_attach_type: cgroup_skb/egress/expected That name may not be ideal, but I don't have a better option. Strictly speaking this is not a fix but rather a missing feature, that's why there is no Fixes tag. But it still seems to be a good idea to merge it to stable tree to fix loading programs that use a feature available for almost a year. Signed-off-by: Andrey Ignatov <rdna@fb.com> --- tools/lib/bpf/libbpf.c | 2 ++ 1 file changed, 2 insertions(+)