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 <[email protected]>
Signed-off-by: Florent Revest <[email protected]>
Acked-by: KP Singh <[email protected]>
---
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
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 <[email protected]>
Acked-by: KP Singh <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Florent Revest <[email protected]>
---
.../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
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 <[email protected]>
Acked-by: KP Singh <[email protected]>
---
.../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
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 <[email protected]>
Acked-by: KP Singh <[email protected]>
---
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
On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <[email protected]> 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 <[email protected]>
> Signed-off-by: Florent Revest <[email protected]>
> ---
Acked-by: Andrii Nakryiko <[email protected]>
> .../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
>
On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <[email protected]> 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 <[email protected]>
> Acked-by: KP Singh <[email protected]>
> ---
> 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
>
On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <[email protected]> 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 <[email protected]>
> Acked-by: KP Singh <[email protected]>
> ---
LGTM.
Acked-by: Andrii Nakryiko <[email protected]>
> 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
>
[...]
On Wed, Jan 27, 2021 at 10:01 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <[email protected]> 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 <[email protected]>
> > Acked-by: KP Singh <[email protected]>
> > ---
> > 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
On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>> Acked-by: KP Singh <[email protected]>
> >>> ---
> >>> 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.
On 1/30/21 12:45 PM, Florent Revest wrote:
> On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>> Acked-by: KP Singh <[email protected]>
>>>>> ---
>>>>> 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/[email protected]/
On Mon, Feb 1, 2021 at 2:32 PM Daniel Borkmann <[email protected]> wrote:
>
> On 1/30/21 12:45 PM, Florent Revest wrote:
> > On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>> Acked-by: KP Singh <[email protected]>
> >>>>> ---
> >>>>> 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.
On Mon, Feb 1, 2021 at 11:37 PM Alexei Starovoitov
<[email protected]> wrote:
>
> On Mon, Feb 1, 2021 at 2:32 PM Daniel Borkmann <[email protected]> wrote:
> >
> > On 1/30/21 12:45 PM, Florent Revest wrote:
> > > On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >>>>> Acked-by: KP Singh <[email protected]>
> > >>>>> ---
> > >>>>> 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.