Date:   Fri, 2 Sep 2022 16:52:54 -0700
From:   YiFei Zhu <zhuyifei@...gle.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Stanislav Fomichev <sdf@...gle.com>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6}
 programs can bind unpriv ICMP ping

On Thu, Sep 1, 2022 at 10:55 PM Martin KaFai Lau <kafai@...com> wrote:
>
> On Thu, Sep 01, 2022 at 07:15:10PM +0000, YiFei Zhu wrote:
> > diff --git a/tools/testing/selftests/bpf/prog_tests/connect_ping.c b/tools/testing/selftests/bpf/prog_tests/connect_ping.c
> > new file mode 100644
> > index 0000000000000..99b1a2f0c4921
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/connect_ping.c
> > @@ -0,0 +1,318 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +/*
> > + * Copyright 2022 Google LLC.
> > + */
> > +
> > +#define _GNU_SOURCE
> > +#include <sys/mount.h>
> > +
> > +#include <test_progs.h>
> > +#include <cgroup_helpers.h>
> > +#include <network_helpers.h>
> > +
> > +#include "connect_ping.skel.h"
> > +
> > +/* 2001:db8::1 */
> > +#define BINDADDR_V6 { { { 0x20,0x01,0x0d,0xb8,0,0,0,0,0,0,0,0,0,0,0,1 } } }
> > +const struct in6_addr bindaddr_v6 = BINDADDR_V6;
> static

ack.

> > +
> > +static bool write_sysctl(const char *sysctl, const char *value)
> This has been copied >2 times now which probably shows it will
> also be useful in the future.
> Take this chance to move it to testing_helpers.{h,c}.

ack.

> > +{
> > +     int fd, err, len;
> > +
> > +     fd = open(sysctl, O_WRONLY);
> > +     if (!ASSERT_GE(fd, 0, "open-sysctl"))
> > +             return false;
> > +
> > +     len = strlen(value);
> > +     err = write(fd, value, len);
> > +     close(fd);
> > +     if (!ASSERT_EQ(err, len, "write-sysctl"))
> > +             return false;
> > +
> > +     return true;
> > +}
> > +
> > +static void test_ipv4(int cgroup_fd)
> > +{
> > +     struct sockaddr_in sa = {
> > +             .sin_family = AF_INET,
> > +             .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> > +     };
> > +     socklen_t sa_len = sizeof(sa);
> > +     struct connect_ping *obj;
> > +     int sock_fd;
> > +
> > +     sock_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
> > +     if (!ASSERT_GE(sock_fd, 0, "sock-create"))
> > +             return;
> > +
> > +     obj = connect_ping__open_and_load();
> > +     if (!ASSERT_OK_PTR(obj, "skel-load"))
> > +             goto close_sock;
> > +
> > +     obj->bss->do_bind = 0;
> > +
> > +     /* Attach connect v4 and connect v6 progs, connect a v4 ping socket to
> > +      * localhost, assert that only v4 is called, and called exactly once,
> > +      * and that the socket's bound address is original loopback address.
> > +      */
> > +     obj->links.connect_v4_prog =
> > +             bpf_program__attach_cgroup(obj->progs.connect_v4_prog, cgroup_fd);
> > +     if (!ASSERT_OK_PTR(obj->links.connect_v4_prog, "cg-attach-v4"))
> > +             goto close_bpf_object;
> > +     obj->links.connect_v6_prog =
> > +             bpf_program__attach_cgroup(obj->progs.connect_v6_prog, cgroup_fd);
> > +     if (!ASSERT_OK_PTR(obj->links.connect_v6_prog, "cg-attach-v6"))
> > +             goto close_bpf_object;
> Overall, it seems like a lot of dup code can be saved
> between test_ipv4, test_ipv6, and their _bind() version.
>
> eg. The skel setup can be done once and the bss variables can be reset
> at the beginning of each test by memset(skel->bss, 0, sizeof(*skel->bss)).
> The result checking part is essentially checking the expected bss values
> and the getsockname result also.

ack.

> btw, does it make sense to do it as a subtest in
> connect_force_port.c or they are very different?

I could try, but they are structured differently; that checks the
ports whereas this checks the bound IPs. That test also doesn't use
skels or sets up netns whereas this test does. I think I would prefer
to have two tests since tests are cheap, but I can try to restructure
connect_force_port.c in a way that is compatible with both if you
insist.

Powered by blists - more mailing lists