diff mbox series

[bpf-next,v3,3/3] libbpf: add selftests for TC-BPF API

Message ID 20210420193740.124285-4-memxor@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Add TC-BPF API | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 2 maintainers not CCed: linux-kselftest@vger.kernel.org shuah@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Kumar Kartikeya Dwivedi April 20, 2021, 7:37 p.m. UTC 
This adds some basic tests for the low level bpf_tc_* API.

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/test_tc_bpf.c    | 169 ++++++++++++++++++
 .../selftests/bpf/progs/test_tc_bpf_kern.c    |  12 ++
 2 files changed, 181 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c

Comments

Yonghong Song April 21, 2021, 7:01 a.m. UTC | #1 
On 4/20/21 12:37 PM, Kumar Kartikeya Dwivedi wrote:
> This adds some basic tests for the low level bpf_tc_* API.
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>   .../selftests/bpf/prog_tests/test_tc_bpf.c    | 169 ++++++++++++++++++
>   .../selftests/bpf/progs/test_tc_bpf_kern.c    |  12 ++
>   2 files changed, 181 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
> new file mode 100644
> index 000000000000..563a3944553c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <linux/err.h>
> +#include <linux/limits.h>
> +#include <bpf/libbpf.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <test_progs.h>
> +#include <linux/if_ether.h>
> +
> +#define LO_IFINDEX 1
> +
[...]
> +
> +void test_test_tc_bpf(void)
> +{
> +	const char *file = "./test_tc_bpf_kern.o";
> +	struct bpf_program *clsp;
> +	struct bpf_object *obj;
> +	int cls_fd, ret;
> +
> +	obj = bpf_object__open(file);
> +	if (!ASSERT_OK_PTR(obj, "bpf_object__open"))
> +		return;
> +
> +	clsp = bpf_object__find_program_by_title(obj, "classifier");
> +	if (!ASSERT_OK_PTR(clsp, "bpf_object__find_program_by_title"))
> +		goto end;

Please use bpf_object__find_program_by_name() API.

> +
> +	ret = bpf_object__load(obj);
> +	if (!ASSERT_EQ(ret, 0, "bpf_object__load"))
> +		goto end;
> +
> +	cls_fd = bpf_program__fd(clsp);
> +
> +	system("tc qdisc del dev lo clsact");
> +
> +	ret = test_tc_internal(cls_fd, BPF_TC_CLSACT_INGRESS);
> +	if (!ASSERT_EQ(ret, 0, "test_tc_internal INGRESS"))
> +		goto end;
> +
> +	if (!ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> +		       "clsact qdisc delete failed"))
> +		goto end;
> +
> +	ret = test_tc_info(cls_fd);
> +	if (!ASSERT_EQ(ret, 0, "test_tc_info"))
> +		goto end;
> +
> +	if (!ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> +		       "clsact qdisc delete failed"))
> +		goto end;
> +
> +	ret = test_tc_internal(cls_fd, BPF_TC_CLSACT_EGRESS);
> +	if (!ASSERT_EQ(ret, 0, "test_tc_internal EGRESS"))
> +		goto end;
> +
> +	ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> +		  "clsact qdisc delete failed");
> +
> +end:
> +	bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
> new file mode 100644
> index 000000000000..18a3a7ed924a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +/* Dummy prog to test TC-BPF API */
> +
> +SEC("classifier")
> +int cls(struct __sk_buff *skb)
> +{
> +	return 0;
> +}
>
Andrii Nakryiko April 21, 2021, 6:24 p.m. UTC | #2 
On Tue, Apr 20, 2021 at 12:37 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> This adds some basic tests for the low level bpf_tc_* API.
>
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/test_tc_bpf.c    | 169 ++++++++++++++++++
>  .../selftests/bpf/progs/test_tc_bpf_kern.c    |  12 ++
>  2 files changed, 181 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c

we normally don't call prog_test's files with "test_" prefix, it can
be just tc_bpf.c (or just tc.c)

>  create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c

we also don't typically call BPF source code files with _kern suffix,
just test_tc_bpf.c would be more in line with most common case

