On Sat, May 01, 2021 at 01:11:47AM IST, Andrii Nakryiko wrote:

On Wed, Apr 28, 2021 at 9:26 AM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

This adds some basic tests for the low level bpf_tc_* API.

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

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
new file mode 100644
index 000000000000..40441f4e23e2
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c

@@ -0,0 +1,467 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include <linux/pkt_cls.h>
+
+#include "test_tc_bpf.skel.h"
+
+#define LO_IFINDEX 1
+
+static int test_tc_internal(const struct bpf_tc_hook *hook, int fd)
+{
+       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1,
+                           .prog_fd = fd);

we have 100 characters, if needed, use it to keep it on the single line

Ok.

quoted

+       struct bpf_prog_info info = {};
+       int ret;
+
+       ret = bpf_obj_get_info_by_fd(fd, &info, &(__u32){sizeof(info)});

as in previous patch, don't do this

quoted

+       if (!ASSERT_OK(ret, "bpf_obj_get_info_by_fd"))
+               return ret;
+
+       ret = bpf_tc_attach(hook, &opts, 0);
+       if (!ASSERT_OK(ret, "bpf_tc_attach"))
+               return ret;
+
+       if (!ASSERT_EQ(opts.handle, 1, "handle set") ||
+           !ASSERT_EQ(opts.priority, 1, "priority set") ||
+           !ASSERT_EQ(opts.prog_id, info.id, "prog_id set"))
+               goto end;
+
+       DECLARE_LIBBPF_OPTS(bpf_tc_opts, info_opts, .prog_fd = fd);

this is not C89, please move variable declarations to the top

quoted

+       ret = bpf_tc_query(hook, &info_opts);
+       if (!ASSERT_OK(ret, "bpf_tc_query"))
+               goto end;
+
+       DECLARE_LIBBPF_OPTS(bpf_tc_opts, info_opts2, .prog_id = info.id);

and here

quoted

+       ret = bpf_tc_query(hook, &info_opts2);
+       if (!ASSERT_OK(ret, "bpf_tc_query"))
+               goto end;
+
+       if (!ASSERT_EQ(opts.handle, 1, "handle set") ||
+           !ASSERT_EQ(opts.priority, 1, "priority set") ||
+           !ASSERT_EQ(opts.prog_id, info.id, "prog_id set"))
+               goto end;
+
+       opts.prog_id = 0;
+       ret = bpf_tc_attach(hook, &opts, BPF_TC_F_REPLACE);
+       if (!ASSERT_OK(ret, "bpf_tc_attach replace mode"))
+               return ret;

goto end?

Yes, thanks for spotting it.

quoted

+
+end:
+       opts.prog_fd = opts.prog_id = 0;
+       ret = bpf_tc_detach(hook, &opts);
+       ASSERT_OK(ret, "bpf_tc_detach");
+       return ret;
+}
+

[...]

quoted

+
+       /* attach */
+       ret = bpf_tc_attach(NULL, &attach_opts, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid hook = NULL"))
+               return -EINVAL;
+       ret = bpf_tc_attach(hook, &attach_opts, 42);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid flags"))
+               return -EINVAL;
+       attach_opts.prog_fd = 0;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid prog_fd unset"))
+               return -EINVAL;
+       attach_opts.prog_fd = fd;
+       attach_opts.prog_id = 42;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid prog_id set"))
+               return -EINVAL;
+       attach_opts.prog_id = 0;
+       attach_opts.handle = 0;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_OK(ret, "bpf_tc_attach valid handle unset"))
+               return -EINVAL;
+       attach_opts.prog_fd = attach_opts.prog_id = 0;
+       ASSERT_OK(bpf_tc_detach(hook, &attach_opts), "bpf_tc_detach");

this code is quite hard to follow, maybe sprinkle empty lines between
logical groups of statements (i.e., prepare inputs + call bpf_tc_xxx +
assert is one group that goes together)

I agree it looks bad. I can also just make a new opts for each combination, and
name it that way. Maybe that will look much better.

quoted

+       attach_opts.prog_fd = fd;
+       attach_opts.handle = 1;
+       attach_opts.priority = 0;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_OK(ret, "bpf_tc_attach valid priority unset"))
+               return -EINVAL;
+       attach_opts.prog_fd = attach_opts.prog_id = 0;
+       ASSERT_OK(bpf_tc_detach(hook, &attach_opts), "bpf_tc_detach");
+       attach_opts.prog_fd = fd;
+       attach_opts.priority = UINT16_MAX + 1;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid priority > UINT16_MAX"))
+               return -EINVAL;
+       attach_opts.priority = 0;
+       attach_opts.handle = attach_opts.priority = 0;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_OK(ret, "bpf_tc_attach valid both handle and priority unset"))
+               return -EINVAL;
+       attach_opts.prog_fd = attach_opts.prog_id = 0;
+       ASSERT_OK(bpf_tc_detach(hook, &attach_opts), "bpf_tc_detach");
+       ret = bpf_tc_attach(hook, NULL, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid opts = NULL"))
+               return -EINVAL;
+
+       return 0;
+}
+
+static int test_tc_query(const struct bpf_tc_hook *hook, int fd)
+{
+       struct test_tc_bpf *skel = NULL;
+       int new_fd, ret, i = 0;
+
+       skel = test_tc_bpf__open_and_load();
+       if (!ASSERT_OK_PTR(skel, "test_tc_bpf__open_and_load"))
+               return -EINVAL;
+
+       new_fd = bpf_program__fd(skel->progs.cls);
+
+       /* make sure no other filters are attached */
+       ret = bpf_tc_query(hook, NULL);
+       if (!ASSERT_EQ(ret, -ENOENT, "bpf_tc_query == -ENOENT"))
+               goto end_destroy;
+
+       for (i = 0; i < 5; i++) {
+               DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .prog_fd = fd);

empty line after variable declaration

Ok, will fix everywhere.

quoted

+               ret = bpf_tc_attach(hook, &opts, 0);
+               if (!ASSERT_OK(ret, "bpf_tc_attach"))
+                       goto end;
+       }
+       DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1,
+                           .prog_fd = new_fd);
+       ret = bpf_tc_attach(hook, &opts, 0);
+       if (!ASSERT_OK(ret, "bpf_tc_attach"))
+               goto end;
+       i++;
+
+       ASSERT_EQ(opts.handle, 1, "handle match");
+       ASSERT_EQ(opts.priority, 1, "priority match");
+       ASSERT_NEQ(opts.prog_id, 0, "prog_id set");
+
+       opts.prog_fd = 0;
+       /* search with handle, priority, prog_id */
+       ret = bpf_tc_query(hook, &opts);
+       if (!ASSERT_OK(ret, "bpf_tc_query"))
+               goto end;
+
+       ASSERT_EQ(opts.handle, 1, "handle match");
+       ASSERT_EQ(opts.priority, 1, "priority match");
+       ASSERT_NEQ(opts.prog_id, 0, "prog_id set");
+
+       opts.priority = opts.prog_fd = 0;
+       /* search with handle, prog_id */
+       ret = bpf_tc_query(hook, &opts);
+       if (!ASSERT_OK(ret, "bpf_tc_query"))
+               goto end;
+
+       ASSERT_EQ(opts.handle, 1, "handle match");
+       ASSERT_EQ(opts.priority, 1, "priority match");
+       ASSERT_NEQ(opts.prog_id, 0, "prog_id set");
+
+       opts.handle = opts.prog_fd = 0;
+       /* search with priority, prog_id */
+       ret = bpf_tc_query(hook, &opts);
+       if (!ASSERT_OK(ret, "bpf_tc_query"))
+               goto end;
+
+       ASSERT_EQ(opts.handle, 1, "handle match");
+       ASSERT_EQ(opts.priority, 1, "priority match");
+       ASSERT_NEQ(opts.prog_id, 0, "prog_id set");
+
+       opts.handle = opts.priority = opts.prog_fd = 0;
+       /* search with prog_id */
+       ret = bpf_tc_query(hook, &opts);
+       if (!ASSERT_OK(ret, "bpf_tc_query"))
+               goto end;
+
+       ASSERT_EQ(opts.handle, 1, "handle match");
+       ASSERT_EQ(opts.priority, 1, "priority match");
+       ASSERT_NEQ(opts.prog_id, 0, "prog_id set");
+
+       while (i != 1) {
+               DECLARE_LIBBPF_OPTS(bpf_tc_opts, del_opts, .prog_fd = fd);

empty line here

quoted

+               ret = bpf_tc_query(hook, &del_opts);
+               if (!ASSERT_OK(ret, "bpf_tc_query"))
+                       goto end;
+               ASSERT_NEQ(del_opts.prog_id, opts.prog_id, "prog_id should not be same");
+               ASSERT_NEQ(del_opts.priority, 1, "priority should not be 1");
+               del_opts.prog_fd = del_opts.prog_id = 0;
+               ret = bpf_tc_detach(hook, &del_opts);
+               if (!ASSERT_OK(ret, "bpf_tc_detach"))
+                       goto end;
+               i--;
+       }
+
+       opts.handle = opts.priority = opts.prog_id = 0;
+       opts.prog_fd = fd;
+       ret = bpf_tc_query(hook, &opts);
+       ASSERT_EQ(ret, -ENOENT, "bpf_tc_query == -ENOENT");
+
+end:
+       while (i--) {
+               DECLARE_LIBBPF_OPTS(bpf_tc_opts, del_opts, 0);

you get the idea by now

quoted

+               ret = bpf_tc_query(hook, &del_opts);
+               if (!ASSERT_OK(ret, "bpf_tc_query"))
+                       break;
+               del_opts.prog_id = 0;
+               ret = bpf_tc_detach(hook, &del_opts);
+               if (!ASSERT_OK(ret, "bpf_tc_detach"))
+                       break;
+       }
+       ASSERT_EQ(bpf_tc_query(hook, NULL), -ENOENT, "bpf_tc_query == -ENOENT");
+end_destroy:
+       test_tc_bpf__destroy(skel);
+       return ret;
+}
+

[...]

--
Kartikeya