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

[PATCH bpf-next v5 1/4] 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>
---
 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.284.gd98b1dd5eaa7-goog

[PATCH bpf-next v5 2/4] 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>
---
 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.284.gd98b1dd5eaa7-goog

Re: [PATCH bpf-next v5 2/4] bpf: Expose bpf_get_socket_cookie to tracing programs

[KP Singh]

On Tue, Jan 19, 2021 at 5:00 PM 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>

[PATCH bpf-next v5 3/4] 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>
---
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../selftests/bpf/prog_tests/socket_cookie.c  |  82 +++++++
 .../selftests/bpf/progs/socket_cookie_prog.c  |   2 -
 .../selftests/bpf/test_socket_cookie.c        | 208 ------------------
 4 files changed, 83 insertions(+), 212 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/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..53d0c44e7907
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
@@ -0,0 +1,82 @@
+// 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)
+{
+	socklen_t addr_len = sizeof(struct sockaddr_in6);
+	struct bpf_link *set_link, *update_link;
+	int server_fd, client_fd, cgroup_fd;
+	struct socket_cookie_prog *skel;
+	__u32 cookie_expected_value;
+	struct sockaddr_in6 addr;
+	struct socket_cookie val;
+	int err = 0;
+
+	skel = socket_cookie_prog__open_and_load();
+	if (CHECK(!skel, "socket_cookie_prog__open_and_load",
+		  "skeleton open_and_load failed\n"))
+		return;
+
+	cgroup_fd = test__join_cgroup("/socket_cookie");
+	if (CHECK(cgroup_fd < 0, "join_cgroup", "cgroup creation failed\n"))
+		goto destroy_skel;
+
+	set_link = bpf_program__attach_cgroup(skel->progs.set_cookie,
+					      cgroup_fd);
+	if (CHECK(IS_ERR(set_link), "set-link-cg-attach", "err %ld\n",
+		  PTR_ERR(set_link)))
+		goto close_cgroup_fd;
+
+	update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
+						 cgroup_fd);
+	if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
+		  PTR_ERR(update_link)))
+		goto free_set_link;
+
+	server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
+	if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
+		goto free_update_link;
+
+	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 (CHECK(err, "map_lookup", "err %d errno %d\n", err, errno))
+		goto close_client_fd;
+
+	err = getsockname(client_fd, (struct sockaddr *)&addr, &addr_len);
+	if (CHECK(err, "getsockname", "Can't get client local addr\n"))
+		goto close_client_fd;
+
+	cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
+	CHECK(val.cookie_value != cookie_expected_value, "",
+	      "Unexpected value in map: %x != %x\n", val.cookie_value,
+	      cookie_expected_value);
+
+close_client_fd:
+	close(client_fd);
+close_server_fd:
+	close(server_fd);
+free_update_link:
+	bpf_link__destroy(update_link);
+free_set_link:
+	bpf_link__destroy(set_link);
+close_cgroup_fd:
+	close(cgroup_fd);
+destroy_skel:
+	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.284.gd98b1dd5eaa7-goog

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

[KP Singh]

On Tue, Jan 19, 2021 at 5:00 PM 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>

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

[Andrii Nakryiko]

On Tue, Jan 19, 2021 at 8:00 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>
> ---

Few nits below regarding skeleton and ASSERT_xxx usage.

>  tools/testing/selftests/bpf/Makefile          |   3 +-
>  .../selftests/bpf/prog_tests/socket_cookie.c  |  82 +++++++
>  .../selftests/bpf/progs/socket_cookie_prog.c  |   2 -
>  .../selftests/bpf/test_socket_cookie.c        | 208 ------------------

please also update .gitignore

>  4 files changed, 83 insertions(+), 212 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/socket_cookie.c
>  delete mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
>

[...]

> +
> +       skel = socket_cookie_prog__open_and_load();
> +       if (CHECK(!skel, "socket_cookie_prog__open_and_load",
> +                 "skeleton open_and_load failed\n"))

nit: ASSERT_PTR_OK

> +               return;
> +
> +       cgroup_fd = test__join_cgroup("/socket_cookie");
> +       if (CHECK(cgroup_fd < 0, "join_cgroup", "cgroup creation failed\n"))
> +               goto destroy_skel;
> +
> +       set_link = bpf_program__attach_cgroup(skel->progs.set_cookie,
> +                                             cgroup_fd);

you can use skel->links->set_cookie here and it will be auto-destroyed
when the whole skeleton is destroyed. More simplification.

> +       if (CHECK(IS_ERR(set_link), "set-link-cg-attach", "err %ld\n",
> +                 PTR_ERR(set_link)))
> +               goto close_cgroup_fd;
> +
> +       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
> +                                                cgroup_fd);

same as above, no need to maintain your link outside of skeleton


> +       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
> +                 PTR_ERR(update_link)))
> +               goto free_set_link;
> +
> +       server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> +       if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
> +               goto free_update_link;
> +
> +       client_fd = connect_to_fd(server_fd, 0);
> +       if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
> +               goto close_server_fd;

nit: ASSERT_OK is nicer (here and in few other places)

> +
> +       err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.socket_cookies),
> +                                 &client_fd, &val);
> +       if (CHECK(err, "map_lookup", "err %d errno %d\n", err, errno))
> +               goto close_client_fd;
> +
> +       err = getsockname(client_fd, (struct sockaddr *)&addr, &addr_len);
> +       if (CHECK(err, "getsockname", "Can't get client local addr\n"))
> +               goto close_client_fd;
> +
> +       cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
> +       CHECK(val.cookie_value != cookie_expected_value, "",
> +             "Unexpected value in map: %x != %x\n", val.cookie_value,
> +             cookie_expected_value);

nit: ASSERT_NEQ is nicer

> +
> +close_client_fd:
> +       close(client_fd);
> +close_server_fd:
> +       close(server_fd);
> +free_update_link:
> +       bpf_link__destroy(update_link);
> +free_set_link:
> +       bpf_link__destroy(set_link);
> +close_cgroup_fd:
> +       close(cgroup_fd);
> +destroy_skel:
> +       socket_cookie_prog__destroy(skel);
> +}

[...]

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

[Florent Revest]

On Thu, Jan 21, 2021 at 8:55 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jan 19, 2021 at 8:00 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>
> > ---
>
> Few nits below regarding skeleton and ASSERT_xxx usage.
>
> >  tools/testing/selftests/bpf/Makefile          |   3 +-
> >  .../selftests/bpf/prog_tests/socket_cookie.c  |  82 +++++++
> >  .../selftests/bpf/progs/socket_cookie_prog.c  |   2 -
> >  .../selftests/bpf/test_socket_cookie.c        | 208 ------------------
>
> please also update .gitignore

Good catch!

> >  4 files changed, 83 insertions(+), 212 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> >  delete mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
> >
>
> [...]
>
> > +
> > +       skel = socket_cookie_prog__open_and_load();
> > +       if (CHECK(!skel, "socket_cookie_prog__open_and_load",
> > +                 "skeleton open_and_load failed\n"))
>
> nit: ASSERT_PTR_OK

Ah great, I find the ASSERT semantic much easier to follow than CHECKs.