>
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
> new file mode 100644
> index 000000000000..563a3944553c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <linux/err.h>
> +#include <linux/limits.h>
> +#include <bpf/libbpf.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <test_progs.h>
> +#include <linux/if_ether.h>
> +
> +#define LO_IFINDEX 1
> +
> +static int test_tc_internal(int fd, __u32 parent_id)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
> +                           .class_id = TC_H_MAKE(1UL << 16, 1));
> +       struct bpf_tc_attach_id id = {};
> +       struct bpf_tc_info info = {};
> +       int ret;
> +
> +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
> +               return ret;
> +
> +       ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
> +               goto end;
> +
> +       if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
> +           !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
> +           !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
> +           !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
> +           !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
> +           !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
> +                      "class_id incorrect") ||
> +           !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
> +               goto end;
> +
> +       opts.replace = true;
> +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach in replace mode"))
> +               return ret;
> +
> +       /* Demonstrate changing attributes */
> +       opts.class_id = TC_H_MAKE(1UL << 16, 2);
> +
> +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc attach in replace mode"))
> +               goto end;
> +
> +       ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
> +               goto end;
> +
> +       if (!ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 2),
> +                      "class_id incorrect after replace"))
> +               goto end;
> +       if (!ASSERT_EQ(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT, 1,
> +                      "direct action mode not set"))
> +               goto end;
> +
> +end:
> +       ret = bpf_tc_detach(LO_IFINDEX, parent_id, &id);
> +       ASSERT_EQ(ret, 0, "detach failed");
> +       return ret;
> +}
> +
> +int test_tc_info(int fd)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
> +                           .class_id = TC_H_MAKE(1UL << 16, 1));
> +       struct bpf_tc_attach_id id = {}, old;
> +       struct bpf_tc_info info = {};
> +       int ret;
> +
> +       ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
> +               return ret;
> +       old = id;
> +
> +       ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
> +               goto end_old;
> +
> +       if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
> +           !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
> +           !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
> +           !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
> +           !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
> +           !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
> +                      "class_id incorrect") ||
> +           !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
> +               goto end_old;
> +
> +       /* choose a priority */
> +       opts.priority = 0;
> +       ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
> +               goto end_old;
> +
> +       ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
> +               goto end;
> +
> +       if (!ASSERT_NEQ(id.priority, old.priority, "filter priority mismatch"))
> +               goto end;
> +       if (!ASSERT_EQ(info.id.priority, id.priority, "priority mismatch"))
> +               goto end;
> +
> +end:
> +       ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id);
> +       ASSERT_EQ(ret, 0, "detach failed");
> +end_old:
> +       ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &old);
> +       ASSERT_EQ(ret, 0, "detach failed");
> +       return ret;
> +}
> +
> +void test_test_tc_bpf(void)

test_test_ tautology, drop one test?

> +{
> +       const char *file = "./test_tc_bpf_kern.o";

please use BPF skeleton instead, see lots of other selftests doing
that already. You won't even need find_program_by_{name,title}, among
other things.

> +       struct bpf_program *clsp;
> +       struct bpf_object *obj;
> +       int cls_fd, ret;
> +
> +       obj = bpf_object__open(file);
> +       if (!ASSERT_OK_PTR(obj, "bpf_object__open"))
> +               return;
> +
> +       clsp = bpf_object__find_program_by_title(obj, "classifier");
> +       if (!ASSERT_OK_PTR(clsp, "bpf_object__find_program_by_title"))
> +               goto end;
> +
> +       ret = bpf_object__load(obj);
> +       if (!ASSERT_EQ(ret, 0, "bpf_object__load"))
> +               goto end;
> +
> +       cls_fd = bpf_program__fd(clsp);
> +
> +       system("tc qdisc del dev lo clsact");

can this fail? also why is this necessary? it's still not possible to
do anything with only libbpf APIs?

> +
> +       ret = test_tc_internal(cls_fd, BPF_TC_CLSACT_INGRESS);
> +       if (!ASSERT_EQ(ret, 0, "test_tc_internal INGRESS"))
> +               goto end;
> +
> +       if (!ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> +                      "clsact qdisc delete failed"))
> +               goto end;
> +
> +       ret = test_tc_info(cls_fd);
> +       if (!ASSERT_EQ(ret, 0, "test_tc_info"))
> +               goto end;
> +
> +       if (!ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> +                      "clsact qdisc delete failed"))
> +               goto end;
> +
> +       ret = test_tc_internal(cls_fd, BPF_TC_CLSACT_EGRESS);
> +       if (!ASSERT_EQ(ret, 0, "test_tc_internal EGRESS"))
> +               goto end;
> +
> +       ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> +                 "clsact qdisc delete failed");
> +
> +end:
> +       bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
> new file mode 100644
> index 000000000000..18a3a7ed924a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +/* Dummy prog to test TC-BPF API */
> +
> +SEC("classifier")
> +int cls(struct __sk_buff *skb)
> +{
> +       return 0;
> +}
> --
> 2.30.2
>
Kumar Kartikeya Dwivedi April 21, 2021, 7:56 p.m. UTC | #3 
On Wed, Apr 21, 2021 at 11:54:18PM IST, Andrii Nakryiko wrote:
> On Tue, Apr 20, 2021 at 12:37 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > This adds some basic tests for the low level bpf_tc_* API.
> >
> > Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  .../selftests/bpf/prog_tests/test_tc_bpf.c    | 169 ++++++++++++++++++
> >  .../selftests/bpf/progs/test_tc_bpf_kern.c    |  12 ++
> >  2 files changed, 181 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
>
> we normally don't call prog_test's files with "test_" prefix, it can
> be just tc_bpf.c (or just tc.c)
>

