[PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks

[PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks

[Jakub Sitnicki]

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

-- 
2.24.1

[PATCH bpf-next 1/3] bpf, sk_msg: Let ULP restore sk_proto and write_space callback

[Jakub Sitnicki]

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

[PATCH bpf-next 2/3] bpf, sk_msg: Don't clear saved sock proto on restore

[Jakub Sitnicki]

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

[PATCH bpf-next 3/3] selftests/bpf: Test unhashing kTLS socket after removing from map

[Jakub Sitnicki]

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

RE: [PATCH bpf-next 1/3] bpf, sk_msg: Let ULP restore sk_proto and write_space callback

[John Fastabend]

Jakub Sitnicki wrote:
> 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
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [PATCH bpf-next 2/3] bpf, sk_msg: Don't clear saved sock proto on restore

[John Fastabend]

Jakub Sitnicki wrote:
> 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
> 

Agreed, also the next line in sk_psock_drop is to NULL user data so
the psock will no longer be attached as far as the sock is concerned.

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [PATCH bpf-next 3/3] selftests/bpf: Test unhashing kTLS socket after removing from map

[John Fastabend]

Jakub Sitnicki wrote:
> 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
> 

I'll push the patches I have on my stack to run some more of the
sockmap tests with ktls this week as well to get our coverage up.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next 0/3] sockmap/ktls: Simplify how we restore sk_prot callbacks

[Daniel Borkmann]

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!