netdev - Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv ICMP ping

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]

Date:   Thu, 1 Sep 2022 22:55:27 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     YiFei Zhu <zhuyifei@...gle.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 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

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

> +{
> +	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.

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