Ok, will rename.

> >  create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
>
> we also don't typically call BPF source code files with _kern suffix,
> just test_tc_bpf.c would be more in line with most common case
>

Will rename.

> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
> > new file mode 100644
> > index 000000000000..563a3944553c
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
> > @@ -0,0 +1,169 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bpf.h>
> > +#include <linux/err.h>
> > +#include <linux/limits.h>
> > +#include <bpf/libbpf.h>
> > +#include <errno.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <test_progs.h>
> > +#include <linux/if_ether.h>
> > +
> > +#define LO_IFINDEX 1
> > +
> > +static int test_tc_internal(int fd, __u32 parent_id)
> > +{
> > +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
> > +                           .class_id = TC_H_MAKE(1UL << 16, 1));
> > +       struct bpf_tc_attach_id id = {};
> > +       struct bpf_tc_info info = {};
> > +       int ret;
> > +
> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
> > +               return ret;
> > +
> > +       ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
> > +               goto end;
> > +
> > +       if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
> > +           !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
> > +           !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
> > +           !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
> > +           !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
> > +           !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
> > +                      "class_id incorrect") ||
> > +           !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
> > +               goto end;
> > +
> > +       opts.replace = true;
> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach in replace mode"))
> > +               return ret;
> > +
> > +       /* Demonstrate changing attributes */
> > +       opts.class_id = TC_H_MAKE(1UL << 16, 2);
> > +
> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc attach in replace mode"))
> > +               goto end;
> > +
> > +       ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
> > +               goto end;
> > +
> > +       if (!ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 2),
> > +                      "class_id incorrect after replace"))
> > +               goto end;
> > +       if (!ASSERT_EQ(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT, 1,
> > +                      "direct action mode not set"))
> > +               goto end;
> > +
> > +end:
> > +       ret = bpf_tc_detach(LO_IFINDEX, parent_id, &id);
> > +       ASSERT_EQ(ret, 0, "detach failed");
> > +       return ret;
> > +}
> > +
> > +int test_tc_info(int fd)
> > +{
> > +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
> > +                           .class_id = TC_H_MAKE(1UL << 16, 1));
> > +       struct bpf_tc_attach_id id = {}, old;
> > +       struct bpf_tc_info info = {};
> > +       int ret;
> > +
> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
> > +               return ret;
> > +       old = id;
> > +
> > +       ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
> > +               goto end_old;
> > +
> > +       if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
> > +           !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
> > +           !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
> > +           !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
> > +           !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
> > +           !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
> > +                      "class_id incorrect") ||
> > +           !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
> > +               goto end_old;
> > +
> > +       /* choose a priority */
> > +       opts.priority = 0;
> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
> > +               goto end_old;
> > +
> > +       ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
> > +               goto end;
> > +
> > +       if (!ASSERT_NEQ(id.priority, old.priority, "filter priority mismatch"))
> > +               goto end;
> > +       if (!ASSERT_EQ(info.id.priority, id.priority, "priority mismatch"))
> > +               goto end;
> > +
> > +end:
> > +       ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id);
> > +       ASSERT_EQ(ret, 0, "detach failed");
> > +end_old:
> > +       ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &old);
> > +       ASSERT_EQ(ret, 0, "detach failed");
> > +       return ret;
> > +}
> > +
> > +void test_test_tc_bpf(void)
>
> test_test_ tautology, drop one test?
>

Ok.

