Re: [PATCH bpf-next V9 7/7] bpf/selftests: tests using bpf_check_mtu BPF-helper

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Fri, 18 Dec 2020 12:13:45 -0800
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> On Thu, Dec 17, 2020 at 9:30 AM Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> >
> > 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 <brouer@redhat.com>
> > ---
> >  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.c
> >
> > diff --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";

I will change this to: /sys/class/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?

ack, fixed.

> > +               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?

Sure, that will be good for me to learn.

> 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

ok.

> 
> > +
> > +       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?

Yes, I noticed with scripts/checkpatch.pl.  And there are a couple
more, that I've already fixed.


> > +             "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

Nice simplification :-)

> 
> > +}
> > +  
> 
> [...]

[... same ...] 

> [...]
> 
> > +
> > +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.

Sure will "move it in".  The intent was to fail the entire test if this
failed, but it is more clean to "move it in".

> > +
> > +       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;
> > +}



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer