netdev - Re: [PATCH bpf-next v5 3/3] libbpf: add selftests for TC-BPF API

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: <CAEf4BzaZLEHF-Sg3H7Q5ERxPhw++ok0samjj9C4ET2ttXqztGQ@mail.gmail.com>
Date:   Mon, 3 May 2021 15:55:39 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf <bpf@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Shaun Crampton <shaun@...era.io>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v5 3/3] libbpf: add selftests for TC-BPF API

On Fri, Apr 30, 2021 at 11:34 PM Kumar Kartikeya Dwivedi
<memxor@...il.com> wrote:
>
> 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
> > <memxor@...il.com> wrote:
> > >
> > > This adds some basic tests for the low level bpf_tc_* API.
> > >
> > > Reviewed-by: Toke Høiland-Jørgensen <toke@...hat.com>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.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
> > >

[...]

> >
> > > +
> > > +       /* 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.

It probably would be just more code to read. Try to space it out with
empty lines into logical groups, that should be enough.

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

[...]