[PATCH bpf 0/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[PATCH bpf 0/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrey Ignatov]

This patch set fixes loading cgroup_skb egress programs that return values
2 or 3 and adds selftest for newly added section name.

See patch 1 for details on the fix.


Andrey Ignatov (2):
  libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]
  selftests/bpf: Test cgroup_skb/egress/expected section name

 tools/lib/bpf/libbpf.c                                 | 2 ++
 tools/testing/selftests/bpf/prog_tests/section_names.c | 5 +++++
 2 files changed, 7 insertions(+)

-- 
2.24.1

[PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrey Ignatov]

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(+)

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

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrii Nakryiko]

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
>

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Alexei Starovoitov]

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.

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrii Nakryiko]

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).

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrey Ignatov]

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.

-- 
Andrey Ignatov

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrey Ignatov]

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

-- 
Andrey Ignatov

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrii Nakryiko]

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

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrii Nakryiko]

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

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[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.

-- 
Andrey Ignatov

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Alexei Starovoitov]

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?

Re: [PATCH bpf 1/2] libbpf: Fix loading cgroup_skb/egress with ret in [2, 3]

[Andrey Ignatov]

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?

-- 
Andrey Ignatov

[PATCH bpf 2/2] selftests/bpf: Test cgroup_skb/egress/expected section name

[Andrey Ignatov]

Add selftests for cgroup_skb/egress/expected:
  # ./test_progs --name=section_names
  #44 section_names:OK
  Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 tools/testing/selftests/bpf/prog_tests/section_names.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/section_names.c b/tools/testing/selftests/bpf/prog_tests/section_names.c
index 9d9351dc2ded..2928ca93f7a5 100644
--- a/tools/testing/selftests/bpf/prog_tests/section_names.c
+++ b/tools/testing/selftests/bpf/prog_tests/section_names.c
@@ -51,6 +51,11 @@ static struct sec_name_test tests[] = {
 		{0, BPF_PROG_TYPE_CGROUP_SKB, 0},
 		{0, BPF_CGROUP_INET_EGRESS},
 	},
+	{
+		"cgroup_skb/egress/expected",
+		{0, BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_EGRESS},
+		{0, BPF_CGROUP_INET_EGRESS},
+	},
 	{"cgroup/skb", {0, BPF_PROG_TYPE_CGROUP_SKB, 0}, {-EINVAL, 0} },
 	{
 		"cgroup/sock",
-- 
2.24.1