* [PATCH bpf-next 0/2] cgroup/connect{4,6} programs for unprivileged ICMP ping
@ 2022-09-01 19:15 YiFei Zhu
2022-09-01 19:15 ` [PATCH bpf-next 1/2] bpf: Invoke " YiFei Zhu
2022-09-01 19:15 ` [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv " YiFei Zhu
0 siblings, 2 replies; 7+ messages in thread
From: YiFei Zhu @ 2022-09-01 19:15 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
Martin KaFai Lau, John Fastabend, Jiri Olsa, David S. Miller,
Hideaki YOSHIFUJI, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next 1/2] bpf: Invoke cgroup/connect{4,6} programs for unprivileged ICMP ping
2022-09-01 19:15 [PATCH bpf-next 0/2] cgroup/connect{4,6} programs for unprivileged ICMP ping YiFei Zhu
@ 2022-09-01 19:15 ` YiFei Zhu
2022-09-01 19:15 ` [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv " YiFei Zhu
1 sibling, 0 replies; 7+ messages in thread
From: YiFei Zhu @ 2022-09-01 19:15 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
Martin KaFai Lau, John Fastabend, Jiri Olsa, David S. Miller,
Hideaki YOSHIFUJI, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv ICMP ping
2022-09-01 19:15 [PATCH bpf-next 0/2] cgroup/connect{4,6} programs for unprivileged ICMP ping YiFei Zhu
2022-09-01 19:15 ` [PATCH bpf-next 1/2] bpf: Invoke " YiFei Zhu
@ 2022-09-01 19:15 ` YiFei Zhu
2022-09-02 5:55 ` Martin KaFai Lau
1 sibling, 1 reply; 7+ messages in thread
From: YiFei Zhu @ 2022-09-01 19:15 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
Martin KaFai Lau, John Fastabend, Jiri Olsa, David S. Miller,
Hideaki YOSHIFUJI, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv ICMP ping
2022-09-01 19:15 ` [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv " YiFei Zhu
@ 2022-09-02 5:55 ` Martin KaFai Lau
2022-09-02 23:52 ` YiFei Zhu
0 siblings, 1 reply; 7+ messages in thread
From: Martin KaFai Lau @ 2022-09-02 5:55 UTC (permalink / raw)
To: YiFei Zhu
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
Stanislav Fomichev, Martin KaFai Lau, John Fastabend, Jiri Olsa,
David S. Miller, Hideaki YOSHIFUJI, David Ahern, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv ICMP ping
2022-09-02 5:55 ` Martin KaFai Lau
@ 2022-09-02 23:52 ` YiFei Zhu
2022-09-06 17:16 ` Martin KaFai Lau
2022-09-06 23:52 ` YiFei Zhu
0 siblings, 2 replies; 7+ messages in thread
From: YiFei Zhu @ 2022-09-02 23:52 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
Stanislav Fomichev, Martin KaFai Lau, John Fastabend, Jiri Olsa,
David S. Miller, Hideaki YOSHIFUJI, David Ahern, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv ICMP ping
2022-09-02 23:52 ` YiFei Zhu
@ 2022-09-06 17:16 ` Martin KaFai Lau
2022-09-06 23:52 ` YiFei Zhu
1 sibling, 0 replies; 7+ messages in thread
From: Martin KaFai Lau @ 2022-09-06 17:16 UTC (permalink / raw)
To: YiFei Zhu
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
Stanislav Fomichev, John Fastabend, Jiri Olsa, David S. Miller,
Hideaki YOSHIFUJI, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv ICMP ping
2022-09-02 23:52 ` YiFei Zhu
2022-09-06 17:16 ` Martin KaFai Lau
@ 2022-09-06 23:52 ` YiFei Zhu
1 sibling, 0 replies; 7+ messages in thread
From: YiFei Zhu @ 2022-09-06 23:52 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
Stanislav Fomichev, Martin KaFai Lau, John Fastabend, Jiri Olsa,
David S. Miller, Hideaki YOSHIFUJI, David Ahern, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-09-06 23:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-01 19:15 [PATCH bpf-next 0/2] cgroup/connect{4,6} programs for unprivileged ICMP ping YiFei Zhu
2022-09-01 19:15 ` [PATCH bpf-next 1/2] bpf: Invoke " YiFei Zhu
2022-09-01 19:15 ` [PATCH bpf-next 2/2] selftests/bpf: Ensure cgroup/connect{4,6} programs can bind unpriv " YiFei Zhu
2022-09-02 5:55 ` Martin KaFai Lau
2022-09-02 23:52 ` YiFei Zhu
2022-09-06 17:16 ` Martin KaFai Lau
2022-09-06 23:52 ` YiFei Zhu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).