> > +{
> > +       const char *file = "./test_tc_bpf_kern.o";
>
> please use BPF skeleton instead, see lots of other selftests doing
> that already. You won't even need find_program_by_{name,title}, among
> other things.
>

Sounds good, will change.

> > +       struct bpf_program *clsp;
> > +       struct bpf_object *obj;
> > +       int cls_fd, ret;
> > +
> > +       obj = bpf_object__open(file);
> > +       if (!ASSERT_OK_PTR(obj, "bpf_object__open"))
> > +               return;
> > +
> > +       clsp = bpf_object__find_program_by_title(obj, "classifier");
> > +       if (!ASSERT_OK_PTR(clsp, "bpf_object__find_program_by_title"))
> > +               goto end;
> > +
> > +       ret = bpf_object__load(obj);
> > +       if (!ASSERT_EQ(ret, 0, "bpf_object__load"))
> > +               goto end;
> > +
> > +       cls_fd = bpf_program__fd(clsp);
> > +
> > +       system("tc qdisc del dev lo clsact");
>
> can this fail? also why is this necessary? it's still not possible to

This is just removing any existing clsact qdisc since it will be setup by the
attach call, which is again removed later (where we do check if it fails, if it
does clsact qdisc was not setup, and something was wrong in that it returned 0
when the attach point was one of the clsact hooks).

We don't care about failure initially, since if it isn't present we'd just move
on to running the test.

> do anything with only libbpf APIs?
>

I don't think so, I can do the qdisc teardown using netlink in the selftest,
but that would mean duplicating a lot of code. I think expecting tc to be
present on the system is a reasonable assumption for this test.

> > +
> > +       ret = test_tc_internal(cls_fd, BPF_TC_CLSACT_INGRESS);
> > +       if (!ASSERT_EQ(ret, 0, "test_tc_internal INGRESS"))
> > +               goto end;
> > +
> > +       if (!ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> > +                      "clsact qdisc delete failed"))
> > +               goto end;
> > +
> > +       ret = test_tc_info(cls_fd);
> > +       if (!ASSERT_EQ(ret, 0, "test_tc_info"))
> > +               goto end;
> > +
> > +       if (!ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> > +                      "clsact qdisc delete failed"))
> > +               goto end;
> > +
> > +       ret = test_tc_internal(cls_fd, BPF_TC_CLSACT_EGRESS);
> > +       if (!ASSERT_EQ(ret, 0, "test_tc_internal EGRESS"))
> > +               goto end;
> > +
> > +       ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
> > +                 "clsact qdisc delete failed");
> > +
> > +end:
> > +       bpf_object__close(obj);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
> > new file mode 100644
> > index 000000000000..18a3a7ed924a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
> > @@ -0,0 +1,12 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +
> > +/* Dummy prog to test TC-BPF API */
> > +
> > +SEC("classifier")
> > +int cls(struct __sk_buff *skb)
> > +{
> > +       return 0;
> > +}
> > --
> > 2.30.2
> >

--
Kartikeya
Toke Høiland-Jørgensen April 21, 2021, 8:38 p.m. UTC | #4 
Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:

