Date:   Tue, 14 Apr 2020 10:24:09 -0700
From:   Andrey Ignatov <rdna@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next] libbpf: always specify expected_attach_type on
 program load if supported

Andrii Nakryiko <andrii.nakryiko@...il.com> [Mon, 2020-04-13 21:49 -0700]:
> On Mon, Apr 13, 2020 at 3:44 PM Andrey Ignatov <rdna@...com> wrote:
> >
> > Andrii Nakryiko <andrii.nakryiko@...il.com> [Mon, 2020-04-13 15:00 -0700]:
> > > On Mon, Apr 13, 2020 at 1:21 PM Andrey Ignatov <rdna@...com> wrote:
> > > >
> > > > Andrii Nakryiko <andriin@...com> [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@...082.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@...082.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@...082.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

Powered by blists - more mailing lists