> > +               return;
> > +
> > +       cgroup_fd = test__join_cgroup("/socket_cookie");
> > +       if (CHECK(cgroup_fd < 0, "join_cgroup", "cgroup creation failed\n"))
> > +               goto destroy_skel;
> > +
> > +       set_link = bpf_program__attach_cgroup(skel->progs.set_cookie,
> > +                                             cgroup_fd);
>
> you can use skel->links->set_cookie here and it will be auto-destroyed
> when the whole skeleton is destroyed. More simplification.

Sick. :)

> > +       if (CHECK(IS_ERR(set_link), "set-link-cg-attach", "err %ld\n",
> > +                 PTR_ERR(set_link)))
> > +               goto close_cgroup_fd;
> > +
> > +       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
> > +                                                cgroup_fd);
>
> same as above, no need to maintain your link outside of skeleton
>
>
> > +       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
> > +                 PTR_ERR(update_link)))
> > +               goto free_set_link;
> > +
> > +       server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> > +       if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
> > +               goto free_update_link;
> > +
> > +       client_fd = connect_to_fd(server_fd, 0);
> > +       if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
> > +               goto close_server_fd;
>
> nit: ASSERT_OK is nicer (here and in few other places)

Did you mean ASSERT_OK for the two following err checks ?

ASSERT_OK does not seem right for a fd check where we want fd to be
positive. ASSERT_OK does: "bool ___ok = ___res == 0;"

I will keep my "CHECK(fd < 0" but maybe there could be an
ASSERT_POSITIVE that does "bool ___ok = ___res >= 0;"

> > +
> > +       err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.socket_cookies),
> > +                                 &client_fd, &val);
> > +       if (CHECK(err, "map_lookup", "err %d errno %d\n", err, errno))
> > +               goto close_client_fd;
> > +
> > +       err = getsockname(client_fd, (struct sockaddr *)&addr, &addr_len);
> > +       if (CHECK(err, "getsockname", "Can't get client local addr\n"))
> > +               goto close_client_fd;
> > +
> > +       cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
> > +       CHECK(val.cookie_value != cookie_expected_value, "",
> > +             "Unexpected value in map: %x != %x\n", val.cookie_value,
> > +             cookie_expected_value);
>
> nit: ASSERT_NEQ is nicer

Indeed.

> > +
> > +close_client_fd:
> > +       close(client_fd);
> > +close_server_fd:
> > +       close(server_fd);
> > +free_update_link:
> > +       bpf_link__destroy(update_link);
> > +free_set_link:
> > +       bpf_link__destroy(set_link);
> > +close_cgroup_fd:
> > +       close(cgroup_fd);
> > +destroy_skel:
> > +       socket_cookie_prog__destroy(skel);
> > +}
>
> [...]

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

[Andrii Nakryiko]

On Fri, Jan 22, 2021 at 6:35 AM Florent Revest <revest@chromium.org> wrote:
>
> On Thu, Jan 21, 2021 at 8:55 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Jan 19, 2021 at 8:00 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>
> > > ---
> >
> > Few nits below regarding skeleton and ASSERT_xxx usage.
> >
> > >  tools/testing/selftests/bpf/Makefile          |   3 +-
> > >  .../selftests/bpf/prog_tests/socket_cookie.c  |  82 +++++++
> > >  .../selftests/bpf/progs/socket_cookie_prog.c  |   2 -
> > >  .../selftests/bpf/test_socket_cookie.c        | 208 ------------------
> >
> > please also update .gitignore
>
> Good catch!
>
> > >  4 files changed, 83 insertions(+), 212 deletions(-)
> > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > >  delete mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
> > >
> >
> > [...]
> >
> > > +
> > > +       skel = socket_cookie_prog__open_and_load();
> > > +       if (CHECK(!skel, "socket_cookie_prog__open_and_load",
> > > +                 "skeleton open_and_load failed\n"))
> >
> > nit: ASSERT_PTR_OK
>
> Ah great, I find the ASSERT semantic much easier to follow than CHECKs.
>
> > > +               return;
> > > +
> > > +       cgroup_fd = test__join_cgroup("/socket_cookie");
> > > +       if (CHECK(cgroup_fd < 0, "join_cgroup", "cgroup creation failed\n"))
> > > +               goto destroy_skel;
> > > +
> > > +       set_link = bpf_program__attach_cgroup(skel->progs.set_cookie,
> > > +                                             cgroup_fd);
> >
> > you can use skel->links->set_cookie here and it will be auto-destroyed
> > when the whole skeleton is destroyed. More simplification.
>
> Sick. :)
>
> > > +       if (CHECK(IS_ERR(set_link), "set-link-cg-attach", "err %ld\n",
> > > +                 PTR_ERR(set_link)))
> > > +               goto close_cgroup_fd;
> > > +
> > > +       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
> > > +                                                cgroup_fd);
> >
> > same as above, no need to maintain your link outside of skeleton
> >
> >
> > > +       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
> > > +                 PTR_ERR(update_link)))
> > > +               goto free_set_link;
> > > +
> > > +       server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> > > +       if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
> > > +               goto free_update_link;
> > > +
> > > +       client_fd = connect_to_fd(server_fd, 0);
> > > +       if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
> > > +               goto close_server_fd;
> >
> > nit: ASSERT_OK is nicer (here and in few other places)
>
> Did you mean ASSERT_OK for the two following err checks ?
>
> ASSERT_OK does not seem right for a fd check where we want fd to be
> positive. ASSERT_OK does: "bool ___ok = ___res == 0;"
>
> I will keep my "CHECK(fd < 0" but maybe there could be an
> ASSERT_POSITIVE that does "bool ___ok = ___res >= 0;"

Ah, I missed that it's returning FD, sorry. For FD we'd have to add
the ASSERT_GEQ(fd, 0, "blah") variant. So yeah, stick to CHECK for
now.

>
> > > +
> > > +       err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.socket_cookies),
> > > +                                 &client_fd, &val);
> > > +       if (CHECK(err, "map_lookup", "err %d errno %d\n", err, errno))
> > > +               goto close_client_fd;
> > > +
> > > +       err = getsockname(client_fd, (struct sockaddr *)&addr, &addr_len);
> > > +       if (CHECK(err, "getsockname", "Can't get client local addr\n"))
> > > +               goto close_client_fd;
> > > +
> > > +       cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
> > > +       CHECK(val.cookie_value != cookie_expected_value, "",
> > > +             "Unexpected value in map: %x != %x\n", val.cookie_value,
> > > +             cookie_expected_value);
> >
> > nit: ASSERT_NEQ is nicer
>
> Indeed.
>
> > > +
> > > +close_client_fd:
> > > +       close(client_fd);
> > > +close_server_fd:
> > > +       close(server_fd);
> > > +free_update_link:
> > > +       bpf_link__destroy(update_link);
> > > +free_set_link:
> > > +       bpf_link__destroy(set_link);
> > > +close_cgroup_fd:
> > > +       close(cgroup_fd);
> > > +destroy_skel:
> > > +       socket_cookie_prog__destroy(skel);
> > > +}
> >
> > [...]

[PATCH bpf-next v5 4/4] 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.

Adding a tracing program to the existing objects requires a different
attachment strategy and different headers.

