[PATCH bpf-next 0/2] cgroup/connect{4,6} programs for unprivileged ICMP ping

[PATCH bpf-next 0/2] cgroup/connect{4,6} programs for unprivileged ICMP ping

[YiFei Zhu]

Usually when a TCP/UDP connection is initiated, we can bind the socket
to a specific IP attached to an interface in a cgroup/connect hook.
But for pings, this is impossible, as the hook is not being called.

This series adds the invocation for cgroup/connect{4,6} programs to
unprivileged ICMP ping (i.e. ping sockets created with SOCK_DGRAM
IPPROTO_ICMP(V6) as opposed to SOCK_RAW). This also adds a test to
verify that the hooks are being called and invoking bpf_bind() from
within the hook actually binds the socket.

Patch 1 adds the invocation of the hook. Patch 2 adds the tests.

YiFei Zhu (2):
  bpf: Invoke cgroup/connect{4,6} programs for unprivileged ICMP ping
  selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv
    ICMP ping

 net/ipv4/ping.c                               |  15 +
 net/ipv6/ping.c                               |  16 +
 .../selftests/bpf/prog_tests/connect_ping.c   | 318 ++++++++++++++++++
 .../selftests/bpf/progs/connect_ping.c        |  53 +++
 4 files changed, 402 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/connect_ping.c
 create mode 100644 tools/testing/selftests/bpf/progs/connect_ping.c

-- 
2.37.2.789.g6183377224-goog

[PATCH bpf-next 1/2] bpf: Invoke cgroup/connect{4,6} programs for unprivileged ICMP ping

[YiFei Zhu]

Usually when a TCP/UDP connection is initiated, we can bind the socket
to a specific IP attached to an interface in a cgroup/connect hook.
But for pings, this is impossible, as the hook is not being called.

This adds the hook invocation to unprivileged ICMP ping (i.e. ping
sockets created with SOCK_DGRAM IPPROTO_ICMP(V6) as opposed to
SOCK_RAW. Logic is mirrored from UDP sockets where the hook is invoked
during pre_connect, after a check for suficiently sized addr_len.

Signed-off-by: YiFei Zhu <zhuyifei@google.com>
---
 net/ipv4/ping.c | 15 +++++++++++++++
 net/ipv6/ping.c | 16 ++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index b83c2bd9d7223..517042caf6dc1 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -33,6 +33,7 @@
 #include <linux/skbuff.h>
 #include <linux/proc_fs.h>
 #include <linux/export.h>
+#include <linux/bpf-cgroup.h>
 #include <net/sock.h>
 #include <net/ping.h>
 #include <net/udp.h>
@@ -295,6 +296,19 @@ void ping_close(struct sock *sk, long timeout)
 }
 EXPORT_SYMBOL_GPL(ping_close);
 
+static int ping_pre_connect(struct sock *sk, struct sockaddr *uaddr,
+			    int addr_len)
+{
+	/* This check is replicated from __ip4_datagram_connect() and
+	 * intended to prevent BPF program called below from accessing bytes
+	 * that are out of the bound specified by user in addr_len.
+	 */
+	if (addr_len < sizeof(struct sockaddr_in))
+		return -EINVAL;
+
+	return BPF_CGROUP_RUN_PROG_INET4_CONNECT_LOCK(sk, uaddr);
+}
+
 /* Checks the bind address and possibly modifies sk->sk_bound_dev_if. */
 static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 				struct sockaddr *uaddr, int addr_len)
@@ -1009,6 +1023,7 @@ struct proto ping_prot = {
 	.owner =	THIS_MODULE,
 	.init =		ping_init_sock,
 	.close =	ping_close,
+	.pre_connect =	ping_pre_connect,
 	.connect =	ip4_datagram_connect,
 	.disconnect =	__udp_disconnect,
 	.setsockopt =	ip_setsockopt,
diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
index 91b8405146569..5f2ef84937142 100644
--- a/net/ipv6/ping.c
+++ b/net/ipv6/ping.c
@@ -20,6 +20,7 @@
 #include <net/udp.h>
 #include <net/transp_v6.h>
 #include <linux/proc_fs.h>
+#include <linux/bpf-cgroup.h>
 #include <net/ping.h>
 
 static void ping_v6_destroy(struct sock *sk)
@@ -49,6 +50,20 @@ static int dummy_ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 	return 0;
 }
 
