On Fri, Apr 10, 2020 at 5:09 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Apr 10, 2020 at 3:57 PM Andrey Ignatov <rdna@xxxxxx> wrote:
> >
> > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [Fri, 2020-04-10 13:39 -0700]:
> > > On Fri, Apr 10, 2020 at 12:54 PM Andrey Ignatov <rdna@xxxxxx> 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@xxxxxx>
> > > > ---
> > > > 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
