netdev - Re: [PATCH bpf-next v6 7/7] selftests/bpf: Add tests for XDP bonding

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: <CAHn8xcnBQhO_=YEO2cd_uCRYQDZkfQjW2r8aExu8=FYTi_=X5A@mail.gmail.com>
Date:   Mon, 9 Aug 2021 16:24:56 +0200
From:   Jussi Maki <joamaki@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>, j.vosburgh@...il.com,
        Andy Gospodarek <andy@...yhouse.net>, vfalico@...il.com,
        Andrii Nakryiko <andrii@...nel.org>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>
Subject: Re: [PATCH bpf-next v6 7/7] selftests/bpf: Add tests for XDP bonding

On Sat, Aug 7, 2021 at 12:50 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Thu, Aug 5, 2021 at 9:10 AM Jussi Maki <joamaki@...il.com> wrote:
> >
> > Add a test suite to test XDP bonding implementation
> > over a pair of veth devices.
> >
> > Signed-off-by: Jussi Maki <joamaki@...il.com>
> > ---
> >  .../selftests/bpf/prog_tests/xdp_bonding.c    | 520 ++++++++++++++++++
> >  1 file changed, 520 insertions(+)
> >
>
> I don't pretend to understand what's going on in this selftests, but
> it looks good from the generic selftest standpoint. One and half small
> issues below, please double-check (and probably fix the fd close
> issue).

Thanks for the reviews!

> > +       if (xdp_attach(skeletons,
> > +                      skeletons->xdp_redirect_multi_kern->progs.xdp_redirect_map_multi_prog,
> > +                      "bond2"))
> > +               goto out;
> > +
> > +       restore_root_netns();
>
> the "goto out" below might call restore_root_netns() again, is that ok?

Yep that's fine.

> > +       if (!test__start_subtest("xdp_bonding_redirect_multi"))
> > +               test_xdp_bonding_redirect_multi(&skeletons);
> > +
> > +out:
> > +       xdp_dummy__destroy(skeletons.xdp_dummy);
> > +       xdp_tx__destroy(skeletons.xdp_tx);
> > +       xdp_redirect_multi_kern__destroy(skeletons.xdp_redirect_multi_kern);
> > +
> > +       libbpf_set_print(old_print_fn);
> > +       if (root_netns_fd)
>
> technically, fd could be 0, so for fds we have if (fd >= 0)
> everywhere. Also, if open() above fails, root_netns_fd will be -1 and
> you'll still attempt to close it.

Good catch. Daniel, could you fix this when applying to be "if
(root_netns_fd >= 0)"?