Signed-off-by: Florent Revest <revest@chromium.org>
---
 .../selftests/bpf/prog_tests/socket_cookie.c  | 24 +++++++----
 .../selftests/bpf/progs/socket_cookie_prog.c  | 41 ++++++++++++++++---
 2 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
index 53d0c44e7907..e5c5e2ea1deb 100644
--- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
@@ -15,8 +15,8 @@ struct socket_cookie {
 
 void test_socket_cookie(void)
 {
+	struct bpf_link *set_link, *update_sockops_link, *update_tracing_link;
 	socklen_t addr_len = sizeof(struct sockaddr_in6);
-	struct bpf_link *set_link, *update_link;
 	int server_fd, client_fd, cgroup_fd;
 	struct socket_cookie_prog *skel;
 	__u32 cookie_expected_value;
@@ -39,15 +39,21 @@ void test_socket_cookie(void)
 		  PTR_ERR(set_link)))
 		goto close_cgroup_fd;
 
-	update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
-						 cgroup_fd);
-	if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
-		  PTR_ERR(update_link)))
+	update_sockops_link = bpf_program__attach_cgroup(
+		skel->progs.update_cookie_sockops, cgroup_fd);
+	if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach",
+		  "err %ld\n", PTR_ERR(update_sockops_link)))
 		goto free_set_link;
 
+	update_tracing_link = bpf_program__attach(
+		skel->progs.update_cookie_tracing);
+	if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach",
+		  "err %ld\n", PTR_ERR(update_tracing_link)))
+		goto free_update_sockops_link;
+
 	server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
 	if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
-		goto free_update_link;
+		goto free_update_tracing_link;
 
 	client_fd = connect_to_fd(server_fd, 0);
 	if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
@@ -71,8 +77,10 @@ void test_socket_cookie(void)
 	close(client_fd);
 close_server_fd:
 	close(server_fd);
-free_update_link:
-	bpf_link__destroy(update_link);
+free_update_tracing_link:
+	bpf_link__destroy(update_tracing_link);
+free_update_sockops_link:
+	bpf_link__destroy(update_sockops_link);
 free_set_link:
 	bpf_link__destroy(set_link);
 close_cgroup_fd:
diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
index 81e84be6f86d..1f770b732cb1 100644
--- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
+++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
@@ -1,11 +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>
+#include <bpf/bpf_tracing.h>
+
+#define AF_INET6 10
 
 struct socket_cookie {
 	__u64 cookie_key;
@@ -19,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)
 {
@@ -32,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;
 	struct socket_cookie *p;
@@ -60,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.284.gd98b1dd5eaa7-goog

Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie

[KP Singh]

On Tue, Jan 19, 2021 at 5:00 PM Florent Revest <revest@chromium.org> wrote:
>
> 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.
>
> Adding a tracing program to the existing objects requires a different
> attachment strategy and different headers.
>
> Signed-off-by: Florent Revest <revest@chromium.org>

Acked-by: KP Singh <kpsingh@kernel.org>

(one minor note, doesn't really need fixing as a part of this though)

> ---
>  .../selftests/bpf/prog_tests/socket_cookie.c  | 24 +++++++----
>  .../selftests/bpf/progs/socket_cookie_prog.c  | 41 ++++++++++++++++---
>  2 files changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> index 53d0c44e7907..e5c5e2ea1deb 100644
> --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> @@ -15,8 +15,8 @@ struct socket_cookie {
>
>  void test_socket_cookie(void)
>  {
> +       struct bpf_link *set_link, *update_sockops_link, *update_tracing_link;
>         socklen_t addr_len = sizeof(struct sockaddr_in6);
> -       struct bpf_link *set_link, *update_link;
>         int server_fd, client_fd, cgroup_fd;
>         struct socket_cookie_prog *skel;
>         __u32 cookie_expected_value;
> @@ -39,15 +39,21 @@ void test_socket_cookie(void)
>                   PTR_ERR(set_link)))
>                 goto close_cgroup_fd;
>
> -       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
> -                                                cgroup_fd);
> -       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
> -                 PTR_ERR(update_link)))
> +       update_sockops_link = bpf_program__attach_cgroup(
> +               skel->progs.update_cookie_sockops, cgroup_fd);
> +       if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach",
> +                 "err %ld\n", PTR_ERR(update_sockops_link)))
>                 goto free_set_link;
>
> +       update_tracing_link = bpf_program__attach(
> +               skel->progs.update_cookie_tracing);
> +       if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach",
> +                 "err %ld\n", PTR_ERR(update_tracing_link)))
> +               goto free_update_sockops_link;
> +
>         server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
>         if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
> -               goto free_update_link;
> +               goto free_update_tracing_link;
>
>         client_fd = connect_to_fd(server_fd, 0);
>         if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
> @@ -71,8 +77,10 @@ void test_socket_cookie(void)
>         close(client_fd);
>  close_server_fd:
>         close(server_fd);
> -free_update_link:
> -       bpf_link__destroy(update_link);
> +free_update_tracing_link:
> +       bpf_link__destroy(update_tracing_link);

I don't think this need to block submission unless there are other
issues but the
bpf_link__destroy can just be called in a single cleanup label because
it handles null or
erroneous inputs:

