netdev - Re: [bpf-next PATCH v2 4/5] selftests/bpf: Migrate tcpbpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0UeV4OKC8dhMgA-RvRfa4kr6NsqF=CCdW-Oe2mx+E4MPEg@mail.gmail.com>
Date:   Tue, 3 Nov 2020 07:44:38 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Kernel Team <kernel-team@...com>,
        Netdev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Lawrence Brakmo <brakmo@...com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        alexanderduyck@...com
Subject: Re: [bpf-next PATCH v2 4/5] selftests/bpf: Migrate tcpbpf_user.c to
 use BPF skeleton

On Mon, Nov 2, 2020 at 4:55 PM Martin KaFai Lau <kafai@...com> wrote:
>
> On Sat, Oct 31, 2020 at 11:52:31AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@...com>
> >
> > Update tcpbpf_user.c to make use of the BPF skeleton. Doing this we can
> > simplify test_tcpbpf_user and reduce the overhead involved in setting up
> > the test.
> >
> > In addition we can clean up the remaining bits such as the one remaining
> > CHECK_FAIL at the end of test_tcpbpf_user so that the function only makes
> > use of CHECK as needed.
> >
> > Acked-by: Andrii Nakryiko <andrii@...nel.org>
> > Signed-off-by: Alexander Duyck <alexanderduyck@...com>
> Acked-by: Martin KaFai Lau <kafai@...com>
>
> > ---
> >  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   48 ++++++++------------
> >  1 file changed, 18 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > index d96f4084d2f5..c7a61b0d616a 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > @@ -4,6 +4,7 @@
> >  #include <network_helpers.h>
> >
> >  #include "test_tcpbpf.h"
> > +#include "test_tcpbpf_kern.skel.h"
> >
> >  #define LO_ADDR6 "::1"
> >  #define CG_NAME "/tcpbpf-user-test"
> > @@ -133,44 +134,31 @@ static void run_test(int map_fd, int sock_map_fd)
> >
> >  void test_tcpbpf_user(void)
> >  {
> > -     const char *file = "test_tcpbpf_kern.o";
> > -     int prog_fd, map_fd, sock_map_fd;
> > -     int error = EXIT_FAILURE;
> > -     struct bpf_object *obj;
> > +     struct test_tcpbpf_kern *skel;
> > +     int map_fd, sock_map_fd;
> >       int cg_fd = -1;
> > -     int rv;
> > -
> > -     cg_fd = test__join_cgroup(CG_NAME);
> > -     if (cg_fd < 0)
> > -             goto err;
> >
> > -     if (bpf_prog_load(file, BPF_PROG_TYPE_SOCK_OPS, &obj, &prog_fd)) {
> > -             fprintf(stderr, "FAILED: load_bpf_file failed for: %s\n", file);
> > -             goto err;
> > -     }
> > +     skel = test_tcpbpf_kern__open_and_load();
> > +     if (CHECK(!skel, "open and load skel", "failed"))
> > +             return;
> >
> > -     rv = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_SOCK_OPS, 0);
> > -     if (rv) {
> > -             fprintf(stderr, "FAILED: bpf_prog_attach: %d (%s)\n",
> > -                    errno, strerror(errno));
> > -             goto err;
> > -     }
> > +     cg_fd = test__join_cgroup(CG_NAME);
> > +     if (CHECK(cg_fd < 0, "test__join_cgroup(" CG_NAME ")",
> > +               "cg_fd:%d errno:%d", cg_fd, errno))
> > +             goto cleanup_skel;
> >
> > -     map_fd = bpf_find_map(__func__, obj, "global_map");
> > -     if (map_fd < 0)
> > -             goto err;
> > +     map_fd = bpf_map__fd(skel->maps.global_map);
> > +     sock_map_fd = bpf_map__fd(skel->maps.sockopt_results);
> >
> > -     sock_map_fd = bpf_find_map(__func__, obj, "sockopt_results");
> > -     if (sock_map_fd < 0)
> > -             goto err;
> > +     skel->links.bpf_testcb = bpf_program__attach_cgroup(skel->progs.bpf_testcb, cg_fd);
> > +     if (ASSERT_OK_PTR(skel->links.bpf_testcb, "attach_cgroup(bpf_testcb)"))
> > +             goto cleanup_namespace;
> >
> >       run_test(map_fd, sock_map_fd);
> >
> > -     error = 0;
> > -err:
> > -     bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
> > +cleanup_namespace:
> nit.
>
> may be "cleanup_cgroup" instead?
>
> or only have one jump label to handle failure since "cg_fd != -1" has been
> tested already.

Good point. I can go through and just drop the second label and
simplify this. Will fix for v3.