Re: [PATCH bpf-next V9 7/7] bpf/selftests: tests using bpf_check_mtu BPF-helper
From: Andrii Nakryiko <hidden>
Date: 2020-12-18 20:14:51
Also in:
bpf
On Thu, Dec 17, 2020 at 9:30 AM Jesper Dangaard Brouer [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Adding selftest for BPF-helper bpf_check_mtu(). Making sure it can be used from both XDP and TC. Signed-off-by: Jesper Dangaard Brouer <redacted> --- tools/testing/selftests/bpf/prog_tests/check_mtu.c | 204 ++++++++++++++++++++ tools/testing/selftests/bpf/progs/test_check_mtu.c | 196 +++++++++++++++++++ 2 files changed, 400 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/check_mtu.c create mode 100644 tools/testing/selftests/bpf/progs/test_check_mtu.cdiff --git a/tools/testing/selftests/bpf/prog_tests/check_mtu.c b/tools/testing/selftests/bpf/prog_tests/check_mtu.c new file mode 100644 index 000000000000..b5d0c3a9abe8 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/check_mtu.c@@ -0,0 +1,204 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2020 Jesper Dangaard Brouer */ + +#include <linux/if_link.h> /* before test_progs.h, avoid bpf_util.h redefines */ + +#include <test_progs.h> +#include "test_check_mtu.skel.h" +#include <network_helpers.h> + +#include <stdlib.h> +#include <inttypes.h> + +#define IFINDEX_LO 1 + +static __u32 duration; /* Hint: needed for CHECK macro */ + +static int read_mtu_device_lo(void) +{ + const char *filename = "/sys/devices/virtual/net/lo/mtu"; + char buf[11] = {}; + int value; + int fd; + + fd = open(filename, 0, O_RDONLY); + if (fd == -1) + return -1; + + if (read(fd, buf, sizeof(buf)) == -1)
close fd missing here?
+ return -2;
+ close(fd);
+
+ value = strtoimax(buf, NULL, 10);
+ if (errno == ERANGE)
+ return -3;
+
+ return value;
+}
+
+static void test_check_mtu_xdp_attach(struct bpf_program *prog)
+{
+ int err = 0;
+ int fd;
+
+ fd = bpf_program__fd(prog);
+ err = bpf_set_link_xdp_fd(IFINDEX_LO, fd, XDP_FLAGS_SKB_MODE);
+ if (CHECK(err, "XDP-attach", "failed"))
+ return;
+
+ bpf_set_link_xdp_fd(IFINDEX_LO, -1, 0);can you please use bpf_link-based bpf_program__attach_xdp() which will provide auto-cleanup in case of crash? also check that it succeeded?
+}
+
+static void test_check_mtu_run_xdp(struct test_check_mtu *skel,
+ struct bpf_program *prog,
+ __u32 mtu_expect)
+{
+ const char *prog_name = bpf_program__name(prog);
+ int retval_expect = XDP_PASS;
+ __u32 mtu_result = 0;
+ char buf[256];
+ int err;
+
+ struct bpf_prog_test_run_attr tattr = {
+ .repeat = 1,
+ .data_in = &pkt_v4,
+ .data_size_in = sizeof(pkt_v4),
+ .data_out = buf,
+ .data_size_out = sizeof(buf),
+ .prog_fd = bpf_program__fd(prog),
+ };nit: it's a variable declaration, so keep it all in one block. There is also opts-based variant, which might be good to use here instead.
+ + memset(buf, 0, sizeof(buf));
char buf[256] = {}; would make this unnecessary
+ + err = bpf_prog_test_run_xattr(&tattr); + CHECK_ATTR(err != 0 || errno != 0, "bpf_prog_test_run", + "prog_name:%s (err %d errno %d retval %d)\n", + prog_name, err, errno, tattr.retval); + + CHECK(tattr.retval != retval_expect, "retval",
whitespaces are off?
+ "progname:%s unexpected retval=%d expected=%d\n", + prog_name, tattr.retval, retval_expect); + + /* Extract MTU that BPF-prog got */ + mtu_result = skel->bss->global_bpf_mtu_xdp; + CHECK(mtu_result != mtu_expect, "MTU-compare-user", + "failed (MTU user:%d bpf:%d)", mtu_expect, mtu_result);
There is nicer ASSERT_EQ() macro for such cases: ASSERT_EQ(mtu_result, mtu_expect, "MTU-compare-user"); it will format sensible error message automatically
+} +
[...]
+ char buf[256];
+ int err;
+
+ struct bpf_prog_test_run_attr tattr = {
+ .repeat = 1,
+ .data_in = &pkt_v4,
+ .data_size_in = sizeof(pkt_v4),
+ .data_out = buf,
+ .data_size_out = sizeof(buf),
+ .prog_fd = bpf_program__fd(prog),
+ };
+
+ memset(buf, 0, sizeof(buf));
+same as above
+ err = bpf_prog_test_run_xattr(&tattr); + CHECK_ATTR(err != 0 || errno != 0, "bpf_prog_test_run", + "prog_name:%s (err %d errno %d retval %d)\n", + prog_name, err, errno, tattr.retval); + + CHECK(tattr.retval != retval_expect, "retval",
same :)
+ "progname:%s unexpected retval=%d expected=%d\n", + prog_name, tattr.retval, retval_expect); + + /* Extract MTU that BPF-prog got */ + mtu_result = skel->bss->global_bpf_mtu_tc; + CHECK(mtu_result != mtu_expect, "MTU-compare-user", + "failed (MTU user:%d bpf:%d)", mtu_expect, mtu_result); +} + +
[...]
+
+void test_check_mtu(void)
+{
+ struct test_check_mtu *skel;
+ __u32 mtu_lo;
+
+ skel = test_check_mtu__open_and_load();
+ if (CHECK(!skel, "open and load skel", "failed"))
+ return; /* Exit if e.g. helper unknown to kernel */
+
+ if (test__start_subtest("bpf_check_mtu XDP-attach"))
+ test_check_mtu_xdp_attach(skel->progs.xdp_use_helper_basic);
+
+ test_check_mtu__destroy(skel);here it's not clear why you instantiate skeleton outside of test_check_mtu_xdp_attach() subtest. Can you please move it in? It will keep this failure local to that specific subtest, not the entire test. And is just cleaner, of course.
+ + mtu_lo = read_mtu_device_lo(); + if (CHECK(mtu_lo < 0, "reading MTU value", "failed (err:%d)", mtu_lo))
ASSERT_OK() could be used here
+ return;
+
+ if (test__start_subtest("bpf_check_mtu XDP-run"))
+ test_check_mtu_xdp(mtu_lo, 0);
+
+ if (test__start_subtest("bpf_check_mtu XDP-run ifindex-lookup"))
+ test_check_mtu_xdp(mtu_lo, IFINDEX_LO);
+
+ if (test__start_subtest("bpf_check_mtu TC-run"))
+ test_check_mtu_tc(mtu_lo, 0);
+
+ if (test__start_subtest("bpf_check_mtu TC-run ifindex-lookup"))
+ test_check_mtu_tc(mtu_lo, IFINDEX_LO);
+}[...]
+
+ global_bpf_mtu_tc = mtu_len;
+ return retval;
+}
+
+SEC("classifier")nice use of the same SEC()'tion BPF programs!
+int tc_minus_delta(struct __sk_buff *ctx)
+{
+ int retval = BPF_OK; /* Expected retval on successful test */
+ __u32 ifindex = GLOBAL_USER_IFINDEX;
+ __u32 skb_len = ctx->len;
+ __u32 mtu_len = 0;
+ int delta;
+
+ /* Boarderline test case: Minus delta exceeding packet length allowed */
+ delta = -((skb_len - ETH_HLEN) + 1);
+
+ /* Minus length (adjusted via delta) still pass MTU check, other helpers
+ * are responsible for catching this, when doing actual size adjust
+ */
+ if (bpf_check_mtu(ctx, ifindex, &mtu_len, delta, 0))
+ retval = BPF_DROP;
+
+ global_bpf_mtu_xdp = mtu_len;
+ return retval;
+}