> On Wed, Apr 21, 2021 at 11:54:18PM IST, Andrii Nakryiko wrote:
>> On Tue, Apr 20, 2021 at 12:37 PM Kumar Kartikeya Dwivedi
>> <memxor@gmail.com> wrote:
>> >
>> > This adds some basic tests for the low level bpf_tc_* API.
>> >
>> > Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> > ---
>> >  .../selftests/bpf/prog_tests/test_tc_bpf.c    | 169 ++++++++++++++++++
>> >  .../selftests/bpf/progs/test_tc_bpf_kern.c    |  12 ++
>> >  2 files changed, 181 insertions(+)
>> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
>>
>> we normally don't call prog_test's files with "test_" prefix, it can
>> be just tc_bpf.c (or just tc.c)
>>
>
> Ok, will rename.
>
>> >  create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
>>
>> we also don't typically call BPF source code files with _kern suffix,
>> just test_tc_bpf.c would be more in line with most common case
>>
>
> Will rename.
>
>> >
>> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
>> > new file mode 100644
>> > index 000000000000..563a3944553c
>> > --- /dev/null
>> > +++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
>> > @@ -0,0 +1,169 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +
>> > +#include <linux/bpf.h>
>> > +#include <linux/err.h>
>> > +#include <linux/limits.h>
>> > +#include <bpf/libbpf.h>
>> > +#include <errno.h>
>> > +#include <stdio.h>
>> > +#include <stdlib.h>
>> > +#include <test_progs.h>
>> > +#include <linux/if_ether.h>
>> > +
>> > +#define LO_IFINDEX 1
>> > +
>> > +static int test_tc_internal(int fd, __u32 parent_id)
>> > +{
>> > +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
>> > +                           .class_id = TC_H_MAKE(1UL << 16, 1));
>> > +       struct bpf_tc_attach_id id = {};
>> > +       struct bpf_tc_info info = {};
>> > +       int ret;
>> > +
>> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
>> > +               return ret;
>> > +
>> > +       ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
>> > +               goto end;
>> > +
>> > +       if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
>> > +           !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
>> > +           !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
>> > +           !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
>> > +           !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
>> > +           !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
>> > +                      "class_id incorrect") ||
>> > +           !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
>> > +               goto end;
>> > +
>> > +       opts.replace = true;
>> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach in replace mode"))
>> > +               return ret;
>> > +
>> > +       /* Demonstrate changing attributes */
>> > +       opts.class_id = TC_H_MAKE(1UL << 16, 2);
>> > +
>> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc attach in replace mode"))
>> > +               goto end;
>> > +
>> > +       ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
>> > +               goto end;
>> > +
>> > +       if (!ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 2),
>> > +                      "class_id incorrect after replace"))
>> > +               goto end;
>> > +       if (!ASSERT_EQ(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT, 1,
>> > +                      "direct action mode not set"))
>> > +               goto end;
>> > +
>> > +end:
>> > +       ret = bpf_tc_detach(LO_IFINDEX, parent_id, &id);
>> > +       ASSERT_EQ(ret, 0, "detach failed");
>> > +       return ret;
>> > +}
>> > +
>> > +int test_tc_info(int fd)
>> > +{
>> > +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
>> > +                           .class_id = TC_H_MAKE(1UL << 16, 1));
>> > +       struct bpf_tc_attach_id id = {}, old;
>> > +       struct bpf_tc_info info = {};
>> > +       int ret;
>> > +
>> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
>> > +               return ret;
>> > +       old = id;
>> > +
>> > +       ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
>> > +               goto end_old;
>> > +
>> > +       if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
>> > +           !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
>> > +           !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
>> > +           !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
>> > +           !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
>> > +           !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
>> > +                      "class_id incorrect") ||
>> > +           !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
>> > +               goto end_old;
>> > +
>> > +       /* choose a priority */
>> > +       opts.priority = 0;
>> > +       ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
>> > +               goto end_old;
>> > +
>> > +       ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
>> > +               goto end;
>> > +
>> > +       if (!ASSERT_NEQ(id.priority, old.priority, "filter priority mismatch"))
>> > +               goto end;
>> > +       if (!ASSERT_EQ(info.id.priority, id.priority, "priority mismatch"))
>> > +               goto end;
>> > +
>> > +end:
>> > +       ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id);
>> > +       ASSERT_EQ(ret, 0, "detach failed");
>> > +end_old:
>> > +       ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &old);
>> > +       ASSERT_EQ(ret, 0, "detach failed");
>> > +       return ret;
>> > +}
>> > +
>> > +void test_test_tc_bpf(void)
>>
>> test_test_ tautology, drop one test?
>>
>
> Ok.
>
>> > +{
>> > +       const char *file = "./test_tc_bpf_kern.o";
>>
>> please use BPF skeleton instead, see lots of other selftests doing
>> that already. You won't even need find_program_by_{name,title}, among
>> other things.
>>
>
> Sounds good, will change.
>
>> > +       struct bpf_program *clsp;
>> > +       struct bpf_object *obj;
>> > +       int cls_fd, ret;
>> > +
>> > +       obj = bpf_object__open(file);
>> > +       if (!ASSERT_OK_PTR(obj, "bpf_object__open"))
>> > +               return;
>> > +
>> > +       clsp = bpf_object__find_program_by_title(obj, "classifier");
>> > +       if (!ASSERT_OK_PTR(clsp, "bpf_object__find_program_by_title"))
>> > +               goto end;
>> > +
>> > +       ret = bpf_object__load(obj);
>> > +       if (!ASSERT_EQ(ret, 0, "bpf_object__load"))
>> > +               goto end;
>> > +
>> > +       cls_fd = bpf_program__fd(clsp);
>> > +
>> > +       system("tc qdisc del dev lo clsact");
>>
>> can this fail? also why is this necessary? it's still not possible to
>
> This is just removing any existing clsact qdisc since it will be setup by the
> attach call, which is again removed later (where we do check if it fails, if it
> does clsact qdisc was not setup, and something was wrong in that it returned 0
> when the attach point was one of the clsact hooks).
>
> We don't care about failure initially, since if it isn't present we'd just move
> on to running the test.
>
>> do anything with only libbpf APIs?
>>
>
> I don't think so, I can do the qdisc teardown using netlink in the selftest,
> but that would mean duplicating a lot of code. I think expecting tc to be
> present on the system is a reasonable assumption for this test.

So this stems from the fact that bpf_tc_detach() doesn't clean up the
clsact qdisc that is added by bpf_tc_attach(). I think we should fix
this.

Andrii, Kumar and I discussed this, and concluded that the best we can
do from userspace right now is query the number of filters before remove
and if there's only one, also remove the clsact qdisc. This is racy in
that a new filter can be attached between the check and the remove, but
to fix that we need a way for the filter to take the ref on the qdisc.
Since something like this will be needed for a bpf_link attach mode as
well, we figured that can be added as part of such a series, and we'll
just do the best-effort thing now. WDYT?

-Toke
Daniel Borkmann April 21, 2021, 10:41 p.m. UTC | #5 
On 4/21/21 10:38 PM, Toke Høiland-Jørgensen wrote:
> Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:
> 
>> On Wed, Apr 21, 2021 at 11:54:18PM IST, Andrii Nakryiko wrote:
>>> On Tue, Apr 20, 2021 at 12:37 PM Kumar Kartikeya Dwivedi
>>> <memxor@gmail.com> wrote:
>>>>
>>>> This adds some basic tests for the low level bpf_tc_* API.
>>>>
>>>> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>>>> ---
>>>>   .../selftests/bpf/prog_tests/test_tc_bpf.c    | 169 ++++++++++++++++++
>>>>   .../selftests/bpf/progs/test_tc_bpf_kern.c    |  12 ++
>>>>   2 files changed, 181 insertions(+)
>>>>   create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
>>>
>>> we normally don't call prog_test's files with "test_" prefix, it can
>>> be just tc_bpf.c (or just tc.c)
>>>
>>
>> Ok, will rename.
>>
>>>>   create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
>>>
>>> we also don't typically call BPF source code files with _kern suffix,
>>> just test_tc_bpf.c would be more in line with most common case
>>>
>>
>> Will rename.
>>
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
>>>> new file mode 100644
>>>> index 000000000000..563a3944553c
>>>> --- /dev/null
>>>> +++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
>>>> @@ -0,0 +1,169 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +#include <linux/bpf.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/limits.h>
>>>> +#include <bpf/libbpf.h>
>>>> +#include <errno.h>
>>>> +#include <stdio.h>
>>>> +#include <stdlib.h>
>>>> +#include <test_progs.h>
>>>> +#include <linux/if_ether.h>
>>>> +
>>>> +#define LO_IFINDEX 1
>>>> +
>>>> +static int test_tc_internal(int fd, __u32 parent_id)
>>>> +{
>>>> +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
>>>> +                           .class_id = TC_H_MAKE(1UL << 16, 1));
>>>> +       struct bpf_tc_attach_id id = {};
>>>> +       struct bpf_tc_info info = {};
>>>> +       int ret;
>>>> +
>>>> +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
>>>> +               return ret;
>>>> +
>>>> +       ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
>>>> +               goto end;
>>>> +
>>>> +       if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
>>>> +           !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
>>>> +           !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
>>>> +           !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
>>>> +           !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
>>>> +           !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
>>>> +                      "class_id incorrect") ||
>>>> +           !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
>>>> +               goto end;
>>>> +
>>>> +       opts.replace = true;
>>>> +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach in replace mode"))
>>>> +               return ret;
>>>> +
>>>> +       /* Demonstrate changing attributes */
>>>> +       opts.class_id = TC_H_MAKE(1UL << 16, 2);
>>>> +
>>>> +       ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc attach in replace mode"))
>>>> +               goto end;
>>>> +
>>>> +       ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
>>>> +               goto end;
>>>> +
>>>> +       if (!ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 2),
>>>> +                      "class_id incorrect after replace"))
>>>> +               goto end;
>>>> +       if (!ASSERT_EQ(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT, 1,
>>>> +                      "direct action mode not set"))
>>>> +               goto end;
>>>> +
>>>> +end:
>>>> +       ret = bpf_tc_detach(LO_IFINDEX, parent_id, &id);
>>>> +       ASSERT_EQ(ret, 0, "detach failed");
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +int test_tc_info(int fd)
>>>> +{
>>>> +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
>>>> +                           .class_id = TC_H_MAKE(1UL << 16, 1));
>>>> +       struct bpf_tc_attach_id id = {}, old;
>>>> +       struct bpf_tc_info info = {};
>>>> +       int ret;
>>>> +
>>>> +       ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
>>>> +               return ret;
>>>> +       old = id;
>>>> +
>>>> +       ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
>>>> +               goto end_old;
>>>> +
>>>> +       if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
>>>> +           !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
>>>> +           !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
>>>> +           !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
>>>> +           !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
>>>> +           !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
>>>> +                      "class_id incorrect") ||
>>>> +           !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
>>>> +               goto end_old;
>>>> +
>>>> +       /* choose a priority */
>>>> +       opts.priority = 0;
>>>> +       ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
>>>> +               goto end_old;
>>>> +
>>>> +       ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
>>>> +               goto end;
>>>> +
>>>> +       if (!ASSERT_NEQ(id.priority, old.priority, "filter priority mismatch"))
>>>> +               goto end;
>>>> +       if (!ASSERT_EQ(info.id.priority, id.priority, "priority mismatch"))
>>>> +               goto end;
>>>> +
>>>> +end:
>>>> +       ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id);
>>>> +       ASSERT_EQ(ret, 0, "detach failed");
>>>> +end_old:
>>>> +       ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &old);
>>>> +       ASSERT_EQ(ret, 0, "detach failed");
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +void test_test_tc_bpf(void)
>>>
>>> test_test_ tautology, drop one test?
>>>
>>
>> Ok.
>>
>>>> +{
>>>> +       const char *file = "./test_tc_bpf_kern.o";
>>>
>>> please use BPF skeleton instead, see lots of other selftests doing
>>> that already. You won't even need find_program_by_{name,title}, among
>>> other things.
>>>
>>
>> Sounds good, will change.
>>
>>>> +       struct bpf_program *clsp;
>>>> +       struct bpf_object *obj;
>>>> +       int cls_fd, ret;
>>>> +
>>>> +       obj = bpf_object__open(file);
>>>> +       if (!ASSERT_OK_PTR(obj, "bpf_object__open"))
>>>> +               return;
>>>> +
>>>> +       clsp = bpf_object__find_program_by_title(obj, "classifier");
>>>> +       if (!ASSERT_OK_PTR(clsp, "bpf_object__find_program_by_title"))
>>>> +               goto end;
>>>> +
>>>> +       ret = bpf_object__load(obj);
>>>> +       if (!ASSERT_EQ(ret, 0, "bpf_object__load"))
>>>> +               goto end;
>>>> +
>>>> +       cls_fd = bpf_program__fd(clsp);
>>>> +
>>>> +       system("tc qdisc del dev lo clsact");
>>>
>>> can this fail? also why is this necessary? it's still not possible to
>>
>> This is just removing any existing clsact qdisc since it will be setup by the
>> attach call, which is again removed later (where we do check if it fails, if it
>> does clsact qdisc was not setup, and something was wrong in that it returned 0
>> when the attach point was one of the clsact hooks).
>>
>> We don't care about failure initially, since if it isn't present we'd just move
>> on to running the test.
>>
>>> do anything with only libbpf APIs?
>>
>> I don't think so, I can do the qdisc teardown using netlink in the selftest,
>> but that would mean duplicating a lot of code. I think expecting tc to be
>> present on the system is a reasonable assumption for this test.
> 
> So this stems from the fact that bpf_tc_detach() doesn't clean up the
> clsact qdisc that is added by bpf_tc_attach(). I think we should fix
> this.

I was wondering whether it would make sense to add a bpf_tc_ctx_init() and
bpf_tc_ctx_destroy() API which would auto-create the sch_clsact qdisc, plus
provide a 'handle' for bpf_tc_attach() and bpf_tc_detach(), and for the other
one, it would delete the qdisc. Otoh, if an empty sch_clsact obj is sitting
around while not being great (given minor effect on fast-path), it also doesn't
harm /overly/ much. Maybe a /poor/ analogy could be that if you open a v6 socket,
it pulls in the ipv6 module, but also doesn't remove it when you close() it.
Anyway, but for the test itself, given you can define prio etc, I don't think
it would even need the system() call?

Thanks,
Daniel
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
new file mode 100644
index 000000000000..563a3944553c
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
@@ -0,0 +1,169 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <linux/err.h>
+#include <linux/limits.h>
+#include <bpf/libbpf.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <test_progs.h>
+#include <linux/if_ether.h>
+
+#define LO_IFINDEX 1
+
+static int test_tc_internal(int fd, __u32 parent_id)
+{
+	DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
+			    .class_id = TC_H_MAKE(1UL << 16, 1));
+	struct bpf_tc_attach_id id = {};
+	struct bpf_tc_info info = {};
+	int ret;
+
+	ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
+		return ret;
+
+	ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
+		goto end;
+
+	if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
+	    !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
+	    !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
+	    !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
+	    !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
+	    !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
+		       "class_id incorrect") ||
+	    !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
+		goto end;
+
+	opts.replace = true;
+	ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc_attach in replace mode"))
+		return ret;
+
+	/* Demonstrate changing attributes */
+	opts.class_id = TC_H_MAKE(1UL << 16, 2);
+
+	ret = bpf_tc_attach(fd, LO_IFINDEX, parent_id, &opts, &id);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc attach in replace mode"))
+		goto end;
+
+	ret = bpf_tc_get_info(LO_IFINDEX, parent_id, &id, &info);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
+		goto end;
+
+	if (!ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 2),
+		       "class_id incorrect after replace"))
+		goto end;
+	if (!ASSERT_EQ(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT, 1,
+		       "direct action mode not set"))
+		goto end;
+
+end:
+	ret = bpf_tc_detach(LO_IFINDEX, parent_id, &id);
+	ASSERT_EQ(ret, 0, "detach failed");
+	return ret;
+}
+
+int test_tc_info(int fd)
+{
+	DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 10,
+			    .class_id = TC_H_MAKE(1UL << 16, 1));
+	struct bpf_tc_attach_id id = {}, old;
+	struct bpf_tc_info info = {};
+	int ret;
+
+	ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
+		return ret;
+	old = id;
+
+	ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
+		goto end_old;
+
+	if (!ASSERT_EQ(info.id.handle, id.handle, "handle mismatch") ||
+	    !ASSERT_EQ(info.id.priority, id.priority, "priority mismatch") ||
+	    !ASSERT_EQ(info.id.handle, 1, "handle incorrect") ||
+	    !ASSERT_EQ(info.chain_index, 0, "chain_index incorrect") ||
+	    !ASSERT_EQ(info.id.priority, 10, "priority incorrect") ||
+	    !ASSERT_EQ(info.class_id, TC_H_MAKE(1UL << 16, 1),
+		       "class_id incorrect") ||
+	    !ASSERT_EQ(info.protocol, ETH_P_ALL, "protocol incorrect"))
+		goto end_old;
+
+	/* choose a priority */
+	opts.priority = 0;
+	ret = bpf_tc_attach(fd, LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &opts, &id);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc_attach"))
+		goto end_old;
+
+	ret = bpf_tc_get_info(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id, &info);
+	if (!ASSERT_EQ(ret, 0, "bpf_tc_get_info"))
+		goto end;
+
+	if (!ASSERT_NEQ(id.priority, old.priority, "filter priority mismatch"))
+		goto end;
+	if (!ASSERT_EQ(info.id.priority, id.priority, "priority mismatch"))
+		goto end;
+
+end:
+	ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &id);
+	ASSERT_EQ(ret, 0, "detach failed");
+end_old:
+	ret = bpf_tc_detach(LO_IFINDEX, BPF_TC_CLSACT_INGRESS, &old);
+	ASSERT_EQ(ret, 0, "detach failed");
+	return ret;
+}
+
+void test_test_tc_bpf(void)
+{
+	const char *file = "./test_tc_bpf_kern.o";
+	struct bpf_program *clsp;
+	struct bpf_object *obj;
+	int cls_fd, ret;
+
+	obj = bpf_object__open(file);
+	if (!ASSERT_OK_PTR(obj, "bpf_object__open"))
+		return;
+
+	clsp = bpf_object__find_program_by_title(obj, "classifier");
+	if (!ASSERT_OK_PTR(clsp, "bpf_object__find_program_by_title"))
+		goto end;
+
+	ret = bpf_object__load(obj);
+	if (!ASSERT_EQ(ret, 0, "bpf_object__load"))
+		goto end;
+
+	cls_fd = bpf_program__fd(clsp);
+
+	system("tc qdisc del dev lo clsact");
+
+	ret = test_tc_internal(cls_fd, BPF_TC_CLSACT_INGRESS);
+	if (!ASSERT_EQ(ret, 0, "test_tc_internal INGRESS"))
+		goto end;
+
+	if (!ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
+		       "clsact qdisc delete failed"))
+		goto end;
+
+	ret = test_tc_info(cls_fd);
+	if (!ASSERT_EQ(ret, 0, "test_tc_info"))
+		goto end;
+
+	if (!ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
+		       "clsact qdisc delete failed"))
+		goto end;
+
+	ret = test_tc_internal(cls_fd, BPF_TC_CLSACT_EGRESS);
+	if (!ASSERT_EQ(ret, 0, "test_tc_internal EGRESS"))
+		goto end;
+
+	ASSERT_EQ(system("tc qdisc del dev lo clsact"), 0,
+		  "clsact qdisc delete failed");
+
+end:
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
new file mode 100644
index 000000000000..18a3a7ed924a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
@@ -0,0 +1,12 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+/* Dummy prog to test TC-BPF API */
+
+SEC("classifier")
+int cls(struct __sk_buff *skb)
+{
+	return 0;
+}