Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [Mon, 2020-04-13 21:49 -0700]:
> On Mon, Apr 13, 2020 at 3:44 PM Andrey Ignatov <rdna@xxxxxx> wrote:
> >
> > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [Mon, 2020-04-13 15:00 -0700]:
> > > On Mon, Apr 13, 2020 at 1:21 PM Andrey Ignatov <rdna@xxxxxx> wrote:
> > > >
> > > > Andrii Nakryiko <andriin@xxxxxx> [Sat, 2020-04-11 22:58 -0700]:
...
> > > >
> > > > But I don't have context on all hooks that can be affected by this
> > > > change and could easily miss something.
> > > >
> > > > Ideally it should be verified by tests. Current section_names.c test
> > > > only verifies what will be returned, but AFAIK there is no test that
> > > > checks whether provided combination of prog_type/expected_attach_type at
> > > > load time and attach_type at attach time would actually work both on
> > > > current and old kernels. Do you think it's possible to add such a
> > > > selftest? (current libbpf CI supports running on old kernels, doesn't
> > > > it?)
> > >
> > > So all the existing selftests are essentially verifying this, if run
> > > on old kernel. I don't think libbpf currently runs tests on such old
> > > kernels, though. But there is no extra selftest that we need to add,
> > > because every single existing one will execute this piece of libbpf
> > > logic.
> >
> > Apparently existing tests didn't catch the very obvious bug with
> > BPF_PROG_TYPE_CGROUP_SOCK / BPF_CGROUP_INET_EGRESS invalid combination.
>
> Sigh.. yeah. I expected cgroup_link test to fail if that functionality
> didn't work, but I missed that bpf_program__attach_cgroup() code will
> use correct expected_attach_type, even if it's not provided to
> BPF_PROG_LOAD.
>
> >
> > I think it'd be useful to start with at least basic test focused on
> > expected_attach_type. Then later extend it to new attach types when they're
> > being added and, ideally, to existing ones.
>
> How this test should look like? I can make a test that will work only
> on new kernel (e.g., by using cgroup program which needs
> expected_attach_type), but it will fail on old kernels. There doesn't
> seem to be a way to query expected_attach_type from kernel. Any hints
> on how to make test that will pass on old and new kernels and will
> validate expected_attach_type is passed properly?
I think there should be two steps here:
1) make a test;
2) make the test work on old kernels;
The "1)" should be pretty straightforward: we can just have an object
with all possible section names and make sure it can be loaded. If
a program type can have different scenarios, IMO all scenarios should be
covered.
For example, part of the object for cgroup_skb can look like this:
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
char _license[] SEC("license") = "GPL";
int _version SEC("version") = 0;
SEC("cgroup_skb/ingress")
int skb_ret1(struct __sk_buff *skb)
{
return 1;
}
/* Support for ret > 1 has different expectations for expected_attach_type */
SEC("cgroup_skb/ingress")
int skb_ret1(struct __sk_buff *skb)
{
return 2;
}
SEC("cgroup_skb/egress")
int skb_ret1(struct __sk_buff *skb)
{
return 1;
}
/* Support for ret > 1 has different expectations for expected_attach_type */
SEC("cgroup_skb/egress")
int skb_ret1(struct __sk_buff *skb)
{
return 2;
}
/* Compat section name */
SEC("cgroup/skb")
int skb_ret1(struct __sk_buff *skb)
{
return 1;
}
/* ... and then other sections .. */
Some time later attach step can be added according to what kind of
program it is (e.g. try to attach cgroup programs to a cgroup, etc).
IMO it'd be beneficial for libbpf to have such a simple/single test that
verifies the very basic thing: simple program for every supported
section name can be loaded.
And such a test would caught the initial problem with NET_XMIT_CN.
I checked whether all sections have at least one program in selftests
and found a bunch that don't:
09:43:11 0 rdna@dev082.prn2:~/bpf-next$>sed -ne '/static const struct bpf_sec_def section_defs/,/^\};/p' tools/lib/bpf/libbpf.c | awk -F'"' 'NF == 3 {printf "SEC(\"%s\n", $2}' | sort > all_sec_names
09:43:19 0 rdna@dev082.prn2:~/bpf-next$>head -n 5 all_sec_names
SEC("action
SEC("cgroup/bind4
SEC("cgroup/bind6
SEC("cgroup/connect4
SEC("cgroup/connect6
09:43:20 0 rdna@dev082.prn2:~/bpf-next$>diff -u all_sec_names <(git grep -ohf all_sec_names tools/testing/selftests/bpf/ | sort -u)
--- all_sec_names 2020-04-14 09:43:19.552675629 -0700
+++ /dev/fd/63 2020-04-14 09:43:30.967648496 -0700
@@ -1,21 +1,13 @@
-SEC("action
-SEC("cgroup/bind4
-SEC("cgroup/bind6
SEC("cgroup/connect4
SEC("cgroup/connect6
SEC("cgroup/dev
SEC("cgroup/getsockopt
-SEC("cgroup/post_bind4
-SEC("cgroup/post_bind6
-SEC("cgroup/recvmsg4
-SEC("cgroup/recvmsg6
SEC("cgroup/sendmsg4
SEC("cgroup/sendmsg6
SEC("cgroup/setsockopt
SEC("cgroup/skb
SEC("cgroup_skb/egress
SEC("cgroup_skb/ingress
-SEC("cgroup/sock
SEC("cgroup/sysctl
SEC("classifier
SEC("fentry/
@@ -27,10 +19,7 @@
SEC("kretprobe/
SEC("lirc_mode2
SEC("lsm/
-SEC("lwt_in
-SEC("lwt_out
SEC("lwt_seg6local
-SEC("lwt_xmit
SEC("perf_event
SEC("raw_tp/
SEC("raw_tracepoint/
That simple test can provide coverage for such sections.
As for "2)" -- I agree, it's not that straightforward: there should be a
way to check for feature presence in the kernel and skip if feature is
not present. AFAIK currently there is no such thing in bpf
selftests(?). IMO it's fine to postpone this step for later time.
What do you think?
--
Andrey Ignatov
