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 PHC | |
Open Source and information security mailing list archives
| ||
|
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