[PATCH bpf-next v6 1/5] bpf: Be less specific about socket cookies guarantees

[PATCH bpf-next v6 1/5] bpf: Be less specific about socket cookies guarantees

[Florent Revest]

Since "92acdc58ab11 bpf, net: Rework cookie generator as per-cpu one"
socket cookies are not guaranteed to be non-decreasing. The
bpf_get_socket_cookie helper descriptions are currently specifying that
cookies are non-decreasing but we don't want users to rely on that.

Reported-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Florent Revest <revest@chromium.org>
Acked-by: KP Singh <kpsingh@kernel.org>
---
 include/uapi/linux/bpf.h       | 8 ++++----
 tools/include/uapi/linux/bpf.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c001766adcbc..0b735c2729b2 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1656,22 +1656,22 @@ union bpf_attr {
  * 		networking traffic statistics as it provides a global socket
  * 		identifier that can be assumed unique.
  * 	Return
- * 		A 8-byte long non-decreasing number on success, or 0 if the
- * 		socket field is missing inside *skb*.
+ * 		A 8-byte long unique number on success, or 0 if the socket
+ * 		field is missing inside *skb*.
  *
  * u64 bpf_get_socket_cookie(struct bpf_sock_addr *ctx)
  * 	Description
  * 		Equivalent to bpf_get_socket_cookie() helper that accepts
  * 		*skb*, but gets socket from **struct bpf_sock_addr** context.
  * 	Return
- * 		A 8-byte long non-decreasing number.
+ * 		A 8-byte long unique number.
  *
  * u64 bpf_get_socket_cookie(struct bpf_sock_ops *ctx)
  * 	Description
  * 		Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
  * 		*skb*, but gets socket from **struct bpf_sock_ops** context.
  * 	Return
- * 		A 8-byte long non-decreasing number.
+ * 		A 8-byte long unique number.
  *
  * u32 bpf_get_socket_uid(struct sk_buff *skb)
  * 	Return
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index c001766adcbc..0b735c2729b2 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1656,22 +1656,22 @@ union bpf_attr {
  * 		networking traffic statistics as it provides a global socket
  * 		identifier that can be assumed unique.
  * 	Return
- * 		A 8-byte long non-decreasing number on success, or 0 if the
- * 		socket field is missing inside *skb*.
+ * 		A 8-byte long unique number on success, or 0 if the socket
+ * 		field is missing inside *skb*.
  *
  * u64 bpf_get_socket_cookie(struct bpf_sock_addr *ctx)
  * 	Description
  * 		Equivalent to bpf_get_socket_cookie() helper that accepts
  * 		*skb*, but gets socket from **struct bpf_sock_addr** context.
  * 	Return
- * 		A 8-byte long non-decreasing number.
+ * 		A 8-byte long unique number.
  *
  * u64 bpf_get_socket_cookie(struct bpf_sock_ops *ctx)
  * 	Description
  * 		Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
  * 		*skb*, but gets socket from **struct bpf_sock_ops** context.
  * 	Return
- * 		A 8-byte long non-decreasing number.
+ * 		A 8-byte long unique number.
  *
  * u32 bpf_get_socket_uid(struct sk_buff *skb)
  * 	Return
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH bpf-next v6 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs

[Florent Revest]

This needs a new helper that:
- can work in a sleepable context (using sock_gen_cookie)
- takes a struct sock pointer and checks that it's not NULL

Signed-off-by: Florent Revest <revest@chromium.org>
Acked-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       |  8 ++++++++
 kernel/trace/bpf_trace.c       |  2 ++
 net/core/filter.c              | 12 ++++++++++++
 tools/include/uapi/linux/bpf.h |  8 ++++++++
 5 files changed, 31 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 1aac2af12fed..26219465e1f7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
 extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
 extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
 extern const struct bpf_func_proto bpf_sock_from_file_proto;
+extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
 
 const struct bpf_func_proto *bpf_tracing_func_proto(
 	enum bpf_func_id func_id, const struct bpf_prog *prog);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 0b735c2729b2..5855c398d685 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1673,6 +1673,14 @@ union bpf_attr {
  * 	Return
  * 		A 8-byte long unique number.
  *
+ * u64 bpf_get_socket_cookie(void *sk)
+ * 	Description
+ * 		Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
+ * 		*sk*, but gets socket from a BTF **struct sock**. This helper
+ * 		also works for sleepable programs.
+ * 	Return
+ * 		A 8-byte long unique number or 0 if *sk* is NULL.
+ *
  * u32 bpf_get_socket_uid(struct sk_buff *skb)
  * 	Return
  * 		The owner UID of the socket associated to *skb*. If the socket
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 6c0018abe68a..845b2168e006 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1760,6 +1760,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_sk_storage_delete_tracing_proto;
 	case BPF_FUNC_sock_from_file:
 		return &bpf_sock_from_file_proto;
+	case BPF_FUNC_get_socket_cookie:
+		return &bpf_get_socket_ptr_cookie_proto;
 #endif
 	case BPF_FUNC_seq_printf:
 		return prog->expected_attach_type == BPF_TRACE_ITER ?
diff --git a/net/core/filter.c b/net/core/filter.c
index 9ab94e90d660..606e2b6115ed 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4631,6 +4631,18 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk)
+{
+	return sk ? sock_gen_cookie(sk) : 0;
+}
+
+const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
+	.func		= bpf_get_socket_ptr_cookie,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON,
+};
+
 BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
 {
 	return __sock_gen_cookie(ctx->sk);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 0b735c2729b2..5855c398d685 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1673,6 +1673,14 @@ union bpf_attr {
  * 	Return
  * 		A 8-byte long unique number.
  *
+ * u64 bpf_get_socket_cookie(void *sk)
+ * 	Description
+ * 		Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
+ * 		*sk*, but gets socket from a BTF **struct sock**. This helper
+ * 		also works for sleepable programs.
+ * 	Return
+ * 		A 8-byte long unique number or 0 if *sk* is NULL.
+ *
  * u32 bpf_get_socket_uid(struct sk_buff *skb)
  * 	Return
  * 		The owner UID of the socket associated to *skb*. If the socket
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH bpf-next v6 3/5] selftests/bpf: Integrate the socket_cookie test to test_progs

[Florent Revest]

Currently, the selftest for the BPF socket_cookie helpers is built and
run independently from test_progs. It's easy to forget and hard to
maintain.

This patch moves the socket cookies test into prog_tests/ and vastly
simplifies its logic by:
- rewriting the loading code with BPF skeletons
- rewriting the server/client code with network helpers
- rewriting the cgroup code with test__join_cgroup
- rewriting the error handling code with CHECKs

Signed-off-by: Florent Revest <revest@chromium.org>
Acked-by: KP Singh <kpsingh@kernel.org>
---
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../selftests/bpf/prog_tests/socket_cookie.c  |  71 ++++++
 .../selftests/bpf/progs/socket_cookie_prog.c  |   2 -
 .../selftests/bpf/test_socket_cookie.c        | 208 ------------------
 5 files changed, 72 insertions(+), 213 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/socket_cookie.c
 delete mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 9abca0616ec0..c0c48fdb9ac1 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -17,7 +17,6 @@ test_sockmap
 test_lirc_mode2_user
 get_cgroup_id_user
 test_skb_cgroup_id_user
-test_socket_cookie
 test_cgroup_storage
 test_flow_dissector
 flow_dissector_load
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 63d6288e419c..af00fe3b7fb9 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -33,7 +33,7 @@ LDLIBS += -lcap -lelf -lz -lrt -lpthread
 # Order correspond to 'make run_tests' order
 TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
 	test_verifier_log test_dev_cgroup \
-	test_sock test_sockmap get_cgroup_id_user test_socket_cookie \
+	test_sock test_sockmap get_cgroup_id_user \
 	test_cgroup_storage \
 	test_netcnt test_tcpnotify_user test_sysctl \
 	test_progs-no_alu32
@@ -187,7 +187,6 @@ $(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
 $(OUTPUT)/test_skb_cgroup_id_user: cgroup_helpers.c
 $(OUTPUT)/test_sock: cgroup_helpers.c
 $(OUTPUT)/test_sock_addr: cgroup_helpers.c
-$(OUTPUT)/test_socket_cookie: cgroup_helpers.c
 $(OUTPUT)/test_sockmap: cgroup_helpers.c
 $(OUTPUT)/test_tcpnotify_user: cgroup_helpers.c trace_helpers.c
 $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
new file mode 100644
index 000000000000..e12a31d3752c
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Google LLC.
+// Copyright (c) 2018 Facebook
+
+#include <test_progs.h>
+#include "socket_cookie_prog.skel.h"
+#include "network_helpers.h"
+
+static int duration;
+
+struct socket_cookie {
+	__u64 cookie_key;
+	__u32 cookie_value;
+};
+
+void test_socket_cookie(void)
+{
+	int server_fd = 0, client_fd = 0, cgroup_fd = 0, err = 0;
+	socklen_t addr_len = sizeof(struct sockaddr_in6);
+	struct socket_cookie_prog *skel;
+	__u32 cookie_expected_value;
+	struct sockaddr_in6 addr;
+	struct socket_cookie val;
+
+	skel = socket_cookie_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	cgroup_fd = test__join_cgroup("/socket_cookie");
+	if (CHECK(cgroup_fd < 0, "join_cgroup", "cgroup creation failed\n"))
+		goto out;
+
+	skel->links.set_cookie = bpf_program__attach_cgroup(
+		skel->progs.set_cookie, cgroup_fd);
+	if (!ASSERT_OK_PTR(skel->links.set_cookie, "prog_attach"))
+		goto close_cgroup_fd;
+
+	skel->links.update_cookie = bpf_program__attach_cgroup(
+		skel->progs.update_cookie, cgroup_fd);
+	if (!ASSERT_OK_PTR(skel->links.update_cookie, "prog_attach"))
+		goto close_cgroup_fd;
+
+	server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
+	if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
+		goto close_cgroup_fd;
+
+	client_fd = connect_to_fd(server_fd, 0);
+	if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
+		goto close_server_fd;
+
+	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.socket_cookies),
+				  &client_fd, &val);
+	if (!ASSERT_OK(err, "map_lookup(socket_cookies)"))
+		goto close_client_fd;
+
+	err = getsockname(client_fd, (struct sockaddr *)&addr, &addr_len);
+	if (!ASSERT_OK(err, "getsockname"))
+		goto close_client_fd;
+
+	cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
+	ASSERT_EQ(val.cookie_value, cookie_expected_value, "cookie_value");
+
+close_client_fd:
+	close(client_fd);
+close_server_fd:
+	close(server_fd);
+close_cgroup_fd:
+	close(cgroup_fd);
+out:
+	socket_cookie_prog__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
index 0cb5656a22b0..81e84be6f86d 100644
--- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
+++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
@@ -65,6 +65,4 @@ int update_cookie(struct bpf_sock_ops *ctx)
 	return 1;
 }
 
-int _version SEC("version") = 1;
-
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.c
deleted file mode 100644
index ca7ca87e91aa..000000000000
--- a/tools/testing/selftests/bpf/test_socket_cookie.c
+++ /dev/null
@@ -1,208 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2018 Facebook
-
-#include <string.h>
-#include <unistd.h>
-
-#include <arpa/inet.h>
-#include <netinet/in.h>
-#include <sys/types.h>
-#include <sys/socket.h>
-
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include "bpf_rlimit.h"
-#include "cgroup_helpers.h"
-
-#define CG_PATH			"/foo"
-#define SOCKET_COOKIE_PROG	"./socket_cookie_prog.o"
-
-struct socket_cookie {
-	__u64 cookie_key;
-	__u32 cookie_value;
-};
-
-static int start_server(void)
-{
-	struct sockaddr_in6 addr;
-	int fd;
-
-	fd = socket(AF_INET6, SOCK_STREAM, 0);
-	if (fd == -1) {
-		log_err("Failed to create server socket");
-		goto out;
-	}
-
-	memset(&addr, 0, sizeof(addr));
-	addr.sin6_family = AF_INET6;
-	addr.sin6_addr = in6addr_loopback;
-	addr.sin6_port = 0;
-
-	if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
-		log_err("Failed to bind server socket");
-		goto close_out;
-	}
-
-	if (listen(fd, 128) == -1) {
-		log_err("Failed to listen on server socket");
-		goto close_out;
-	}
-
-	goto out;
-
-close_out:
-	close(fd);
-	fd = -1;
-out:
-	return fd;
-}
-
-static int connect_to_server(int server_fd)
-{
-	struct sockaddr_storage addr;
-	socklen_t len = sizeof(addr);
-	int fd;
-
-	fd = socket(AF_INET6, SOCK_STREAM, 0);
-	if (fd == -1) {
-		log_err("Failed to create client socket");
-		goto out;
-	}
-
-	if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
-		log_err("Failed to get server addr");
-		goto close_out;
-	}
-
-	if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
-		log_err("Fail to connect to server");
-		goto close_out;
-	}
-
-	goto out;
-
-close_out:
-	close(fd);
-	fd = -1;
-out:
-	return fd;
-}
-
-static int validate_map(struct bpf_map *map, int client_fd)
-{
-	__u32 cookie_expected_value;
-	struct sockaddr_in6 addr;
-	socklen_t len = sizeof(addr);
-	struct socket_cookie val;
-	int err = 0;
-	int map_fd;
-
-	if (!map) {
-		log_err("Map not found in BPF object");
-		goto err;
-	}
-
-	map_fd = bpf_map__fd(map);
-
-	err = bpf_map_lookup_elem(map_fd, &client_fd, &val);
-
-	err = getsockname(client_fd, (struct sockaddr *)&addr, &len);
-	if (err) {
-		log_err("Can't get client local addr");
-		goto out;
-	}
-
-	cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
-	if (val.cookie_value != cookie_expected_value) {
-		log_err("Unexpected value in map: %x != %x", val.cookie_value,
-			cookie_expected_value);
-		goto err;
-	}
-
-	goto out;
-err:
-	err = -1;
-out:
-	return err;
-}
-
-static int run_test(int cgfd)
-{
-	enum bpf_attach_type attach_type;
-	struct bpf_prog_load_attr attr;
-	struct bpf_program *prog;
-	struct bpf_object *pobj;
-	const char *prog_name;
-	int server_fd = -1;
-	int client_fd = -1;
-	int prog_fd = -1;
-	int err = 0;
-
-	memset(&attr, 0, sizeof(attr));
-	attr.file = SOCKET_COOKIE_PROG;
-	attr.prog_type = BPF_PROG_TYPE_UNSPEC;
-	attr.prog_flags = BPF_F_TEST_RND_HI32;
-
-	err = bpf_prog_load_xattr(&attr, &pobj, &prog_fd);
-	if (err) {
-		log_err("Failed to load %s", attr.file);
-		goto out;
-	}
-
-	bpf_object__for_each_program(prog, pobj) {
-		prog_name = bpf_program__section_name(prog);
-
-		if (libbpf_attach_type_by_name(prog_name, &attach_type))
-			goto err;
-
-		err = bpf_prog_attach(bpf_program__fd(prog), cgfd, attach_type,
-				      BPF_F_ALLOW_OVERRIDE);
-		if (err) {
-			log_err("Failed to attach prog %s", prog_name);
-			goto out;
-		}
-	}
-
-	server_fd = start_server();
-	if (server_fd == -1)
-		goto err;
-
-	client_fd = connect_to_server(server_fd);
-	if (client_fd == -1)
-		goto err;
-
-	if (validate_map(bpf_map__next(NULL, pobj), client_fd))
-		goto err;
-
-	goto out;
-err:
-	err = -1;
-out:
-	close(client_fd);
-	close(server_fd);
-	bpf_object__close(pobj);
-	printf("%s\n", err ? "FAILED" : "PASSED");
-	return err;
-}
-
-int main(int argc, char **argv)
-{
-	int cgfd = -1;
-	int err = 0;
-
-	cgfd = cgroup_setup_and_join(CG_PATH);
-	if (cgfd < 0)
-		goto err;
-
-	if (run_test(cgfd))
-		goto err;
-
-	goto out;
-err:
-	err = -1;
-out:
-	close(cgfd);
-	cleanup_cgroup_environment();
-	return err;
-}
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH bpf-next v6 4/5] selftests/bpf: Use vmlinux.h in socket_cookie_prog.c