+static int ping_v6_pre_connect(struct sock *sk, struct sockaddr *uaddr,
+			       int addr_len)
+{
+	/* This check is replicated from __ip6_datagram_connect() and
+	 * intended to prevent BPF program called below from accessing
+	 * bytes that are out of the bound specified by user in addr_len.
+	 */
+
+	if (addr_len < SIN6_LEN_RFC2133)
+		return -EINVAL;
+
+	return BPF_CGROUP_RUN_PROG_INET6_CONNECT_LOCK(sk, uaddr);
+}
+
 static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 {
 	struct inet_sock *inet = inet_sk(sk);
@@ -191,6 +206,7 @@ struct proto pingv6_prot = {
 	.init =		ping_init_sock,
 	.close =	ping_close,
 	.destroy =	ping_v6_destroy,
+	.pre_connect =	ping_v6_pre_connect,
 	.connect =	ip6_datagram_connect_v6_only,
 	.disconnect =	__udp_disconnect,
 	.setsockopt =	ipv6_setsockopt,
-- 
2.37.2.789.g6183377224-goog

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

[YiFei Zhu]

This tests that when an unprivileged ICMP ping socket connects,
the hooks are actually invoked. We also ensure that if the hook does
not call bpf_bind(), the bound address is unmodified, and if the
hook calls bpf_bind(), the bound address is exactly what we provided
to the helper.

A new netns is used to enable ping_group_range in the test without
affecting ouside of the test, because by default, not even root is
permitted to use unprivileged ICMP ping...

Signed-off-by: YiFei Zhu <zhuyifei@google.com>
---
 .../selftests/bpf/prog_tests/connect_ping.c   | 318 ++++++++++++++++++
 .../selftests/bpf/progs/connect_ping.c        |  53 +++
 2 files changed, 371 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/connect_ping.c
 create mode 100644 tools/testing/selftests/bpf/progs/connect_ping.c

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 bool write_sysctl(const char *sysctl, const char *value)
+{
+	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;
+
+	if (!ASSERT_OK(connect(sock_fd, (struct sockaddr *)&sa, sa_len),
+		       "connect"))
+		goto close_bpf_object;
+
+	if (!ASSERT_EQ(obj->bss->invocations_v4, 1, "invocations_v4"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(obj->bss->invocations_v6, 0, "invocations_v6"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(obj->bss->has_error, 0, "has_error"))
+		goto close_bpf_object;
+
+	if (!ASSERT_OK(getsockname(sock_fd, (struct sockaddr *)&sa, &sa_len),
+		       "getsockname"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(sa.sin_family, AF_INET, "sin_family"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(sa.sin_addr.s_addr, htonl(INADDR_LOOPBACK), "sin_addr"))
+		goto close_bpf_object;
+
+close_bpf_object:
+	connect_ping__destroy(obj);
+close_sock:
+	close(sock_fd);
+}
+
+static void test_ipv4_bind(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 = 1;
+
+	/* 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 address we explicitly bound.
+	 */
+	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;
+
+	if (!ASSERT_OK(connect(sock_fd, (struct sockaddr *)&sa, sa_len),
+		       "connect"))
+		goto close_bpf_object;
+
+	if (!ASSERT_EQ(obj->bss->invocations_v4, 1, "invocations_v4"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(obj->bss->invocations_v6, 0, "invocations_v6"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(obj->bss->has_error, 0, "has_error"))
+		goto close_bpf_object;
+
+	if (!ASSERT_OK(getsockname(sock_fd, (struct sockaddr *)&sa, &sa_len),
+		       "getsockname"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(sa.sin_family, AF_INET, "sin_family"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(sa.sin_addr.s_addr, htonl(0x01010101), "sin_addr"))
+		goto close_bpf_object;
+
+close_bpf_object:
+	connect_ping__destroy(obj);
+close_sock:
+	close(sock_fd);
+}
+
+static void test_ipv6(int cgroup_fd)
+{
+	struct sockaddr_in6 sa = {
+		.sin6_family = AF_INET6,
+		.sin6_addr = IN6ADDR_LOOPBACK_INIT,
+	};
+	socklen_t sa_len = sizeof(sa);
+	struct connect_ping *obj;
+	int sock_fd;
+
+	sock_fd = socket(AF_INET6, SOCK_DGRAM, IPPROTO_ICMPV6);
+	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 v6 ping socket to
+	 * localhost, assert that only v6 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;
+
+	if (!ASSERT_OK(connect(sock_fd, (struct sockaddr *)&sa, sa_len),
+		       "connect"))
+		goto close_bpf_object;
+
+	if (!ASSERT_EQ(obj->bss->invocations_v4, 0, "invocations_v4"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(obj->bss->invocations_v6, 1, "invocations_v6"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(obj->bss->has_error, 0, "has_error"))
+		goto close_bpf_object;
+
+	if (!ASSERT_OK(getsockname(sock_fd, (struct sockaddr *)&sa, &sa_len),
+		       "getsockname"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(sa.sin6_family, AF_INET6, "sin6_family"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(memcmp(&sa.sin6_addr, &in6addr_loopback, sizeof(in6addr_loopback)),
+		       0, "sin_addr"))
+		goto close_bpf_object;
+
+close_bpf_object:
+	connect_ping__destroy(obj);
+close_sock:
+	close(sock_fd);
+}
+
+static void test_ipv6_bind(int cgroup_fd)
+{
+	struct sockaddr_in6 sa = {
+		.sin6_family = AF_INET6,
+		.sin6_addr = IN6ADDR_LOOPBACK_INIT,
+	};
+	socklen_t sa_len = sizeof(sa);
+	struct connect_ping *obj;
+	int sock_fd;
+
+	sock_fd = socket(AF_INET6, SOCK_DGRAM, IPPROTO_ICMPV6);
+	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 = 1;
+
+	/* Attach connect v4 and connect v6 progs, connect a v6 ping socket to
+	 * localhost, assert that only v6 is called, and called exactly once,
+	 * and that the socket's bound address is address we explicitly bound.
+	 */
+	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;
+
+	if (!ASSERT_OK(connect(sock_fd, (struct sockaddr *)&sa, sa_len),
+		       "connect"))
+		goto close_bpf_object;
+
+	if (!ASSERT_EQ(obj->bss->invocations_v4, 0, "invocations_v4"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(obj->bss->invocations_v6, 1, "invocations_v6"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(obj->bss->has_error, 0, "has_error"))
+		goto close_bpf_object;
+
+	if (!ASSERT_OK(getsockname(sock_fd, (struct sockaddr *)&sa, &sa_len),
+		       "getsockname"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(sa.sin6_family, AF_INET6, "sin6_family"))
+		goto close_bpf_object;
+	if (!ASSERT_EQ(memcmp(&sa.sin6_addr, &bindaddr_v6, sizeof(bindaddr_v6)),
+		       0, "sin_addr"))
+		goto close_bpf_object;
+
+close_bpf_object:
+	connect_ping__destroy(obj);
+close_sock:
+	close(sock_fd);
+}
+
+void test_connect_ping(void)
+{
+	int cgroup_fd;
+
+	if (!ASSERT_OK(unshare(CLONE_NEWNET | CLONE_NEWNS), "unshare"))
+		return;
+
+	/* overmount sysfs, and making original sysfs private so overmount
+	 * does not propagate to other mntns.
+	 */
+	if (!ASSERT_OK(mount("none", "/sys", NULL, MS_PRIVATE, NULL),
+		       "remount-private-sys"))
+		return;
+	if (!ASSERT_OK(mount("sysfs", "/sys", "sysfs", 0, NULL),
+		       "mount-sys"))
+		return;
+	if (!ASSERT_OK(mount("bpffs", "/sys/fs/bpf", "bpf", 0, NULL),
+		       "mount-bpf"))
+		goto clean_mount;
+
+	if (!ASSERT_OK(system("ip link set dev lo up"), "lo-up"))
+		goto clean_mount;
+	if (!ASSERT_OK(system("ip addr add 1.1.1.1 dev lo"), "lo-addr-v4"))
+		goto clean_mount;
+	if (!ASSERT_OK(system("ip -6 addr add 2001:db8::1 dev lo"), "lo-addr-v6"))
+		goto clean_mount;
+	if (!write_sysctl("/proc/sys/net/ipv4/ping_group_range", "0 0"))
+		goto clean_mount;
+
+	cgroup_fd = test__join_cgroup("/connect_ping");
+	if (!ASSERT_GE(cgroup_fd, 0, "cg-create"))
+		goto clean_mount;
+
+	if (test__start_subtest("ipv4"))
+		test_ipv4(cgroup_fd);
+	if (test__start_subtest("ipv4-bind"))
+		test_ipv4_bind(cgroup_fd);
+
+	if (test__start_subtest("ipv6"))
+		test_ipv6(cgroup_fd);
+	if (test__start_subtest("ipv6-bind"))
+		test_ipv6_bind(cgroup_fd);
+
+	close(cgroup_fd);
+
+clean_mount:
+	umount2("/sys", MNT_DETACH);
+}
diff --git a/tools/testing/selftests/bpf/progs/connect_ping.c b/tools/testing/selftests/bpf/progs/connect_ping.c
new file mode 100644
index 0000000000000..60178192b672f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/connect_ping.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright 2022 Google LLC.
+ */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <netinet/in.h>
+#include <sys/socket.h>
+
+/* 2001:db8::1 */
+#define BINDADDR_V6 { { { 0x20,0x01,0x0d,0xb8,0,0,0,0,0,0,0,0,0,0,0,1 } } }
+
+__u32 do_bind = 0;
+__u32 has_error = 0;
+__u32 invocations_v4 = 0;
+__u32 invocations_v6 = 0;
+
+SEC("cgroup/connect4")
+int connect_v4_prog(struct bpf_sock_addr *ctx)
+{
+	struct sockaddr_in sa = {
+		.sin_family = AF_INET,
+		.sin_addr.s_addr = bpf_htonl(0x01010101),
+	};
+
+	__sync_fetch_and_add(&invocations_v4, 1);
+
+	if (do_bind && bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)))
+		has_error = 1;
+
+	return 1;
+}
+
+SEC("cgroup/connect6")
+int connect_v6_prog(struct bpf_sock_addr *ctx)
+{
+	struct sockaddr_in6 sa = {
+		.sin6_family = AF_INET6,
+		.sin6_addr = BINDADDR_V6,
+	};
+
+	__sync_fetch_and_add(&invocations_v6, 1);
+
+	if (do_bind && bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)))
+		has_error = 1;
+
+	return 1;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.37.2.789.g6183377224-goog

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

[Martin KaFai Lau]

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?

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

[YiFei Zhu]

On Thu, Sep 1, 2022 at 10:55 PM Martin KaFai Lau <kafai@fb.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.

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

[Martin KaFai Lau]

On 9/2/22 4:52 PM, YiFei Zhu wrote:
>> 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.
Yep. Keeping them separate is fine.  I was asking because they are 
testing the same hook other than the port-vs-ip difference.

connect[46]_prog.c looks like a better one also but not yet in 
test_progs infra.  It will be useful to migrate it in the future.

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

[YiFei Zhu]

On Fri, Sep 2, 2022 at 4:52 PM YiFei Zhu <zhuyifei@google.com> wrote:
>
> On Thu, Sep 1, 2022 at 10:55 PM Martin KaFai Lau <kafai@fb.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.

I changed it to test_progs.c because this helper uses CHECK which is
provided by test_progs.h, and testing_helpers.c seems to be used
elsewhere (non-test_progs) too. Lmk if this is not a good alternative.