[PATCH net 1/2] net/tls: partially revert fix transition through disconnect with close

[PATCH net 1/2] net/tls: partially revert fix transition through disconnect with close

[Jakub Kicinski]

Looks like we were slightly overzealous with the shutdown()
cleanup. Even though the sock->sk_state can reach CLOSED again,
socket->state will not got back to SS_UNCONNECTED once
connections is ESTABLISHED. Meaning we will see EISCONN if
we try to reconnect, and EINVAL if we try to listen.

Only listen sockets can be shutdown() and reused, but since
ESTABLISHED sockets can never be re-connected() or used for
listen() we don't need to try to clean up the ULP state early.

Fixes: 32857cf57f92 ("net/tls: fix transition through disconnect with close")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 Documentation/networking/tls-offload.rst |  6 ---
 include/net/tls.h                        |  2 -
 net/tls/tls_main.c                       | 55 ------------------------
 3 files changed, 63 deletions(-)

diff --git a/Documentation/networking/tls-offload.rst b/Documentation/networking/tls-offload.rst
index 2d9f9ebf4117..b70b70dc4524 100644
--- a/Documentation/networking/tls-offload.rst
+++ b/Documentation/networking/tls-offload.rst
@@ -524,9 +524,3 @@ Redirects leak clear text
 
 In the RX direction, if segment has already been decrypted by the device
 and it gets redirected or mirrored - clear text will be transmitted out.
-
-shutdown() doesn't clear TLS state
-----------------------------------
-
-shutdown() system call allows for a TLS socket to be reused as a different
-connection. Offload doesn't currently handle that.
diff --git a/include/net/tls.h b/include/net/tls.h
index 9e425ac2de45..41b2d41bb1b8 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -290,8 +290,6 @@ struct tls_context {
 
 	struct list_head list;
 	refcount_t refcount;
-
-	struct work_struct gc;
 };
 
 enum tls_offload_ctx_dir {
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index f208f8455ef2..9cbbae606ced 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -261,33 +261,6 @@ void tls_ctx_free(struct tls_context *ctx)
 	kfree(ctx);
 }
 