int bpf_link__destroy(struct bpf_link *link)
{
    int err = 0;

    if (IS_ERR_OR_NULL(link))
         return 0;
[...]

Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie

[Alexei Starovoitov]

On Wed, Jan 20, 2021 at 9:08 AM KP Singh <kpsingh@kernel.org> wrote:
>
> On Tue, Jan 19, 2021 at 5:00 PM Florent Revest <revest@chromium.org> wrote:
> >
> > 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.
> >
> > Adding a tracing program to the existing objects requires a different
> > attachment strategy and different headers.
> >
> > Signed-off-by: Florent Revest <revest@chromium.org>
>
> Acked-by: KP Singh <kpsingh@kernel.org>
>
> (one minor note, doesn't really need fixing as a part of this though)
>
> > ---
> >  .../selftests/bpf/prog_tests/socket_cookie.c  | 24 +++++++----
> >  .../selftests/bpf/progs/socket_cookie_prog.c  | 41 ++++++++++++++++---
> >  2 files changed, 52 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > index 53d0c44e7907..e5c5e2ea1deb 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > @@ -15,8 +15,8 @@ struct socket_cookie {
> >
> >  void test_socket_cookie(void)
> >  {
> > +       struct bpf_link *set_link, *update_sockops_link, *update_tracing_link;
> >         socklen_t addr_len = sizeof(struct sockaddr_in6);
> > -       struct bpf_link *set_link, *update_link;
> >         int server_fd, client_fd, cgroup_fd;
> >         struct socket_cookie_prog *skel;
> >         __u32 cookie_expected_value;
> > @@ -39,15 +39,21 @@ void test_socket_cookie(void)
> >                   PTR_ERR(set_link)))
> >                 goto close_cgroup_fd;
> >
> > -       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
> > -                                                cgroup_fd);
> > -       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
> > -                 PTR_ERR(update_link)))
> > +       update_sockops_link = bpf_program__attach_cgroup(
> > +               skel->progs.update_cookie_sockops, cgroup_fd);
> > +       if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach",
> > +                 "err %ld\n", PTR_ERR(update_sockops_link)))
> >                 goto free_set_link;
> >
> > +       update_tracing_link = bpf_program__attach(
> > +               skel->progs.update_cookie_tracing);
> > +       if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach",
> > +                 "err %ld\n", PTR_ERR(update_tracing_link)))
> > +               goto free_update_sockops_link;
> > +
> >         server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> >         if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
> > -               goto free_update_link;
> > +               goto free_update_tracing_link;
> >
> >         client_fd = connect_to_fd(server_fd, 0);
> >         if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
> > @@ -71,8 +77,10 @@ void test_socket_cookie(void)
> >         close(client_fd);
> >  close_server_fd:
> >         close(server_fd);
> > -free_update_link:
> > -       bpf_link__destroy(update_link);
> > +free_update_tracing_link:
> > +       bpf_link__destroy(update_tracing_link);
>
> I don't think this need to block submission unless there are other
> issues but the
> bpf_link__destroy can just be called in a single cleanup label because
> it handles null or
> erroneous inputs:
>
> int bpf_link__destroy(struct bpf_link *link)
> {
>     int err = 0;
>
>     if (IS_ERR_OR_NULL(link))
>          return 0;
> [...]

+1 to KP's point.

Also Florent, how did you test it?
This test fails in CI and in my manual run:
./test_progs -t cook
libbpf: load bpf program failed: Permission denied
libbpf: -- BEGIN DUMP LOG ---
libbpf:
; int update_cookie_sockops(struct bpf_sock_ops *ctx)
0: (bf) r6 = r1
; if (ctx->family != AF_INET6)
1: (61) r1 = *(u32 *)(r6 +20)
; if (ctx->family != AF_INET6)
2: (56) if w1 != 0xa goto pc+21
 R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
3: (61) r1 = *(u32 *)(r6 +0)
; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
4: (56) if w1 != 0x3 goto pc+19
 R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
; if (!ctx->sk)
5: (79) r1 = *(u64 *)(r6 +184)
; if (!ctx->sk)
6: (15) if r1 == 0x0 goto pc+17
 R1_w=sock(id=0,ref_obj_id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0
; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
7: (79) r2 = *(u64 *)(r6 +184)
; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
8: (18) r1 = 0xffff888106e41400
10: (b7) r3 = 0
11: (b7) r4 = 0
12: (85) call bpf_sk_storage_get#107
R2 type=sock_or_null expected=sock_common, sock, tcp_sock, xdp_sock, ptr_
processed 12 insns (limit 1000000) max_states_per_insn 0 total_states
0 peak_states 0 mark_read 0

libbpf: -- END LOG --
libbpf: failed to load program 'update_cookie_sockops'
libbpf: failed to load object 'socket_cookie_prog'
libbpf: failed to load BPF skeleton 'socket_cookie_prog': -4007
test_socket_cookie:FAIL:socket_cookie_prog__open_and_load skeleton
open_and_load failed
#95 socket_cookie:FAIL
Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie

[Florent Revest]

On Wed, Jan 20, 2021 at 8:04 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jan 20, 2021 at 9:08 AM KP Singh <kpsingh@kernel.org> wrote:
> >
> > On Tue, Jan 19, 2021 at 5:00 PM Florent Revest <revest@chromium.org> wrote:
> > >
> > > 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.
> > >
> > > Adding a tracing program to the existing objects requires a different
> > > attachment strategy and different headers.
> > >
> > > Signed-off-by: Florent Revest <revest@chromium.org>
> >
> > Acked-by: KP Singh <kpsingh@kernel.org>
> >
> > (one minor note, doesn't really need fixing as a part of this though)
> >
> > > ---
> > >  .../selftests/bpf/prog_tests/socket_cookie.c  | 24 +++++++----
> > >  .../selftests/bpf/progs/socket_cookie_prog.c  | 41 ++++++++++++++++---
> > >  2 files changed, 52 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > > index 53d0c44e7907..e5c5e2ea1deb 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > > @@ -15,8 +15,8 @@ struct socket_cookie {
> > >
> > >  void test_socket_cookie(void)
> > >  {
> > > +       struct bpf_link *set_link, *update_sockops_link, *update_tracing_link;
> > >         socklen_t addr_len = sizeof(struct sockaddr_in6);
> > > -       struct bpf_link *set_link, *update_link;
> > >         int server_fd, client_fd, cgroup_fd;
> > >         struct socket_cookie_prog *skel;
> > >         __u32 cookie_expected_value;
> > > @@ -39,15 +39,21 @@ void test_socket_cookie(void)
> > >                   PTR_ERR(set_link)))
> > >                 goto close_cgroup_fd;
> > >
> > > -       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
> > > -                                                cgroup_fd);
> > > -       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
> > > -                 PTR_ERR(update_link)))
> > > +       update_sockops_link = bpf_program__attach_cgroup(
> > > +               skel->progs.update_cookie_sockops, cgroup_fd);
> > > +       if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach",
> > > +                 "err %ld\n", PTR_ERR(update_sockops_link)))
> > >                 goto free_set_link;
> > >
> > > +       update_tracing_link = bpf_program__attach(
> > > +               skel->progs.update_cookie_tracing);
> > > +       if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach",
> > > +                 "err %ld\n", PTR_ERR(update_tracing_link)))
> > > +               goto free_update_sockops_link;
> > > +
> > >         server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> > >         if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
> > > -               goto free_update_link;
> > > +               goto free_update_tracing_link;
> > >
> > >         client_fd = connect_to_fd(server_fd, 0);
> > >         if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
> > > @@ -71,8 +77,10 @@ void test_socket_cookie(void)
> > >         close(client_fd);
> > >  close_server_fd:
> > >         close(server_fd);
> > > -free_update_link:
> > > -       bpf_link__destroy(update_link);
> > > +free_update_tracing_link:
> > > +       bpf_link__destroy(update_tracing_link);
> >
> > I don't think this need to block submission unless there are other
> > issues but the
> > bpf_link__destroy can just be called in a single cleanup label because
> > it handles null or
> > erroneous inputs:
> >
> > int bpf_link__destroy(struct bpf_link *link)
> > {
> >     int err = 0;
> >
> >     if (IS_ERR_OR_NULL(link))
> >          return 0;
> > [...]
>
> +1 to KP's point.
>
> Also Florent, how did you test it?
> This test fails in CI and in my manual run:
> ./test_progs -t cook
> libbpf: load bpf program failed: Permission denied
> libbpf: -- BEGIN DUMP LOG ---
> libbpf:
> ; int update_cookie_sockops(struct bpf_sock_ops *ctx)
> 0: (bf) r6 = r1
> ; if (ctx->family != AF_INET6)
> 1: (61) r1 = *(u32 *)(r6 +20)
> ; if (ctx->family != AF_INET6)
> 2: (56) if w1 != 0xa goto pc+21
>  R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
> 3: (61) r1 = *(u32 *)(r6 +0)
> ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
> 4: (56) if w1 != 0x3 goto pc+19
>  R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> ; if (!ctx->sk)
> 5: (79) r1 = *(u64 *)(r6 +184)
> ; if (!ctx->sk)
> 6: (15) if r1 == 0x0 goto pc+17
>  R1_w=sock(id=0,ref_obj_id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> 7: (79) r2 = *(u64 *)(r6 +184)
> ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> 8: (18) r1 = 0xffff888106e41400
> 10: (b7) r3 = 0
> 11: (b7) r4 = 0
> 12: (85) call bpf_sk_storage_get#107
> R2 type=sock_or_null expected=sock_common, sock, tcp_sock, xdp_sock, ptr_
> processed 12 insns (limit 1000000) max_states_per_insn 0 total_states
> 0 peak_states 0 mark_read 0
>
> libbpf: -- END LOG --
> libbpf: failed to load program 'update_cookie_sockops'
> libbpf: failed to load object 'socket_cookie_prog'
> libbpf: failed to load BPF skeleton 'socket_cookie_prog': -4007
> test_socket_cookie:FAIL:socket_cookie_prog__open_and_load skeleton
> open_and_load failed
> #95 socket_cookie:FAIL
> Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

Oh :| I must have missed something in the rebase, I will fix this and
address KP's comment then. Thanks for the review and sorry for the
waste of time :)

Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie

[Florent Revest]

On Wed, Jan 20, 2021 at 8:06 PM Florent Revest <revest@chromium.org> wrote:
>
> On Wed, Jan 20, 2021 at 8:04 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 9:08 AM KP Singh <kpsingh@kernel.org> wrote:
> > >
> > > On Tue, Jan 19, 2021 at 5:00 PM Florent Revest <revest@chromium.org> wrote:
> > > >
> > > > 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.
> > > >
> > > > Adding a tracing program to the existing objects requires a different
> > > > attachment strategy and different headers.
> > > >
> > > > Signed-off-by: Florent Revest <revest@chromium.org>
> > >
> > > Acked-by: KP Singh <kpsingh@kernel.org>
> > >
> > > (one minor note, doesn't really need fixing as a part of this though)
> > >
> > > > ---
> > > >  .../selftests/bpf/prog_tests/socket_cookie.c  | 24 +++++++----
> > > >  .../selftests/bpf/progs/socket_cookie_prog.c  | 41 ++++++++++++++++---
> > > >  2 files changed, 52 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > > > index 53d0c44e7907..e5c5e2ea1deb 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> > > > @@ -15,8 +15,8 @@ struct socket_cookie {
> > > >
> > > >  void test_socket_cookie(void)
> > > >  {
> > > > +       struct bpf_link *set_link, *update_sockops_link, *update_tracing_link;
> > > >         socklen_t addr_len = sizeof(struct sockaddr_in6);
> > > > -       struct bpf_link *set_link, *update_link;
> > > >         int server_fd, client_fd, cgroup_fd;
> > > >         struct socket_cookie_prog *skel;
> > > >         __u32 cookie_expected_value;
> > > > @@ -39,15 +39,21 @@ void test_socket_cookie(void)
> > > >                   PTR_ERR(set_link)))
> > > >                 goto close_cgroup_fd;
> > > >
> > > > -       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
> > > > -                                                cgroup_fd);
> > > > -       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
> > > > -                 PTR_ERR(update_link)))
> > > > +       update_sockops_link = bpf_program__attach_cgroup(
> > > > +               skel->progs.update_cookie_sockops, cgroup_fd);
> > > > +       if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach",
> > > > +                 "err %ld\n", PTR_ERR(update_sockops_link)))
> > > >                 goto free_set_link;
> > > >
> > > > +       update_tracing_link = bpf_program__attach(
> > > > +               skel->progs.update_cookie_tracing);
> > > > +       if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach",
> > > > +                 "err %ld\n", PTR_ERR(update_tracing_link)))
> > > > +               goto free_update_sockops_link;
> > > > +
> > > >         server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> > > >         if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
> > > > -               goto free_update_link;
> > > > +               goto free_update_tracing_link;
> > > >
> > > >         client_fd = connect_to_fd(server_fd, 0);
> > > >         if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
> > > > @@ -71,8 +77,10 @@ void test_socket_cookie(void)
> > > >         close(client_fd);
> > > >  close_server_fd:
> > > >         close(server_fd);
> > > > -free_update_link:
> > > > -       bpf_link__destroy(update_link);
> > > > +free_update_tracing_link:
> > > > +       bpf_link__destroy(update_tracing_link);
> > >
> > > I don't think this need to block submission unless there are other
> > > issues but the
> > > bpf_link__destroy can just be called in a single cleanup label because
> > > it handles null or
> > > erroneous inputs:
> > >
> > > int bpf_link__destroy(struct bpf_link *link)
> > > {
> > >     int err = 0;
> > >
> > >     if (IS_ERR_OR_NULL(link))
> > >          return 0;
> > > [...]
> >
> > +1 to KP's point.
> >
> > Also Florent, how did you test it?
> > This test fails in CI and in my manual run:
> > ./test_progs -t cook
> > libbpf: load bpf program failed: Permission denied
> > libbpf: -- BEGIN DUMP LOG ---
> > libbpf:
> > ; int update_cookie_sockops(struct bpf_sock_ops *ctx)
> > 0: (bf) r6 = r1
> > ; if (ctx->family != AF_INET6)
> > 1: (61) r1 = *(u32 *)(r6 +20)
> > ; if (ctx->family != AF_INET6)
> > 2: (56) if w1 != 0xa goto pc+21
> >  R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> > ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
> > 3: (61) r1 = *(u32 *)(r6 +0)
> > ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
> > 4: (56) if w1 != 0x3 goto pc+19
> >  R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> > ; if (!ctx->sk)
> > 5: (79) r1 = *(u64 *)(r6 +184)
> > ; if (!ctx->sk)
> > 6: (15) if r1 == 0x0 goto pc+17
> >  R1_w=sock(id=0,ref_obj_id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> > ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> > 7: (79) r2 = *(u64 *)(r6 +184)
> > ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> > 8: (18) r1 = 0xffff888106e41400
> > 10: (b7) r3 = 0
> > 11: (b7) r4 = 0
> > 12: (85) call bpf_sk_storage_get#107
> > R2 type=sock_or_null expected=sock_common, sock, tcp_sock, xdp_sock, ptr_
> > processed 12 insns (limit 1000000) max_states_per_insn 0 total_states
> > 0 peak_states 0 mark_read 0
> >
> > libbpf: -- END LOG --
> > libbpf: failed to load program 'update_cookie_sockops'
> > libbpf: failed to load object 'socket_cookie_prog'
> > libbpf: failed to load BPF skeleton 'socket_cookie_prog': -4007
> > test_socket_cookie:FAIL:socket_cookie_prog__open_and_load skeleton
> > open_and_load failed
> > #95 socket_cookie:FAIL
> > Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>
> Oh :| I must have missed something in the rebase, I will fix this and
> address KP's comment then. Thanks for the review and sorry for the
> waste of time :)

So this is actually an interesting one I think. :) The failure was
triggered by the combination of an LLVM update and this change:

-#include <linux/bpf.h>
+#include "vmlinux.h"

With an older LLVM, this used to work.
With a recent LLVM, the change of header causes those 3 lines to get
compiled differently:

if (!ctx->sk)
    return 1;
p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);

When including linux/bpf.h
; if (!ctx->sk)
       5: 79 62 b8 00 00 00 00 00 r2 = *(u64 *)(r6 + 184)
       6: 15 02 10 00 00 00 00 00 if r2 == 0 goto +16 <LBB1_6>
; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
       7: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
       9: b7 03 00 00 00 00 00 00 r3 = 0
      10: b7 04 00 00 00 00 00 00 r4 = 0
      11: 85 00 00 00 6b 00 00 00 call 107
      12: bf 07 00 00 00 00 00 00 r7 = r0

When including vmlinux.h
; if (!ctx->sk)
       5: 79 61 b8 00 00 00 00 00 r1 = *(u64 *)(r6 + 184)
       6: 15 01 11 00 00 00 00 00 if r1 == 0 goto +17 <LBB1_6>
; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
       7: 79 62 b8 00 00 00 00 00 r2 = *(u64 *)(r6 + 184)
       8: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
      10: b7 03 00 00 00 00 00 00 r3 = 0
      11: b7 04 00 00 00 00 00 00 r4 = 0
      12: 85 00 00 00 6b 00 00 00 call 107
      13: bf 07 00 00 00 00 00 00 r7 = r0

Note that ctx->sk gets fetched once in the first case (l5), and twice
in the second case (l5 and l7).
I'm assuming that struct bpf_sock_ops gets defined with different
attributes in vmlinux.h and causes LLVM to assume that ctx->sk could
have changed between the time of check and the time of use so it
yields two fetches and the verifier can't track that r2 is non null.

If I save ctx->sk in a temporary variable first:

struct bpf_sock *sk = ctx->sk;
if (!sk)
    return 1;
p = bpf_sk_storage_get(&socket_cookies, sk, 0, 0);

this loads correctly. However, Brendan pointed out that this is also a
weak guarantee and that LLVM could still decide to compile this into
two fetches, Brendan suggested that we save sk out of an access to a
volatile pointer to ctx, what do you think ?

Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie

[Yonghong Song]

On 1/22/21 7:34 AM, Florent Revest wrote:
> On Wed, Jan 20, 2021 at 8:06 PM Florent Revest <revest@chromium.org> wrote:
>>
>> On Wed, Jan 20, 2021 at 8:04 PM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>>
>>> On Wed, Jan 20, 2021 at 9:08 AM KP Singh <kpsingh@kernel.org> wrote:
>>>>
>>>> On Tue, Jan 19, 2021 at 5:00 PM Florent Revest <revest@chromium.org> wrote:
>>>>>
>>>>> 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.
>>>>>
>>>>> Adding a tracing program to the existing objects requires a different
>>>>> attachment strategy and different headers.
>>>>>
>>>>> Signed-off-by: Florent Revest <revest@chromium.org>
>>>>
>>>> Acked-by: KP Singh <kpsingh@kernel.org>
>>>>
>>>> (one minor note, doesn't really need fixing as a part of this though)
>>>>
>>>>> ---
>>>>>   .../selftests/bpf/prog_tests/socket_cookie.c  | 24 +++++++----
>>>>>   .../selftests/bpf/progs/socket_cookie_prog.c  | 41 ++++++++++++++++---
>>>>>   2 files changed, 52 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
>>>>> index 53d0c44e7907..e5c5e2ea1deb 100644
>>>>> --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
>>>>> +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
>>>>> @@ -15,8 +15,8 @@ struct socket_cookie {
>>>>>
>>>>>   void test_socket_cookie(void)
>>>>>   {
>>>>> +       struct bpf_link *set_link, *update_sockops_link, *update_tracing_link;
>>>>>          socklen_t addr_len = sizeof(struct sockaddr_in6);
>>>>> -       struct bpf_link *set_link, *update_link;
>>>>>          int server_fd, client_fd, cgroup_fd;
>>>>>          struct socket_cookie_prog *skel;
>>>>>          __u32 cookie_expected_value;
>>>>> @@ -39,15 +39,21 @@ void test_socket_cookie(void)
>>>>>                    PTR_ERR(set_link)))
>>>>>                  goto close_cgroup_fd;
>>>>>
>>>>> -       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
>>>>> -                                                cgroup_fd);
>>>>> -       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
>>>>> -                 PTR_ERR(update_link)))
>>>>> +       update_sockops_link = bpf_program__attach_cgroup(
>>>>> +               skel->progs.update_cookie_sockops, cgroup_fd);
>>>>> +       if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach",
>>>>> +                 "err %ld\n", PTR_ERR(update_sockops_link)))
>>>>>                  goto free_set_link;
>>>>>
>>>>> +       update_tracing_link = bpf_program__attach(
>>>>> +               skel->progs.update_cookie_tracing);
>>>>> +       if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach",
>>>>> +                 "err %ld\n", PTR_ERR(update_tracing_link)))
>>>>> +               goto free_update_sockops_link;
>>>>> +
>>>>>          server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
>>>>>          if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
>>>>> -               goto free_update_link;
>>>>> +               goto free_update_tracing_link;
>>>>>
>>>>>          client_fd = connect_to_fd(server_fd, 0);
>>>>>          if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
>>>>> @@ -71,8 +77,10 @@ void test_socket_cookie(void)
>>>>>          close(client_fd);
>>>>>   close_server_fd:
>>>>>          close(server_fd);
>>>>> -free_update_link:
>>>>> -       bpf_link__destroy(update_link);
>>>>> +free_update_tracing_link:
>>>>> +       bpf_link__destroy(update_tracing_link);
>>>>
>>>> I don't think this need to block submission unless there are other
>>>> issues but the
>>>> bpf_link__destroy can just be called in a single cleanup label because
>>>> it handles null or
>>>> erroneous inputs:
>>>>
>>>> int bpf_link__destroy(struct bpf_link *link)
>>>> {
>>>>      int err = 0;
>>>>
>>>>      if (IS_ERR_OR_NULL(link))
>>>>           return 0;
>>>> [...]
>>>
>>> +1 to KP's point.
>>>
>>> Also Florent, how did you test it?
>>> This test fails in CI and in my manual run:
>>> ./test_progs -t cook
>>> libbpf: load bpf program failed: Permission denied
>>> libbpf: -- BEGIN DUMP LOG ---
>>> libbpf:
>>> ; int update_cookie_sockops(struct bpf_sock_ops *ctx)
>>> 0: (bf) r6 = r1
>>> ; if (ctx->family != AF_INET6)
>>> 1: (61) r1 = *(u32 *)(r6 +20)
>>> ; if (ctx->family != AF_INET6)
>>> 2: (56) if w1 != 0xa goto pc+21
>>>   R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
>>> ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
>>> 3: (61) r1 = *(u32 *)(r6 +0)
>>> ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
>>> 4: (56) if w1 != 0x3 goto pc+19
>>>   R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
>>> ; if (!ctx->sk)
>>> 5: (79) r1 = *(u64 *)(r6 +184)
>>> ; if (!ctx->sk)
>>> 6: (15) if r1 == 0x0 goto pc+17
>>>   R1_w=sock(id=0,ref_obj_id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0
>>> ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
>>> 7: (79) r2 = *(u64 *)(r6 +184)
>>> ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
>>> 8: (18) r1 = 0xffff888106e41400
>>> 10: (b7) r3 = 0
>>> 11: (b7) r4 = 0
>>> 12: (85) call bpf_sk_storage_get#107
>>> R2 type=sock_or_null expected=sock_common, sock, tcp_sock, xdp_sock, ptr_
>>> processed 12 insns (limit 1000000) max_states_per_insn 0 total_states
>>> 0 peak_states 0 mark_read 0
>>>
>>> libbpf: -- END LOG --
>>> libbpf: failed to load program 'update_cookie_sockops'
>>> libbpf: failed to load object 'socket_cookie_prog'
>>> libbpf: failed to load BPF skeleton 'socket_cookie_prog': -4007
>>> test_socket_cookie:FAIL:socket_cookie_prog__open_and_load skeleton
>>> open_and_load failed
>>> #95 socket_cookie:FAIL
>>> Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>>
>> Oh :| I must have missed something in the rebase, I will fix this and
>> address KP's comment then. Thanks for the review and sorry for the
>> waste of time :)
> 
> So this is actually an interesting one I think. :) The failure was
> triggered by the combination of an LLVM update and this change:
> 
> -#include <linux/bpf.h>
> +#include "vmlinux.h"
> 
> With an older LLVM, this used to work.
> With a recent LLVM, the change of header causes those 3 lines to get
> compiled differently:
> 
> if (!ctx->sk)
>      return 1;
> p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> 
> When including linux/bpf.h
> ; if (!ctx->sk)
>         5: 79 62 b8 00 00 00 00 00 r2 = *(u64 *)(r6 + 184)
>         6: 15 02 10 00 00 00 00 00 if r2 == 0 goto +16 <LBB1_6>
> ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
>         7: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
>         9: b7 03 00 00 00 00 00 00 r3 = 0
>        10: b7 04 00 00 00 00 00 00 r4 = 0
>        11: 85 00 00 00 6b 00 00 00 call 107
>        12: bf 07 00 00 00 00 00 00 r7 = r0
> 
> When including vmlinux.h
> ; if (!ctx->sk)
>         5: 79 61 b8 00 00 00 00 00 r1 = *(u64 *)(r6 + 184)
>         6: 15 01 11 00 00 00 00 00 if r1 == 0 goto +17 <LBB1_6>
> ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
>         7: 79 62 b8 00 00 00 00 00 r2 = *(u64 *)(r6 + 184)
>         8: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
>        10: b7 03 00 00 00 00 00 00 r3 = 0
>        11: b7 04 00 00 00 00 00 00 r4 = 0
>        12: 85 00 00 00 6b 00 00 00 call 107
>        13: bf 07 00 00 00 00 00 00 r7 = r0
> 
> Note that ctx->sk gets fetched once in the first case (l5), and twice
> in the second case (l5 and l7).
> I'm assuming that struct bpf_sock_ops gets defined with different
> attributes in vmlinux.h and causes LLVM to assume that ctx->sk could
> have changed between the time of check and the time of use so it
> yields two fetches and the verifier can't track that r2 is non null.
> 
> If I save ctx->sk in a temporary variable first:
> 
> struct bpf_sock *sk = ctx->sk;
> if (!sk)
>      return 1;
> p = bpf_sk_storage_get(&socket_cookies, sk, 0, 0);
> 
> this loads correctly. However, Brendan pointed out that this is also a
> weak guarantee and that LLVM could still decide to compile this into
> two fetches, Brendan suggested that we save sk out of an access to a
> volatile pointer to ctx, what do you think ?

