* [PATCH bpf-next 1/3] bpf, sk_msg: Let ULP restore sk_proto and write_space callback
2020-02-17 12:15 [PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks Jakub Sitnicki
@ 2020-02-17 12:15 ` Jakub Sitnicki
2020-02-19 6:09 ` John Fastabend
2020-02-17 12:15 ` [PATCH bpf-next 2/3] bpf, sk_msg: Don't clear saved sock proto on restore Jakub Sitnicki
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Jakub Sitnicki @ 2020-02-17 12:15 UTC (permalink / raw)
To: bpf; +Cc: netdev, kernel-team, Daniel Borkmann, John Fastabend
We don't need a fallback for when the socket is not using ULP.
tcp_update_ulp handles this case exactly the same as we do in
sk_psock_restore_proto. Get rid of the duplicated code.
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
include/linux/skmsg.h | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 14d61bba0b79..8605947d6c08 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -361,16 +361,7 @@ static inline void sk_psock_restore_proto(struct sock *sk,
sk->sk_prot->unhash = psock->saved_unhash;
if (psock->sk_proto) {
- struct inet_connection_sock *icsk = inet_csk(sk);
- bool has_ulp = !!icsk->icsk_ulp_data;
-
- if (has_ulp) {
- tcp_update_ulp(sk, psock->sk_proto,
- psock->saved_write_space);
- } else {
- sk->sk_prot = psock->sk_proto;
- sk->sk_write_space = psock->saved_write_space;
- }
+ tcp_update_ulp(sk, psock->sk_proto, psock->saved_write_space);
psock->sk_proto = NULL;
} else {
sk->sk_write_space = psock->saved_write_space;
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH bpf-next 2/3] bpf, sk_msg: Don't clear saved sock proto on restore
2020-02-17 12:15 [PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks Jakub Sitnicki
2020-02-17 12:15 ` [PATCH bpf-next 1/3] bpf, sk_msg: Let ULP restore sk_proto and write_space callback Jakub Sitnicki
@ 2020-02-17 12:15 ` Jakub Sitnicki
2020-02-19 6:19 ` John Fastabend
2020-02-17 12:15 ` [PATCH bpf-next 3/3] selftests/bpf: Test unhashing kTLS socket after removing from map Jakub Sitnicki
2020-02-19 17:36 ` [PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks Daniel Borkmann
3 siblings, 1 reply; 8+ messages in thread
From: Jakub Sitnicki @ 2020-02-17 12:15 UTC (permalink / raw)
To: bpf; +Cc: netdev, kernel-team, Daniel Borkmann, John Fastabend
There is no need to clear psock->sk_proto when restoring socket protocol
callbacks in sk->sk_prot. The psock is about to get detached from the sock
and eventually destroyed. At worst we will restore the protocol callbacks
and the write callback twice.
This makes reasoning about psock state easier. Once psock is initialized,
we can count on psock->sk_proto always being set.
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
include/linux/skmsg.h | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 8605947d6c08..d90ef61712a1 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -359,13 +359,7 @@ static inline void sk_psock_restore_proto(struct sock *sk,
struct sk_psock *psock)
{
sk->sk_prot->unhash = psock->saved_unhash;
-
- if (psock->sk_proto) {
- tcp_update_ulp(sk, psock->sk_proto, psock->saved_write_space);
- psock->sk_proto = NULL;
- } else {
- sk->sk_write_space = psock->saved_write_space;
- }
+ tcp_update_ulp(sk, psock->sk_proto, psock->saved_write_space);
}
static inline void sk_psock_set_state(struct sk_psock *psock,
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH bpf-next 3/3] selftests/bpf: Test unhashing kTLS socket after removing from map
2020-02-17 12:15 [PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks Jakub Sitnicki
2020-02-17 12:15 ` [PATCH bpf-next 1/3] bpf, sk_msg: Let ULP restore sk_proto and write_space callback Jakub Sitnicki
2020-02-17 12:15 ` [PATCH bpf-next 2/3] bpf, sk_msg: Don't clear saved sock proto on restore Jakub Sitnicki
@ 2020-02-17 12:15 ` Jakub Sitnicki
2020-02-19 6:22 ` John Fastabend
2020-02-19 17:36 ` [PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks Daniel Borkmann
3 siblings, 1 reply; 8+ messages in thread
From: Jakub Sitnicki @ 2020-02-17 12:15 UTC (permalink / raw)
To: bpf; +Cc: netdev, kernel-team, Daniel Borkmann, John Fastabend
When a TCP socket gets inserted into a sockmap, its sk_prot callbacks get
replaced with tcp_bpf callbacks built from regular tcp callbacks. If TLS
gets enabled on the same socket, sk_prot callbacks get replaced once again,
this time with kTLS callbacks built from tcp_bpf callbacks.
Now, we allow removing a socket from a sockmap that has kTLS enabled. After
removal, socket remains with kTLS configured. This is where things things
get tricky.
Since the socket has a set of sk_prot callbacks that are a mix of kTLS and
tcp_bpf callbacks, we need to restore just the tcp_bpf callbacks to the
original ones. At the moment, it comes down to the the unhash operation.
We had a regression recently because tcp_bpf callbacks were not cleared in
this particular scenario of removing a kTLS socket from a sockmap. It got
fixed in commit 4da6a196f93b ("bpf: Sockmap/tls, during free we may call
tcp_bpf_unhash() in loop").
Add a test that triggers the regression so that we don't reintroduce it in
the future.
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
.../selftests/bpf/prog_tests/sockmap_ktls.c | 123 ++++++++++++++++++
1 file changed, 123 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
new file mode 100644
index 000000000000..589b50c91b96
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Cloudflare
+/*
+ * Tests for sockmap/sockhash holding kTLS sockets.
+ */
+
+#include "test_progs.h"
+
+#define MAX_TEST_NAME 80
+
+static int tcp_server(int family)
+{
+ int err, s;
+
+ s = socket(family, SOCK_STREAM, 0);
+ if (CHECK_FAIL(s == -1)) {
+ perror("socket");
+ return -1;
+ }
+
+ err = listen(s, SOMAXCONN);
+ if (CHECK_FAIL(err)) {
+ perror("listen");
+ return -1;
+ }
+
+ return s;
+}
+
+static int disconnect(int fd)
+{
+ struct sockaddr unspec = { AF_UNSPEC };
+
+ return connect(fd, &unspec, sizeof(unspec));
+}
+
+/* Disconnect (unhash) a kTLS socket after removing it from sockmap. */
+static void test_sockmap_ktls_disconnect_after_delete(int family, int map)
+{
+ struct sockaddr_storage addr = {0};
+ socklen_t len = sizeof(addr);
+ int err, cli, srv, zero = 0;
+
+ srv = tcp_server(family);
+ if (srv == -1)
+ return;
+
+ err = getsockname(srv, (struct sockaddr *)&addr, &len);
+ if (CHECK_FAIL(err)) {
+ perror("getsockopt");
+ goto close_srv;
+ }
+
+ cli = socket(family, SOCK_STREAM, 0);
+ if (CHECK_FAIL(cli == -1)) {
+ perror("socket");
+ goto close_srv;
+ }
+
+ err = connect(cli, (struct sockaddr *)&addr, len);
+ if (CHECK_FAIL(err)) {
+ perror("connect");
+ goto close_cli;
+ }
+
+ err = bpf_map_update_elem(map, &zero, &cli, 0);
+ if (CHECK_FAIL(err)) {
+ perror("bpf_map_update_elem");
+ goto close_cli;
+ }
+
+ err = setsockopt(cli, IPPROTO_TCP, TCP_ULP, "tls", strlen("tls"));
+ if (CHECK_FAIL(err)) {
+ perror("setsockopt(TCP_ULP)");
+ goto close_cli;
+ }
+
+ err = bpf_map_delete_elem(map, &zero);
+ if (CHECK_FAIL(err)) {
+ perror("bpf_map_delete_elem");
+ goto close_cli;
+ }
+
+ err = disconnect(cli);
+ if (CHECK_FAIL(err))
+ perror("disconnect");
+
+close_cli:
+ close(cli);
+close_srv:
+ close(srv);
+}
+
+static void run_tests(int family, enum bpf_map_type map_type)
+{
+ char test_name[MAX_TEST_NAME];
+ int map;
+
+ map = bpf_create_map(map_type, sizeof(int), sizeof(int), 1, 0);
+ if (CHECK_FAIL(map == -1)) {
+ perror("bpf_map_create");
+ return;
+ }
+
+ snprintf(test_name, MAX_TEST_NAME,
+ "sockmap_ktls disconnect_after_delete %s %s",
+ family == AF_INET ? "IPv4" : "IPv6",
+ map_type == BPF_MAP_TYPE_SOCKMAP ? "SOCKMAP" : "SOCKHASH");
+ if (!test__start_subtest(test_name))
+ return;
+
+ test_sockmap_ktls_disconnect_after_delete(family, map);
+
+ close(map);
+}
+
+void test_sockmap_ktls(void)
+{
+ run_tests(AF_INET, BPF_MAP_TYPE_SOCKMAP);
+ run_tests(AF_INET, BPF_MAP_TYPE_SOCKHASH);
+ run_tests(AF_INET6, BPF_MAP_TYPE_SOCKMAP);
+ run_tests(AF_INET6, BPF_MAP_TYPE_SOCKHASH);
+}
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks
2020-02-17 12:15 [PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks Jakub Sitnicki
` (2 preceding siblings ...)
2020-02-17 12:15 ` [PATCH bpf-next 3/3] selftests/bpf: Test unhashing kTLS socket after removing from map Jakub Sitnicki
@ 2020-02-19 17:36 ` Daniel Borkmann
3 siblings, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2020-02-19 17:36 UTC (permalink / raw)
To: Jakub Sitnicki, bpf; +Cc: netdev, kernel-team, John Fastabend
On 2/17/20 1:15 PM, Jakub Sitnicki wrote:
> This series has been split out from "Extend SOCKMAP to store listening
> sockets" [0]. I think it stands on its own, and makes the latter series
> smaller, which will make the review easier, hopefully.
>
> The essence is that we don't need to do a complicated dance in
> sk_psock_restore_proto, if we agree that the contract with tcp_update_ulp
> is to restore callbacks even when the socket doesn't use ULP. This is what
> tcp_update_ulp currently does, and we just make use of it.
>
> Series is accompanied by a test for a particularly tricky case of restoring
> callbacks when we have both sockmap and tls callbacks configured in
> sk->sk_prot.
>
> [0] https://lore.kernel.org/bpf/20200127131057.150941-1-jakub@cloudflare.com/
>
>
> Jakub Sitnicki (3):
> bpf, sk_msg: Let ULP restore sk_proto and write_space callback
> bpf, sk_msg: Don't clear saved sock proto on restore
> selftests/bpf: Test unhashing kTLS socket after removing from map
>
> include/linux/skmsg.h | 17 +--
> .../selftests/bpf/prog_tests/sockmap_ktls.c | 123 ++++++++++++++++++
> 2 files changed, 124 insertions(+), 16 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
>
Applied, thanks!
^ permalink raw reply [flat|nested] 8+ messages in thread