-static void tls_ctx_free_deferred(struct work_struct *gc)
-{
-	struct tls_context *ctx = container_of(gc, struct tls_context, gc);
-
-	/* Ensure any remaining work items are completed. The sk will
-	 * already have lost its tls_ctx reference by the time we get
-	 * here so no xmit operation will actually be performed.
-	 */
-	if (ctx->tx_conf == TLS_SW) {
-		tls_sw_cancel_work_tx(ctx);
-		tls_sw_free_ctx_tx(ctx);
-	}
-
-	if (ctx->rx_conf == TLS_SW) {
-		tls_sw_strparser_done(ctx);
-		tls_sw_free_ctx_rx(ctx);
-	}
-
-	tls_ctx_free(ctx);
-}
-
-static void tls_ctx_free_wq(struct tls_context *ctx)
-{
-	INIT_WORK(&ctx->gc, tls_ctx_free_deferred);
-	schedule_work(&ctx->gc);
-}
-
 static void tls_sk_proto_cleanup(struct sock *sk,
 				 struct tls_context *ctx, long timeo)
 {
@@ -315,29 +288,6 @@ static void tls_sk_proto_cleanup(struct sock *sk,
 #endif
 }
 
-static void tls_sk_proto_unhash(struct sock *sk)
-{
-	struct inet_connection_sock *icsk = inet_csk(sk);
-	long timeo = sock_sndtimeo(sk, 0);
-	struct tls_context *ctx;
-
-	if (unlikely(!icsk->icsk_ulp_data)) {
-		if (sk->sk_prot->unhash)
-			sk->sk_prot->unhash(sk);
-	}
-
-	ctx = tls_get_ctx(sk);
-	tls_sk_proto_cleanup(sk, ctx, timeo);
-	write_lock_bh(&sk->sk_callback_lock);
-	icsk->icsk_ulp_data = NULL;
-	sk->sk_prot = ctx->sk_proto;
-	write_unlock_bh(&sk->sk_callback_lock);
-
-	if (ctx->sk_proto->unhash)
-		ctx->sk_proto->unhash(sk);
-	tls_ctx_free_wq(ctx);
-}
-
 static void tls_sk_proto_close(struct sock *sk, long timeout)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
@@ -786,7 +736,6 @@ static void build_protos(struct proto prot[TLS_NUM_CONFIG][TLS_NUM_CONFIG],
 	prot[TLS_BASE][TLS_BASE].setsockopt	= tls_setsockopt;
 	prot[TLS_BASE][TLS_BASE].getsockopt	= tls_getsockopt;
 	prot[TLS_BASE][TLS_BASE].close		= tls_sk_proto_close;
-	prot[TLS_BASE][TLS_BASE].unhash		= tls_sk_proto_unhash;
 
 	prot[TLS_SW][TLS_BASE] = prot[TLS_BASE][TLS_BASE];
 	prot[TLS_SW][TLS_BASE].sendmsg		= tls_sw_sendmsg;
@@ -804,20 +753,16 @@ static void build_protos(struct proto prot[TLS_NUM_CONFIG][TLS_NUM_CONFIG],
 
 #ifdef CONFIG_TLS_DEVICE
 	prot[TLS_HW][TLS_BASE] = prot[TLS_BASE][TLS_BASE];
-	prot[TLS_HW][TLS_BASE].unhash		= base->unhash;
 	prot[TLS_HW][TLS_BASE].sendmsg		= tls_device_sendmsg;
 	prot[TLS_HW][TLS_BASE].sendpage		= tls_device_sendpage;
 
 	prot[TLS_HW][TLS_SW] = prot[TLS_BASE][TLS_SW];
-	prot[TLS_HW][TLS_SW].unhash		= base->unhash;
 	prot[TLS_HW][TLS_SW].sendmsg		= tls_device_sendmsg;
 	prot[TLS_HW][TLS_SW].sendpage		= tls_device_sendpage;
 
 	prot[TLS_BASE][TLS_HW] = prot[TLS_BASE][TLS_SW];
-	prot[TLS_BASE][TLS_HW].unhash		= base->unhash;
 
 	prot[TLS_SW][TLS_HW] = prot[TLS_SW][TLS_SW];
-	prot[TLS_SW][TLS_HW].unhash		= base->unhash;
 
 	prot[TLS_HW][TLS_HW] = prot[TLS_HW][TLS_SW];
 #endif
-- 
2.21.0

[PATCH net 2/2] selftests/tls: add a litmus test for the socket reuse through shutdown

[Jakub Kicinski]

Make sure that shutdown never works, and at the same time document how
I tested to came to the conclusion that currently reuse is not possible.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/testing/selftests/net/tls.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index d995e6503b1a..4c285b6e1db8 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -984,6 +984,30 @@ TEST_F(tls, shutdown_unsent)
 	shutdown(self->cfd, SHUT_RDWR);
 }
 
+TEST_F(tls, shutdown_reuse)
+{
+	struct sockaddr_in addr;
+	int ret;
+
+	shutdown(self->fd, SHUT_RDWR);
+	shutdown(self->cfd, SHUT_RDWR);
+	close(self->cfd);
+
+	addr.sin_family = AF_INET;
+	addr.sin_addr.s_addr = htonl(INADDR_ANY);
+	addr.sin_port = 0;
+
+	ret = bind(self->fd, &addr, sizeof(addr));
+	EXPECT_EQ(ret, 0);
+	ret = listen(self->fd, 10);
+	EXPECT_EQ(ret, -1);
+	EXPECT_EQ(errno, EINVAL);
+
+	ret = connect(self->fd, &addr, sizeof(addr));
+	EXPECT_EQ(ret, -1);
+	EXPECT_EQ(errno, EISCONN);
+}
+
 TEST(non_established) {
 	struct tls12_crypto_info_aes_gcm_256 tls12;
 	struct sockaddr_in addr;
-- 
2.21.0

RE: [PATCH net 1/2] net/tls: partially revert fix transition through disconnect with close

[John Fastabend]

Jakub Kicinski wrote:
> Looks like we were slightly overzealous with the shutdown()
> cleanup. Even though the sock->sk_state can reach CLOSED again,
> socket->state will not got back to SS_UNCONNECTED once
> connections is ESTABLISHED. Meaning we will see EISCONN if
> we try to reconnect, and EINVAL if we try to listen.
> 
> Only listen sockets can be shutdown() and reused, but since
> ESTABLISHED sockets can never be re-connected() or used for
> listen() we don't need to try to clean up the ULP state early.
> 
> Fixes: 32857cf57f92 ("net/tls: fix transition through disconnect with close")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---

Thanks Jakub this looks reasonable to me I'm going to run some tests
with sockmap + ktls on it this weekend to be sure the two work
together with some of our applications. I believe the original
series should be enough so that BPF can be safely unloaded out
from underneath ktls now.

I guess we could do something similar on the sockmap side but I
do want to loosen the restrictions there at some point so might
be best just to keep it as is.

Thanks,
John

Re: [PATCH net 1/2] net/tls: partially revert fix transition through disconnect with close

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu,  1 Aug 2019 14:36:01 -0700

> Looks like we were slightly overzealous with the shutdown()
> cleanup. Even though the sock->sk_state can reach CLOSED again,
> socket->state will not got back to SS_UNCONNECTED once
> connections is ESTABLISHED. Meaning we will see EISCONN if
> we try to reconnect, and EINVAL if we try to listen.
> 
> Only listen sockets can be shutdown() and reused, but since
> ESTABLISHED sockets can never be re-connected() or used for
> listen() we don't need to try to clean up the ULP state early.
> 
> Fixes: 32857cf57f92 ("net/tls: fix transition through disconnect with close")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Applied and queued up for -stable.

Re: [PATCH net 2/2] selftests/tls: add a litmus test for the socket reuse through shutdown

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu,  1 Aug 2019 14:36:02 -0700

> Make sure that shutdown never works, and at the same time document how
> I tested to came to the conclusion that currently reuse is not possible.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Applied.

Re: [PATCH net 1/2] net/tls: partially revert fix transition through disconnect with close

[John Fastabend]

David Miller wrote:
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Thu,  1 Aug 2019 14:36:01 -0700
> 
> > Looks like we were slightly overzealous with the shutdown()
> > cleanup. Even though the sock->sk_state can reach CLOSED again,
> > socket->state will not got back to SS_UNCONNECTED once
> > connections is ESTABLISHED. Meaning we will see EISCONN if
> > we try to reconnect, and EINVAL if we try to listen.
> > 
> > Only listen sockets can be shutdown() and reused, but since
> > ESTABLISHED sockets can never be re-connected() or used for
> > listen() we don't need to try to clean up the ULP state early.
> > 
> > Fixes: 32857cf57f92 ("net/tls: fix transition through disconnect with close")
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> 
> Applied and queued up for -stable.

Bit late but, I went back and ran some of the syzbot tests that
were failing before original series and most of my ktls+bpf tests
and everything seems in good shape now. There is still one issue
with crypto stack that I'll look at fixing now. Thanks.

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