Your above workaround should be good. Compiler should not do *bad* 
optimizations to have two fetches if the source code just has one
in the above case. If this happens, it will be a llvm bug.

The latest llvm trunk can reproduce the above issue. It is due to
(1). llvm's partial (not complete) CSE and (2). this partial CSE
resulted in llvm BPF backend generating two CORE_MEM operations (for 
CORE relocations) instead of one. If llvm will do complete cse like the 
above your code, we will be fine.

Although fixing llvm CSE is desired, it may take
some time. At the same time, please use your above source workaround.
Thanks.

Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie

[Florent Revest]

On Sat, Jan 23, 2021 at 9:45 PM Yonghong Song <yhs@fb.com> wrote:
> On 1/22/21 7:34 AM, Florent Revest wrote:
> > On Wed, Jan 20, 2021 at 8:06 PM Florent Revest <revest@chromium.org> wrote:
> >>
> >> On Wed, Jan 20, 2021 at 8:04 PM Alexei Starovoitov
> >> <alexei.starovoitov@gmail.com> wrote:
> >>>
> >>> On Wed, Jan 20, 2021 at 9:08 AM KP Singh <kpsingh@kernel.org> wrote:
> >>>>
> >>>> On Tue, Jan 19, 2021 at 5:00 PM Florent Revest <revest@chromium.org> wrote:
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> Adding a tracing program to the existing objects requires a different
> >>>>> attachment strategy and different headers.
> >>>>>
> >>>>> Signed-off-by: Florent Revest <revest@chromium.org>
> >>>>
> >>>> Acked-by: KP Singh <kpsingh@kernel.org>
> >>>>
> >>>> (one minor note, doesn't really need fixing as a part of this though)
> >>>>
> >>>>> ---
> >>>>>   .../selftests/bpf/prog_tests/socket_cookie.c  | 24 +++++++----
> >>>>>   .../selftests/bpf/progs/socket_cookie_prog.c  | 41 ++++++++++++++++---
> >>>>>   2 files changed, 52 insertions(+), 13 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> >>>>> index 53d0c44e7907..e5c5e2ea1deb 100644
> >>>>> --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> >>>>> +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
> >>>>> @@ -15,8 +15,8 @@ struct socket_cookie {
> >>>>>
> >>>>>   void test_socket_cookie(void)
> >>>>>   {
> >>>>> +       struct bpf_link *set_link, *update_sockops_link, *update_tracing_link;
> >>>>>          socklen_t addr_len = sizeof(struct sockaddr_in6);
> >>>>> -       struct bpf_link *set_link, *update_link;
> >>>>>          int server_fd, client_fd, cgroup_fd;
> >>>>>          struct socket_cookie_prog *skel;
> >>>>>          __u32 cookie_expected_value;
> >>>>> @@ -39,15 +39,21 @@ void test_socket_cookie(void)
> >>>>>                    PTR_ERR(set_link)))
> >>>>>                  goto close_cgroup_fd;
> >>>>>
> >>>>> -       update_link = bpf_program__attach_cgroup(skel->progs.update_cookie,
> >>>>> -                                                cgroup_fd);
> >>>>> -       if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n",
> >>>>> -                 PTR_ERR(update_link)))
> >>>>> +       update_sockops_link = bpf_program__attach_cgroup(
> >>>>> +               skel->progs.update_cookie_sockops, cgroup_fd);
> >>>>> +       if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach",
> >>>>> +                 "err %ld\n", PTR_ERR(update_sockops_link)))
> >>>>>                  goto free_set_link;
> >>>>>
> >>>>> +       update_tracing_link = bpf_program__attach(
> >>>>> +               skel->progs.update_cookie_tracing);
> >>>>> +       if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach",
> >>>>> +                 "err %ld\n", PTR_ERR(update_tracing_link)))
> >>>>> +               goto free_update_sockops_link;
> >>>>> +
> >>>>>          server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> >>>>>          if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
> >>>>> -               goto free_update_link;
> >>>>> +               goto free_update_tracing_link;
> >>>>>
> >>>>>          client_fd = connect_to_fd(server_fd, 0);
> >>>>>          if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
> >>>>> @@ -71,8 +77,10 @@ void test_socket_cookie(void)
> >>>>>          close(client_fd);
> >>>>>   close_server_fd:
> >>>>>          close(server_fd);
> >>>>> -free_update_link:
> >>>>> -       bpf_link__destroy(update_link);
> >>>>> +free_update_tracing_link:
> >>>>> +       bpf_link__destroy(update_tracing_link);
> >>>>
> >>>> I don't think this need to block submission unless there are other
> >>>> issues but the
> >>>> bpf_link__destroy can just be called in a single cleanup label because
> >>>> it handles null or
> >>>> erroneous inputs:
> >>>>
> >>>> int bpf_link__destroy(struct bpf_link *link)
> >>>> {
> >>>>      int err = 0;
> >>>>
> >>>>      if (IS_ERR_OR_NULL(link))
> >>>>           return 0;
> >>>> [...]
> >>>
> >>> +1 to KP's point.
> >>>
> >>> Also Florent, how did you test it?
> >>> This test fails in CI and in my manual run:
> >>> ./test_progs -t cook
> >>> libbpf: load bpf program failed: Permission denied
> >>> libbpf: -- BEGIN DUMP LOG ---
> >>> libbpf:
> >>> ; int update_cookie_sockops(struct bpf_sock_ops *ctx)
> >>> 0: (bf) r6 = r1
> >>> ; if (ctx->family != AF_INET6)
> >>> 1: (61) r1 = *(u32 *)(r6 +20)
> >>> ; if (ctx->family != AF_INET6)
> >>> 2: (56) if w1 != 0xa goto pc+21
> >>>   R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> >>> ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
> >>> 3: (61) r1 = *(u32 *)(r6 +0)
> >>> ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
> >>> 4: (56) if w1 != 0x3 goto pc+19
> >>>   R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> >>> ; if (!ctx->sk)
> >>> 5: (79) r1 = *(u64 *)(r6 +184)
> >>> ; if (!ctx->sk)
> >>> 6: (15) if r1 == 0x0 goto pc+17
> >>>   R1_w=sock(id=0,ref_obj_id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0
> >>> ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> >>> 7: (79) r2 = *(u64 *)(r6 +184)
> >>> ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> >>> 8: (18) r1 = 0xffff888106e41400
> >>> 10: (b7) r3 = 0
> >>> 11: (b7) r4 = 0
> >>> 12: (85) call bpf_sk_storage_get#107
> >>> R2 type=sock_or_null expected=sock_common, sock, tcp_sock, xdp_sock, ptr_
> >>> processed 12 insns (limit 1000000) max_states_per_insn 0 total_states
> >>> 0 peak_states 0 mark_read 0
> >>>
> >>> libbpf: -- END LOG --
> >>> libbpf: failed to load program 'update_cookie_sockops'
> >>> libbpf: failed to load object 'socket_cookie_prog'
> >>> libbpf: failed to load BPF skeleton 'socket_cookie_prog': -4007
> >>> test_socket_cookie:FAIL:socket_cookie_prog__open_and_load skeleton
> >>> open_and_load failed
> >>> #95 socket_cookie:FAIL
> >>> Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
> >>
> >> Oh :| I must have missed something in the rebase, I will fix this and
> >> address KP's comment then. Thanks for the review and sorry for the
> >> waste of time :)
> >
> > So this is actually an interesting one I think. :) The failure was
> > triggered by the combination of an LLVM update and this change:
> >
> > -#include <linux/bpf.h>
> > +#include "vmlinux.h"
> >
> > With an older LLVM, this used to work.
> > With a recent LLVM, the change of header causes those 3 lines to get
> > compiled differently:
> >
> > if (!ctx->sk)
> >      return 1;
> > p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> >
> > When including linux/bpf.h
> > ; if (!ctx->sk)
> >         5: 79 62 b8 00 00 00 00 00 r2 = *(u64 *)(r6 + 184)
> >         6: 15 02 10 00 00 00 00 00 if r2 == 0 goto +16 <LBB1_6>
> > ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> >         7: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
> >         9: b7 03 00 00 00 00 00 00 r3 = 0
> >        10: b7 04 00 00 00 00 00 00 r4 = 0
> >        11: 85 00 00 00 6b 00 00 00 call 107
> >        12: bf 07 00 00 00 00 00 00 r7 = r0
> >
> > When including vmlinux.h
> > ; if (!ctx->sk)
> >         5: 79 61 b8 00 00 00 00 00 r1 = *(u64 *)(r6 + 184)
> >         6: 15 01 11 00 00 00 00 00 if r1 == 0 goto +17 <LBB1_6>
> > ; p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
> >         7: 79 62 b8 00 00 00 00 00 r2 = *(u64 *)(r6 + 184)
> >         8: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
> >        10: b7 03 00 00 00 00 00 00 r3 = 0
> >        11: b7 04 00 00 00 00 00 00 r4 = 0
> >        12: 85 00 00 00 6b 00 00 00 call 107
> >        13: bf 07 00 00 00 00 00 00 r7 = r0
> >
> > Note that ctx->sk gets fetched once in the first case (l5), and twice
> > in the second case (l5 and l7).
> > I'm assuming that struct bpf_sock_ops gets defined with different
> > attributes in vmlinux.h and causes LLVM to assume that ctx->sk could
> > have changed between the time of check and the time of use so it
> > yields two fetches and the verifier can't track that r2 is non null.
> >
> > If I save ctx->sk in a temporary variable first:
> >
> > struct bpf_sock *sk = ctx->sk;
> > if (!sk)
> >      return 1;
> > p = bpf_sk_storage_get(&socket_cookies, sk, 0, 0);
> >
> > this loads correctly. However, Brendan pointed out that this is also a
> > weak guarantee and that LLVM could still decide to compile this into
> > two fetches, Brendan suggested that we save sk out of an access to a
> > volatile pointer to ctx, what do you think ?
>
> Your above workaround should be good. Compiler should not do *bad*
> optimizations to have two fetches if the source code just has one
> in the above case. If this happens, it will be a llvm bug.
>
> The latest llvm trunk can reproduce the above issue. It is due to
> (1). llvm's partial (not complete) CSE and (2). this partial CSE
> resulted in llvm BPF backend generating two CORE_MEM operations (for
> CORE relocations) instead of one. If llvm will do complete cse like the
> above your code, we will be fine.
>
> Although fixing llvm CSE is desired, it may take
> some time. At the same time, please use your above source workaround.
> Thanks.

Good to know! Thank you Yonghong :)

Re: [PATCH bpf-next v5 1/4] bpf: Be less specific about socket cookies guarantees

[KP Singh]

On Tue, Jan 19, 2021 at 5:00 PM Florent Revest <revest@chromium.org> wrote:
>
> 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>