[Florent Revest]

When migrating from the bpf.h's to the vmlinux.h's definition of struct
bps_sock, an interesting LLVM behavior happened. LLVM started producing
two fetches of ctx->sk in the sockops program this means that the
verifier could not keep track of the NULL-check on ctx->sk. Therefore,
we need to extract ctx->sk in a variable before checking and
dereferencing it.

Acked-by: KP Singh <kpsingh@kernel.org>
Signed-off-by: Florent Revest <revest@chromium.org>
---
 .../testing/selftests/bpf/progs/socket_cookie_prog.c  | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
index 81e84be6f86d..fbd5eaf39720 100644
--- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
+++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
@@ -1,12 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2018 Facebook
 
-#include <linux/bpf.h>
-#include <sys/socket.h>
+#include "vmlinux.h"
 
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 
+#define AF_INET6 10
+
 struct socket_cookie {
 	__u64 cookie_key;
 	__u32 cookie_value;
@@ -41,7 +42,7 @@ int set_cookie(struct bpf_sock_addr *ctx)
 SEC("sockops")
 int update_cookie(struct bpf_sock_ops *ctx)
 {
-	struct bpf_sock *sk;
+	struct bpf_sock *sk = ctx->sk;
 	struct socket_cookie *p;
 
 	if (ctx->family != AF_INET6)
@@ -50,10 +51,10 @@ int update_cookie(struct bpf_sock_ops *ctx)
 	if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
 		return 1;
 
-	if (!ctx->sk)
+	if (!sk)
 		return 1;
 
-	p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
+	p = bpf_sk_storage_get(&socket_cookies, sk, 0, 0);
 	if (!p)
 		return 1;
 
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH bpf-next v6 5/5] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie

[Florent Revest]

This builds up on the existing socket cookie test which checks whether
the bpf_get_socket_cookie helpers provide the same value in
cgroup/connect6 and sockops programs for a socket created by the
userspace part of the test.

Instead of having an update_cookie sockops program tag a socket local
storage with 0xFF, this uses both an update_cookie_sockops program and
an update_cookie_tracing program which succesively tag the socket with
0x0F and then 0xF0.

Signed-off-by: Florent Revest <revest@chromium.org>
Acked-by: KP Singh <kpsingh@kernel.org>
---
 .../selftests/bpf/prog_tests/socket_cookie.c  | 11 ++++--
 .../selftests/bpf/progs/socket_cookie_prog.c  | 36 +++++++++++++++++--
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
index e12a31d3752c..232db28dde18 100644
--- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
@@ -35,9 +35,14 @@ void test_socket_cookie(void)
 	if (!ASSERT_OK_PTR(skel->links.set_cookie, "prog_attach"))
 		goto close_cgroup_fd;
 
-	skel->links.update_cookie = bpf_program__attach_cgroup(
-		skel->progs.update_cookie, cgroup_fd);
-	if (!ASSERT_OK_PTR(skel->links.update_cookie, "prog_attach"))
+	skel->links.update_cookie_sockops = bpf_program__attach_cgroup(
+		skel->progs.update_cookie_sockops, cgroup_fd);
+	if (!ASSERT_OK_PTR(skel->links.update_cookie_sockops, "prog_attach"))
+		goto close_cgroup_fd;
+
+	skel->links.update_cookie_tracing = bpf_program__attach(
+		skel->progs.update_cookie_tracing);
+	if (!ASSERT_OK_PTR(skel->links.update_cookie_tracing, "prog_attach"))
 		goto close_cgroup_fd;
 
 	server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
index fbd5eaf39720..35630a5aaf5f 100644
--- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
+++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
@@ -5,6 +5,7 @@
 
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
 
 #define AF_INET6 10
 
@@ -20,6 +21,14 @@ struct {
 	__type(value, struct socket_cookie);
 } socket_cookies SEC(".maps");
 
+/*
+ * These three programs get executed in a row on connect() syscalls. The
+ * userspace side of the test creates a client socket, issues a connect() on it
+ * and then checks that the local storage associated with this socket has:
+ * cookie_value == local_port << 8 | 0xFF
+ * The different parts of this cookie_value are appended by those hooks if they
+ * all agree on the output of bpf_get_socket_cookie().
+ */
 SEC("cgroup/connect6")
 int set_cookie(struct bpf_sock_addr *ctx)
 {
@@ -33,14 +42,14 @@ int set_cookie(struct bpf_sock_addr *ctx)
 	if (!p)
 		return 1;
 
-	p->cookie_value = 0xFF;
+	p->cookie_value = 0xF;
 	p->cookie_key = bpf_get_socket_cookie(ctx);
 
 	return 1;
 }
 
 SEC("sockops")
-int update_cookie(struct bpf_sock_ops *ctx)
+int update_cookie_sockops(struct bpf_sock_ops *ctx)
 {
 	struct bpf_sock *sk = ctx->sk;
 	struct socket_cookie *p;
@@ -61,9 +70,30 @@ int update_cookie(struct bpf_sock_ops *ctx)
 	if (p->cookie_key != bpf_get_socket_cookie(ctx))
 		return 1;
 
-	p->cookie_value = (ctx->local_port << 8) | p->cookie_value;
+	p->cookie_value |= (ctx->local_port << 8);
 
 	return 1;
 }
 
+SEC("fexit/inet_stream_connect")
+int BPF_PROG(update_cookie_tracing, struct socket *sock,
+	     struct sockaddr *uaddr, int addr_len, int flags)
+{
+	struct socket_cookie *p;
+
+	if (uaddr->sa_family != AF_INET6)
+		return 0;
+
+	p = bpf_sk_storage_get(&socket_cookies, sock->sk, 0, 0);
+	if (!p)
+		return 0;
+
+	if (p->cookie_key != bpf_get_socket_cookie(sock->sk))
+		return 0;
+
+	p->cookie_value |= 0xF0;
+
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.30.0.280.ga3ce27912f-goog

Re: [PATCH bpf-next v6 4/5] selftests/bpf: Use vmlinux.h in socket_cookie_prog.c

[Andrii Nakryiko]

On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote:
>
> When migrating from the bpf.h's to the vmlinux.h's definition of struct
> bps_sock, an interesting LLVM behavior happened. LLVM started producing
> two fetches of ctx->sk in the sockops program this means that the
> verifier could not keep track of the NULL-check on ctx->sk. Therefore,
> we need to extract ctx->sk in a variable before checking and
> dereferencing it.
>
> Acked-by: KP Singh <kpsingh@kernel.org>
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>


>  .../testing/selftests/bpf/progs/socket_cookie_prog.c  | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
> index 81e84be6f86d..fbd5eaf39720 100644
> --- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
> +++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
> @@ -1,12 +1,13 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (c) 2018 Facebook
>
> -#include <linux/bpf.h>
> -#include <sys/socket.h>
> +#include "vmlinux.h"
>
>  #include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_endian.h>
>
> +#define AF_INET6 10
> +
>  struct socket_cookie {
>         __u64 cookie_key;
>         __u32 cookie_value;
> @@ -41,7 +42,7 @@ int set_cookie(struct bpf_sock_addr *ctx)
>  SEC("sockops")
>  int update_cookie(struct bpf_sock_ops *ctx)
>  {
> -       struct bpf_sock *sk;
> +       struct bpf_sock *sk = ctx->sk;
>         struct socket_cookie *p;
>
>         if (ctx->family != AF_INET6)
> @@ -50,10 +51,10 @@ int update_cookie(struct bpf_sock_ops *ctx)
>         if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
>                 return 1;
>
> -       if (!ctx->sk)
> +       if (!sk)
>                 return 1;
>
> -       p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> +       p = bpf_sk_storage_get(&socket_cookies, sk, 0, 0);
>         if (!p)
>                 return 1;
>
> --
> 2.30.0.280.ga3ce27912f-goog
>

Re: [PATCH bpf-next v6 3/5] selftests/bpf: Integrate the socket_cookie test to test_progs

[Andrii Nakryiko]

On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote:
>
> Currently, the selftest for the BPF socket_cookie helpers is built and
> run independently from test_progs. It's easy to forget and hard to
> maintain.
>
> This patch moves the socket cookies test into prog_tests/ and vastly
> simplifies its logic by:
> - rewriting the loading code with BPF skeletons
> - rewriting the server/client code with network helpers
> - rewriting the cgroup code with test__join_cgroup
> - rewriting the error handling code with CHECKs
>
> Signed-off-by: Florent Revest <revest@chromium.org>
> Acked-by: KP Singh <kpsingh@kernel.org>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/testing/selftests/bpf/.gitignore        |   1 -
>  tools/testing/selftests/bpf/Makefile          |   3 +-
>  .../selftests/bpf/prog_tests/socket_cookie.c  |  71 ++++++
>  .../selftests/bpf/progs/socket_cookie_prog.c  |   2 -
>  .../selftests/bpf/test_socket_cookie.c        | 208 ------------------
>  5 files changed, 72 insertions(+), 213 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/socket_cookie.c
>  delete mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
>

[...]

Re: [PATCH bpf-next v6 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs

[Andrii Nakryiko]

On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote:
>
> This needs a new helper that:
> - can work in a sleepable context (using sock_gen_cookie)
> - takes a struct sock pointer and checks that it's not NULL
>
> Signed-off-by: Florent Revest <revest@chromium.org>
> Acked-by: KP Singh <kpsingh@kernel.org>
> ---
>  include/linux/bpf.h            |  1 +
>  include/uapi/linux/bpf.h       |  8 ++++++++
>  kernel/trace/bpf_trace.c       |  2 ++
>  net/core/filter.c              | 12 ++++++++++++
>  tools/include/uapi/linux/bpf.h |  8 ++++++++
>  5 files changed, 31 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 1aac2af12fed..26219465e1f7 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
>  extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
>  extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
>  extern const struct bpf_func_proto bpf_sock_from_file_proto;
> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
>
>  const struct bpf_func_proto *bpf_tracing_func_proto(
>         enum bpf_func_id func_id, const struct bpf_prog *prog);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0b735c2729b2..5855c398d685 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1673,6 +1673,14 @@ union bpf_attr {
>   *     Return
>   *             A 8-byte long unique number.
>   *
> + * u64 bpf_get_socket_cookie(void *sk)

should the type be `struct sock *` then?

> + *     Description
> + *             Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
> + *             *sk*, but gets socket from a BTF **struct sock**. This helper
> + *             also works for sleepable programs.
> + *     Return
> + *             A 8-byte long unique number or 0 if *sk* is NULL.
> + *
>   * u32 bpf_get_socket_uid(struct sk_buff *skb)
>   *     Return
>   *             The owner UID of the socket associated to *skb*. If the socket
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 6c0018abe68a..845b2168e006 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1760,6 +1760,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_sk_storage_delete_tracing_proto;
>         case BPF_FUNC_sock_from_file:
>                 return &bpf_sock_from_file_proto;
> +       case BPF_FUNC_get_socket_cookie:
> +               return &bpf_get_socket_ptr_cookie_proto;
>  #endif
>         case BPF_FUNC_seq_printf:
>                 return prog->expected_attach_type == BPF_TRACE_ITER ?
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9ab94e90d660..606e2b6115ed 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4631,6 +4631,18 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
>         .arg1_type      = ARG_PTR_TO_CTX,
>  };
>
> +BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk)
> +{
> +       return sk ? sock_gen_cookie(sk) : 0;
> +}
> +
> +const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
> +       .func           = bpf_get_socket_ptr_cookie,
> +       .gpl_only       = false,
> +       .ret_type       = RET_INTEGER,
> +       .arg1_type      = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> +};
> +
>  BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
>  {
>         return __sock_gen_cookie(ctx->sk);
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 0b735c2729b2..5855c398d685 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1673,6 +1673,14 @@ union bpf_attr {
>   *     Return
>   *             A 8-byte long unique number.
>   *
> + * u64 bpf_get_socket_cookie(void *sk)
> + *     Description
> + *             Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
> + *             *sk*, but gets socket from a BTF **struct sock**. This helper
> + *             also works for sleepable programs.
> + *     Return
> + *             A 8-byte long unique number or 0 if *sk* is NULL.
> + *
>   * u32 bpf_get_socket_uid(struct sk_buff *skb)
>   *     Return
>   *             The owner UID of the socket associated to *skb*. If the socket
> --
> 2.30.0.280.ga3ce27912f-goog
>

Re: [PATCH bpf-next v6 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs

[Florent Revest]

On Wed, Jan 27, 2021 at 10:01 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote:
> >
> > This needs a new helper that:
> > - can work in a sleepable context (using sock_gen_cookie)
> > - takes a struct sock pointer and checks that it's not NULL
> >
> > Signed-off-by: Florent Revest <revest@chromium.org>
> > Acked-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  include/linux/bpf.h            |  1 +
> >  include/uapi/linux/bpf.h       |  8 ++++++++
> >  kernel/trace/bpf_trace.c       |  2 ++
> >  net/core/filter.c              | 12 ++++++++++++
> >  tools/include/uapi/linux/bpf.h |  8 ++++++++
> >  5 files changed, 31 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 1aac2af12fed..26219465e1f7 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
> >  extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
> >  extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
> >  extern const struct bpf_func_proto bpf_sock_from_file_proto;
> > +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
> >
> >  const struct bpf_func_proto *bpf_tracing_func_proto(
> >         enum bpf_func_id func_id, const struct bpf_prog *prog);
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 0b735c2729b2..5855c398d685 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1673,6 +1673,14 @@ union bpf_attr {
> >   *     Return
> >   *             A 8-byte long unique number.
> >   *
> > + * u64 bpf_get_socket_cookie(void *sk)
>
> should the type be `struct sock *` then?

Oh, absolutely. :) Thank you Andrii

Re: [PATCH bpf-next v6 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs

[Florent Revest]

On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 1/29/21 11:57 AM, Daniel Borkmann wrote:
> > On 1/27/21 10:01 PM, Andrii Nakryiko wrote:
> >> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote:
> >>>
> >>> This needs a new helper that:
> >>> - can work in a sleepable context (using sock_gen_cookie)
> >>> - takes a struct sock pointer and checks that it's not NULL
> >>>
> >>> Signed-off-by: Florent Revest <revest@chromium.org>
> >>> Acked-by: KP Singh <kpsingh@kernel.org>
> >>> ---
> >>>   include/linux/bpf.h            |  1 +
> >>>   include/uapi/linux/bpf.h       |  8 ++++++++
> >>>   kernel/trace/bpf_trace.c       |  2 ++
> >>>   net/core/filter.c              | 12 ++++++++++++
> >>>   tools/include/uapi/linux/bpf.h |  8 ++++++++
> >>>   5 files changed, 31 insertions(+)
> >>>
> >>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >>> index 1aac2af12fed..26219465e1f7 100644
> >>> --- a/include/linux/bpf.h
> >>> +++ b/include/linux/bpf.h
> >>> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
> >>>   extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
> >>>   extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
> >>>   extern const struct bpf_func_proto bpf_sock_from_file_proto;
> >>> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
> >>>
> >>>   const struct bpf_func_proto *bpf_tracing_func_proto(
> >>>          enum bpf_func_id func_id, const struct bpf_prog *prog);
> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>> index 0b735c2729b2..5855c398d685 100644
> >>> --- a/include/uapi/linux/bpf.h
> >>> +++ b/include/uapi/linux/bpf.h
> >>> @@ -1673,6 +1673,14 @@ union bpf_attr {
> >>>    *     Return
> >>>    *             A 8-byte long unique number.
> >>>    *
> >>> + * u64 bpf_get_socket_cookie(void *sk)
> >>
> >> should the type be `struct sock *` then?
> >
> > Checking libbpf's generated bpf_helper_defs.h it generates:
> >
> > /*
> >   * bpf_get_socket_cookie
> >   *
> >   *      If the **struct sk_buff** pointed by *skb* has a known socket,
> >   *      retrieve the cookie (generated by the kernel) of this socket.
> >   *      If no cookie has been set yet, generate a new cookie. Once
> >   *      generated, the socket cookie remains stable for the life of the
> >   *      socket. This helper can be useful for monitoring per socket
> >   *      networking traffic statistics as it provides a global socket
> >   *      identifier that can be assumed unique.
> >   *
> >   * Returns
> >   *      A 8-byte long non-decreasing number on success, or 0 if the
> >   *      socket field is missing inside *skb*.
> >   */
> > static __u64 (*bpf_get_socket_cookie)(void *ctx) = (void *) 46;
> >
> > So in terms of helper comment it's picking up the description from the
> > `u64 bpf_get_socket_cookie(struct sk_buff *skb)` signature. With that
> > in mind it would likely make sense to add the actual `struct sock *` type
> > to the comment to make it more clear in here.
>
> One thought that still came to mind when looking over the series again, do
> we need to blacklist certain functions from bpf_get_socket_cookie() under
> tracing e.g. when attaching to, say fexit? For example, if sk_prot_free()
> would be temporary uninlined/exported for testing and bpf_get_socket_cookie()
> was invoked from a prog upon fexit where sock was already passed back to
> allocator, I presume there's risk of mem corruption, no?

Mh, this is interesting. I can try to add a deny list in v7 but I'm
not sure whether I'll be able to catch them all. I'm assuming that
__sk_destruct, sk_destruct, __sk_free, sk_free would be other
problematic functions but potentially there would be more.

Re: [PATCH bpf-next v6 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs

[Daniel Borkmann]

On 1/30/21 12:45 PM, Florent Revest wrote:
> On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 1/29/21 11:57 AM, Daniel Borkmann wrote:
>>> On 1/27/21 10:01 PM, Andrii Nakryiko wrote:
>>>> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote:
>>>>>
>>>>> This needs a new helper that:
>>>>> - can work in a sleepable context (using sock_gen_cookie)
>>>>> - takes a struct sock pointer and checks that it's not NULL
>>>>>
>>>>> Signed-off-by: Florent Revest <revest@chromium.org>
>>>>> Acked-by: KP Singh <kpsingh@kernel.org>
>>>>> ---
>>>>>    include/linux/bpf.h            |  1 +
>>>>>    include/uapi/linux/bpf.h       |  8 ++++++++
>>>>>    kernel/trace/bpf_trace.c       |  2 ++
>>>>>    net/core/filter.c              | 12 ++++++++++++
>>>>>    tools/include/uapi/linux/bpf.h |  8 ++++++++
>>>>>    5 files changed, 31 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>>> index 1aac2af12fed..26219465e1f7 100644
>>>>> --- a/include/linux/bpf.h
>>>>> +++ b/include/linux/bpf.h
>>>>> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
>>>>>    extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
>>>>>    extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
>>>>>    extern const struct bpf_func_proto bpf_sock_from_file_proto;
>>>>> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
>>>>>
>>>>>    const struct bpf_func_proto *bpf_tracing_func_proto(
>>>>>           enum bpf_func_id func_id, const struct bpf_prog *prog);
>>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>>> index 0b735c2729b2..5855c398d685 100644
>>>>> --- a/include/uapi/linux/bpf.h
>>>>> +++ b/include/uapi/linux/bpf.h
>>>>> @@ -1673,6 +1673,14 @@ union bpf_attr {
>>>>>     *     Return
>>>>>     *             A 8-byte long unique number.
>>>>>     *
>>>>> + * u64 bpf_get_socket_cookie(void *sk)
>>>>
>>>> should the type be `struct sock *` then?
>>>
>>> Checking libbpf's generated bpf_helper_defs.h it generates:
>>>
>>> /*
>>>    * bpf_get_socket_cookie
>>>    *
>>>    *      If the **struct sk_buff** pointed by *skb* has a known socket,
>>>    *      retrieve the cookie (generated by the kernel) of this socket.
>>>    *      If no cookie has been set yet, generate a new cookie. Once
>>>    *      generated, the socket cookie remains stable for the life of the
>>>    *      socket. This helper can be useful for monitoring per socket
>>>    *      networking traffic statistics as it provides a global socket
>>>    *      identifier that can be assumed unique.
>>>    *
>>>    * Returns
>>>    *      A 8-byte long non-decreasing number on success, or 0 if the
>>>    *      socket field is missing inside *skb*.
>>>    */
>>> static __u64 (*bpf_get_socket_cookie)(void *ctx) = (void *) 46;
>>>
>>> So in terms of helper comment it's picking up the description from the
>>> `u64 bpf_get_socket_cookie(struct sk_buff *skb)` signature. With that
>>> in mind it would likely make sense to add the actual `struct sock *` type
>>> to the comment to make it more clear in here.
>>
>> One thought that still came to mind when looking over the series again, do
>> we need to blacklist certain functions from bpf_get_socket_cookie() under
>> tracing e.g. when attaching to, say fexit? For example, if sk_prot_free()
>> would be temporary uninlined/exported for testing and bpf_get_socket_cookie()
>> was invoked from a prog upon fexit where sock was already passed back to
>> allocator, I presume there's risk of mem corruption, no?
> 
> Mh, this is interesting. I can try to add a deny list in v7 but I'm
> not sure whether I'll be able to catch them all. I'm assuming that
> __sk_destruct, sk_destruct, __sk_free, sk_free would be other
> problematic functions but potentially there would be more.

I was just looking at bpf_skb_output() from a7658e1a4164 ("bpf: Check types of
arguments passed into helpers") which afaiu has similar issue, back at the time
this was introduced there was no fentry/fexit but rather raw tp progs, so you
could still safely dump skb this way including for kfree_skb() tp. Presumably if
you bpf_skb_output() at 'fexit/kfree_skb' you might be able to similarly crash
your kernel which I don't think is intentional (also given we go above and beyond
in other areas to avoid crashing or destabilizing e.g. [0] to mention one). Maybe
these should really only be for BPF_TRACE_RAW_TP (or BPF_PROG_TYPE_LSM) where it
can be audited that it's safe to use like a7658e1a4164's original intention ...
or have some sort of function annotation like __acquires/__releases but for tracing
e.g. __frees(skb) where use would then be blocked (not sure iff feasible).

   [0] https://lore.kernel.org/bpf/20210126001219.845816-1-yhs@fb.com/

Re: [PATCH bpf-next v6 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs

[Alexei Starovoitov]

On Mon, Feb 1, 2021 at 2:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 1/30/21 12:45 PM, Florent Revest wrote:
> > On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> On 1/29/21 11:57 AM, Daniel Borkmann wrote:
> >>> On 1/27/21 10:01 PM, Andrii Nakryiko wrote:
> >>>> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote:
> >>>>>
> >>>>> This needs a new helper that:
> >>>>> - can work in a sleepable context (using sock_gen_cookie)
> >>>>> - takes a struct sock pointer and checks that it's not NULL
> >>>>>
> >>>>> Signed-off-by: Florent Revest <revest@chromium.org>
> >>>>> Acked-by: KP Singh <kpsingh@kernel.org>
> >>>>> ---
> >>>>>    include/linux/bpf.h            |  1 +
> >>>>>    include/uapi/linux/bpf.h       |  8 ++++++++
> >>>>>    kernel/trace/bpf_trace.c       |  2 ++
> >>>>>    net/core/filter.c              | 12 ++++++++++++
> >>>>>    tools/include/uapi/linux/bpf.h |  8 ++++++++
> >>>>>    5 files changed, 31 insertions(+)
> >>>>>
> >>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >>>>> index 1aac2af12fed..26219465e1f7 100644
> >>>>> --- a/include/linux/bpf.h
> >>>>> +++ b/include/linux/bpf.h
> >>>>> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
> >>>>>    extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
> >>>>>    extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
> >>>>>    extern const struct bpf_func_proto bpf_sock_from_file_proto;
> >>>>> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
> >>>>>
> >>>>>    const struct bpf_func_proto *bpf_tracing_func_proto(
> >>>>>           enum bpf_func_id func_id, const struct bpf_prog *prog);
> >>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>>>> index 0b735c2729b2..5855c398d685 100644
> >>>>> --- a/include/uapi/linux/bpf.h
> >>>>> +++ b/include/uapi/linux/bpf.h
> >>>>> @@ -1673,6 +1673,14 @@ union bpf_attr {
> >>>>>     *     Return
> >>>>>     *             A 8-byte long unique number.
> >>>>>     *
> >>>>> + * u64 bpf_get_socket_cookie(void *sk)
> >>>>
> >>>> should the type be `struct sock *` then?
> >>>
> >>> Checking libbpf's generated bpf_helper_defs.h it generates:
> >>>
> >>> /*
> >>>    * bpf_get_socket_cookie
> >>>    *
> >>>    *      If the **struct sk_buff** pointed by *skb* has a known socket,
> >>>    *      retrieve the cookie (generated by the kernel) of this socket.
> >>>    *      If no cookie has been set yet, generate a new cookie. Once
> >>>    *      generated, the socket cookie remains stable for the life of the
> >>>    *      socket. This helper can be useful for monitoring per socket
> >>>    *      networking traffic statistics as it provides a global socket
> >>>    *      identifier that can be assumed unique.
> >>>    *
> >>>    * Returns
> >>>    *      A 8-byte long non-decreasing number on success, or 0 if the
> >>>    *      socket field is missing inside *skb*.
> >>>    */
> >>> static __u64 (*bpf_get_socket_cookie)(void *ctx) = (void *) 46;
> >>>
> >>> So in terms of helper comment it's picking up the description from the
> >>> `u64 bpf_get_socket_cookie(struct sk_buff *skb)` signature. With that
> >>> in mind it would likely make sense to add the actual `struct sock *` type
> >>> to the comment to make it more clear in here.
> >>
> >> One thought that still came to mind when looking over the series again, do
> >> we need to blacklist certain functions from bpf_get_socket_cookie() under
> >> tracing e.g. when attaching to, say fexit? For example, if sk_prot_free()
> >> would be temporary uninlined/exported for testing and bpf_get_socket_cookie()
> >> was invoked from a prog upon fexit where sock was already passed back to
> >> allocator, I presume there's risk of mem corruption, no?
> >
> > Mh, this is interesting. I can try to add a deny list in v7 but I'm
> > not sure whether I'll be able to catch them all. I'm assuming that
> > __sk_destruct, sk_destruct, __sk_free, sk_free would be other
> > problematic functions but potentially there would be more.
>
> I was just looking at bpf_skb_output() from a7658e1a4164 ("bpf: Check types of
> arguments passed into helpers") which afaiu has similar issue, back at the time
> this was introduced there was no fentry/fexit but rather raw tp progs, so you
> could still safely dump skb this way including for kfree_skb() tp. Presumably if
> you bpf_skb_output() at 'fexit/kfree_skb' you might be able to similarly crash

the verifier cannot check absolutely everything.
Whitelisting and blacklisting all combinations is not practical.

> your kernel which I don't think is intentional (also given we go above and beyond
> in other areas to avoid crashing or destabilizing e.g. [0] to mention one). Maybe
> these should really only be for BPF_TRACE_RAW_TP (or BPF_PROG_TYPE_LSM) where it
> can be audited that it's safe to use like a7658e1a4164's original intention ...
> or have some sort of function annotation like __acquires/__releases but for tracing
> e.g. __frees(skb) where use would then be blocked (not sure iff feasible).

I think this set is useful and corner cases like fexit in sk_free are
not worth the hassle.
One can install xdp prog that drops all packets. The server is dead at
this point.

Re: [PATCH bpf-next v6 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs

[Florent Revest]

On Mon, Feb 1, 2021 at 11:37 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Feb 1, 2021 at 2:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 1/30/21 12:45 PM, Florent Revest wrote:
> > > On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >> On 1/29/21 11:57 AM, Daniel Borkmann wrote:
> > >>> On 1/27/21 10:01 PM, Andrii Nakryiko wrote:
> > >>>> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote:
> > >>>>>
> > >>>>> This needs a new helper that:
> > >>>>> - can work in a sleepable context (using sock_gen_cookie)
> > >>>>> - takes a struct sock pointer and checks that it's not NULL
> > >>>>>
> > >>>>> Signed-off-by: Florent Revest <revest@chromium.org>
> > >>>>> Acked-by: KP Singh <kpsingh@kernel.org>
> > >>>>> ---
> > >>>>>    include/linux/bpf.h            |  1 +
> > >>>>>    include/uapi/linux/bpf.h       |  8 ++++++++
> > >>>>>    kernel/trace/bpf_trace.c       |  2 ++
> > >>>>>    net/core/filter.c              | 12 ++++++++++++
> > >>>>>    tools/include/uapi/linux/bpf.h |  8 ++++++++
> > >>>>>    5 files changed, 31 insertions(+)
> > >>>>>
> > >>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > >>>>> index 1aac2af12fed..26219465e1f7 100644
> > >>>>> --- a/include/linux/bpf.h
> > >>>>> +++ b/include/linux/bpf.h
> > >>>>> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
> > >>>>>    extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
> > >>>>>    extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
> > >>>>>    extern const struct bpf_func_proto bpf_sock_from_file_proto;
> > >>>>> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
> > >>>>>
> > >>>>>    const struct bpf_func_proto *bpf_tracing_func_proto(
> > >>>>>           enum bpf_func_id func_id, const struct bpf_prog *prog);
> > >>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > >>>>> index 0b735c2729b2..5855c398d685 100644
> > >>>>> --- a/include/uapi/linux/bpf.h
> > >>>>> +++ b/include/uapi/linux/bpf.h
> > >>>>> @@ -1673,6 +1673,14 @@ union bpf_attr {
> > >>>>>     *     Return
> > >>>>>     *             A 8-byte long unique number.
> > >>>>>     *
> > >>>>> + * u64 bpf_get_socket_cookie(void *sk)
> > >>>>
> > >>>> should the type be `struct sock *` then?
> > >>>
> > >>> Checking libbpf's generated bpf_helper_defs.h it generates:
> > >>>
> > >>> /*
> > >>>    * bpf_get_socket_cookie
> > >>>    *
> > >>>    *      If the **struct sk_buff** pointed by *skb* has a known socket,
> > >>>    *      retrieve the cookie (generated by the kernel) of this socket.
> > >>>    *      If no cookie has been set yet, generate a new cookie. Once
> > >>>    *      generated, the socket cookie remains stable for the life of the
> > >>>    *      socket. This helper can be useful for monitoring per socket
> > >>>    *      networking traffic statistics as it provides a global socket
> > >>>    *      identifier that can be assumed unique.
> > >>>    *
> > >>>    * Returns
> > >>>    *      A 8-byte long non-decreasing number on success, or 0 if the
> > >>>    *      socket field is missing inside *skb*.
> > >>>    */
> > >>> static __u64 (*bpf_get_socket_cookie)(void *ctx) = (void *) 46;
> > >>>
> > >>> So in terms of helper comment it's picking up the description from the
> > >>> `u64 bpf_get_socket_cookie(struct sk_buff *skb)` signature. With that
> > >>> in mind it would likely make sense to add the actual `struct sock *` type
> > >>> to the comment to make it more clear in here.
> > >>
> > >> One thought that still came to mind when looking over the series again, do
> > >> we need to blacklist certain functions from bpf_get_socket_cookie() under
> > >> tracing e.g. when attaching to, say fexit? For example, if sk_prot_free()
> > >> would be temporary uninlined/exported for testing and bpf_get_socket_cookie()
> > >> was invoked from a prog upon fexit where sock was already passed back to
> > >> allocator, I presume there's risk of mem corruption, no?
> > >
> > > Mh, this is interesting. I can try to add a deny list in v7 but I'm
> > > not sure whether I'll be able to catch them all. I'm assuming that
> > > __sk_destruct, sk_destruct, __sk_free, sk_free would be other
> > > problematic functions but potentially there would be more.
> >
> > I was just looking at bpf_skb_output() from a7658e1a4164 ("bpf: Check types of
> > arguments passed into helpers") which afaiu has similar issue, back at the time
> > this was introduced there was no fentry/fexit but rather raw tp progs, so you
> > could still safely dump skb this way including for kfree_skb() tp. Presumably if
> > you bpf_skb_output() at 'fexit/kfree_skb' you might be able to similarly crash
>
> the verifier cannot check absolutely everything.
> Whitelisting and blacklisting all combinations is not practical.

Ok, I'm sending a v7 that only changes the signature to take a struct
sock * argument then and I won't be adding an allow or deny list in
this series. :)

> > your kernel which I don't think is intentional (also given we go above and beyond
> > in other areas to avoid crashing or destabilizing e.g. [0] to mention one). Maybe
> > these should really only be for BPF_TRACE_RAW_TP (or BPF_PROG_TYPE_LSM) where it
> > can be audited that it's safe to use like a7658e1a4164's original intention ...
> > or have some sort of function annotation like __acquires/__releases but for tracing
> > e.g. __frees(skb) where use would then be blocked (not sure iff feasible).
>
> I think this set is useful and corner cases like fexit in sk_free are
> not worth the hassle.
> One can install xdp prog that drops all packets. The server is dead at
> this point.