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? 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@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
>
