[PATCH net-next 00/10] tls: rx: random refactoring part 3

[PATCH net-next 00/10] tls: rx: random refactoring part 3

[Jakub Kicinski]

TLS Rx refactoring. Part 3 of 3. This set is mostly around rx_list
and async processing. The last two patches are minor optimizations.
A couple of features to follow.

Jakub Kicinski (10):
  tls: rx: consistently use unlocked accessors for rx_list
  tls: rx: reuse leave_on_list label for psock
  tls: rx: move counting TlsDecryptErrors for sync
  tls: rx: don't handle TLS 1.3 in the async crypto callback
  tls: rx: assume crypto always calls our callback
  tls: rx: treat process_rx_list() errors as transient
  tls: rx: return the already-copied data on crypto error
  tls: rx: use async as an in-out argument
  tls: rx: use MAX_IV_SIZE for allocations
  tls: rx: only copy IV from the packet for TLS 1.2

 net/tls/tls_sw.c | 131 ++++++++++++++++++++++-------------------------
 1 file changed, 60 insertions(+), 71 deletions(-)

-- 
2.34.1

[PATCH net-next 01/10] tls: rx: consistently use unlocked accessors for rx_list

[Jakub Kicinski]

rx_list is protected by the socket lock, no need to take
the built-in spin lock on accesses.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 2e8a896af81a..e176549216ac 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1709,7 +1709,7 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
 		next_skb = skb_peek_next(skb, &ctx->rx_list);
 
 		if (!is_peek) {
-			skb_unlink(skb, &ctx->rx_list);
+			__skb_unlink(skb, &ctx->rx_list);
 			consume_skb(skb);
 		}
 
@@ -1827,7 +1827,7 @@ int tls_sw_recvmsg(struct sock *sk,
 
 		ctx->recv_pkt = NULL;
 		__strp_unpause(&ctx->strp);
-		skb_queue_tail(&ctx->rx_list, skb);
+		__skb_queue_tail(&ctx->rx_list, skb);
 
 		if (async) {
 			/* TLS 1.2-only, to_decrypt must be text length */
@@ -1848,7 +1848,7 @@ int tls_sw_recvmsg(struct sock *sk,
 				if (err != __SK_PASS) {
 					rxm->offset = rxm->offset + rxm->full_len;
 					rxm->full_len = 0;
-					skb_unlink(skb, &ctx->rx_list);
+					__skb_unlink(skb, &ctx->rx_list);
 					if (err == __SK_DROP)
 						consume_skb(skb);
 					continue;
@@ -1876,7 +1876,7 @@ int tls_sw_recvmsg(struct sock *sk,
 		decrypted += chunk;
 		len -= chunk;
 
-		skb_unlink(skb, &ctx->rx_list);
+		__skb_unlink(skb, &ctx->rx_list);
 		consume_skb(skb);
 
 		/* Return full control message to userspace before trying
@@ -2176,7 +2176,7 @@ void tls_sw_release_resources_rx(struct sock *sk)
 	if (ctx->aead_recv) {
 		kfree_skb(ctx->recv_pkt);
 		ctx->recv_pkt = NULL;
-		skb_queue_purge(&ctx->rx_list);
+		__skb_queue_purge(&ctx->rx_list);
 		crypto_free_aead(ctx->aead_recv);
 		strp_stop(&ctx->strp);
 		/* If tls_sw_strparser_arm() was not called (cleanup paths)
-- 
2.34.1

[PATCH net-next 02/10] tls: rx: reuse leave_on_list label for psock

[Jakub Kicinski]

The code is identical, we can save a few LoC.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index e176549216ac..42ac605d48bb 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1778,14 +1778,10 @@ int tls_sw_recvmsg(struct sock *sk,
 		skb = tls_wait_data(sk, psock, flags & MSG_DONTWAIT, timeo, &err);
 		if (!skb) {
 			if (psock) {
-				int ret = sk_msg_recvmsg(sk, psock, msg, len,
-							 flags);
-
-				if (ret > 0) {
-					decrypted += ret;
-					len -= ret;
-					continue;
-				}
+				chunk = sk_msg_recvmsg(sk, psock, msg, len,
+						       flags);
+				if (chunk > 0)
+					goto leave_on_list;
 			}
 			goto recv_end;
 		}
-- 
2.34.1

[PATCH net-next 03/10] tls: rx: move counting TlsDecryptErrors for sync

[Jakub Kicinski]

Move counting TlsDecryptErrors to tls_do_decryption()
where differences between sync and async crypto are
reconciled.

No functional changes, this code just always gave
me a pause.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 42ac605d48bb..c8378396e616 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -270,6 +270,8 @@ static int tls_do_decryption(struct sock *sk,
 
 		ret = crypto_wait_req(ret, &ctx->async_wait);
 	}
+	if (ret == -EBADMSG)
+		TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSDECRYPTERROR);
 
 	if (async)
 		atomic_dec(&ctx->decrypt_pending);
@@ -1584,8 +1586,6 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 	if (err < 0) {
 		if (err == -EINPROGRESS)
 			tls_advance_record_sn(sk, prot, &tls_ctx->rx);
-		else if (err == -EBADMSG)
-			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSDECRYPTERROR);
 		return err;
 	}
 
-- 
2.34.1

[PATCH net-next 04/10] tls: rx: don't handle TLS 1.3 in the async crypto callback

[Jakub Kicinski]

Async crypto never worked with TLS 1.3 and was explicitly disabled in
commit 8497ded2d16c ("net/tls: Disable async decrytion for tls1.3").
There's no need for us to handle TLS 1.3 padding in the async cb.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index c8378396e616..b3a15dc3d4eb 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -188,17 +188,12 @@ static void tls_decrypt_done(struct crypto_async_request *req, int err)
 		tls_err_abort(skb->sk, err);
 	} else {
 		struct strp_msg *rxm = strp_msg(skb);
-		int pad;
 
-		pad = padding_length(prot, skb);
-		if (pad < 0) {
-			ctx->async_wait.err = pad;
-			tls_err_abort(skb->sk, pad);
-		} else {
-			rxm->full_len -= pad;
-			rxm->offset += prot->prepend_size;
-			rxm->full_len -= prot->overhead_size;
-		}
+		/* No TLS 1.3 support with async crypto */
+		WARN_ON(prot->tail_size);
+
+		rxm->offset += prot->prepend_size;
+		rxm->full_len -= prot->overhead_size;
 	}
 
 	/* After using skb->sk to propagate sk through crypto async callback
-- 
2.34.1

[PATCH net-next 05/10] tls: rx: assume crypto always calls our callback

[Jakub Kicinski]

If crypto didn't always invoke our callback for async
we'd not be clearing skb->sk and would crash in the
skb core when freeing it. This if must be dead code.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index b3a15dc3d4eb..fcecf4ef8922 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -268,9 +268,6 @@ static int tls_do_decryption(struct sock *sk,
 	if (ret == -EBADMSG)
 		TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSDECRYPTERROR);
 
-	if (async)
-		atomic_dec(&ctx->decrypt_pending);
-
 	return ret;
 }
 
-- 
2.34.1

[PATCH net-next 06/10] tls: rx: treat process_rx_list() errors as transient

[Jakub Kicinski]

process_rx_list() only fails if it can't copy data to user
space. There is no point recording the error onto sk->sk_err
or giving up on the data which was read partially. Treat
the return value like a normal socket partial read.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index fcecf4ef8922..bba69706aea9 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1650,7 +1650,7 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
 
 		err = tls_record_content_type(msg, tlm, control);
 		if (err <= 0)
-			return err;
+			goto out;
 
 		if (skip < rxm->full_len)
 			break;
@@ -1668,13 +1668,13 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
 
 		err = tls_record_content_type(msg, tlm, control);
 		if (err <= 0)
-			return err;
+			goto out;
 
 		if (!zc || (rxm->full_len - skip) > len) {
 			err = skb_copy_datagram_msg(skb, rxm->offset + skip,
 						    msg, chunk);
 			if (err < 0)
-				return err;
+				goto out;
 		}
 
 		len = len - chunk;
@@ -1707,8 +1707,10 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
 
 		skb = next_skb;
 	}
+	err = 0;
 
-	return copied;
+out:
+	return copied ? : err;
 }
 
 int tls_sw_recvmsg(struct sock *sk,
@@ -1747,10 +1749,8 @@ int tls_sw_recvmsg(struct sock *sk,
 
 	/* Process pending decrypted records. It must be non-zero-copy */
 	err = process_rx_list(ctx, msg, &control, 0, len, false, is_peek);
-	if (err < 0) {
-		tls_err_abort(sk, err);
+	if (err < 0)
 		goto end;
-	}
 
 	copied = err;
 	if (len <= copied)
@@ -1902,11 +1902,7 @@ int tls_sw_recvmsg(struct sock *sk,
 		else
 			err = process_rx_list(ctx, msg, &control, 0,
 					      decrypted, true, is_peek);
-		if (err < 0) {
-			tls_err_abort(sk, err);
-			copied = 0;
-			goto end;
-		}
+		decrypted = max(err, 0);
 	}
 
 	copied += decrypted;
-- 
2.34.1

[PATCH net-next 07/10] tls: rx: return the already-copied data on crypto error

[Jakub Kicinski]

async crypto handler will report the socket error no need
to report it again. We can, however, let the data we already
copied be reported to user space but we need to make sure
the error will be reported next time around.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index bba69706aea9..b5d1393aa8d4 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1747,6 +1747,11 @@ int tls_sw_recvmsg(struct sock *sk,
 	lock_sock(sk);
 	bpf_strp_enabled = sk_psock_strp_enabled(psock);
 
+	/* If crypto failed the connection is broken */
+	err = ctx->async_wait.err;
+	if (err)
+		goto end;
+
 	/* Process pending decrypted records. It must be non-zero-copy */
 	err = process_rx_list(ctx, msg, &control, 0, len, false, is_peek);
 	if (err < 0)
@@ -1877,7 +1882,7 @@ int tls_sw_recvmsg(struct sock *sk,
 
 recv_end:
 	if (async) {
-		int pending;
+		int ret, pending;
 
 		/* Wait for all previously submitted records to be decrypted */
 		spin_lock_bh(&ctx->decrypt_compl_lock);
@@ -1885,11 +1890,10 @@ int tls_sw_recvmsg(struct sock *sk,
 		pending = atomic_read(&ctx->decrypt_pending);
 		spin_unlock_bh(&ctx->decrypt_compl_lock);
 		if (pending) {
-			err = crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
-			if (err) {
-				/* one of async decrypt failed */
-				tls_err_abort(sk, err);
-				copied = 0;
+			ret = crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
+			if (ret) {
+				if (err >= 0 || err == -EINPROGRESS)
+					err = ret;
 				decrypted = 0;
 				goto end;
 			}
-- 
2.34.1

[PATCH net-next 08/10] tls: rx: use async as an in-out argument

[Jakub Kicinski]

Propagating EINPROGRESS thru multiple layers of functions is
error prone. Use darg->async as an in/out argument, like we
use darg->zc today. On input it tells the code if async is
allowed, on output if it took place.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index b5d1393aa8d4..6b906b0cb2fd 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -227,7 +227,7 @@ static int tls_do_decryption(struct sock *sk,
 			     char *iv_recv,
 			     size_t data_len,
 			     struct aead_request *aead_req,
-			     bool async)
+			     struct tls_decrypt_arg *darg)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
@@ -240,7 +240,7 @@ static int tls_do_decryption(struct sock *sk,
 			       data_len + prot->tag_size,
 			       (u8 *)iv_recv);
 
-	if (async) {
+	if (darg->async) {
 		/* Using skb->sk to push sk through to crypto async callback
 		 * handler. This allows propagating errors up to the socket
 		 * if needed. It _must_ be cleared in the async handler
@@ -260,11 +260,13 @@ static int tls_do_decryption(struct sock *sk,
 
 	ret = crypto_aead_decrypt(aead_req);
 	if (ret == -EINPROGRESS) {
-		if (async)
-			return ret;
+		if (darg->async)
+			return 0;
 
 		ret = crypto_wait_req(ret, &ctx->async_wait);
 	}
+	darg->async = false;
+
 	if (ret == -EBADMSG)
 		TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSDECRYPTERROR);
 
@@ -1536,9 +1538,9 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 
 	/* Prepare and submit AEAD request */
 	err = tls_do_decryption(sk, skb, sgin, sgout, iv,
-				data_len, aead_req, darg->async);
-	if (err == -EINPROGRESS)
-		return err;
+				data_len, aead_req, darg);
+	if (darg->async)
+		return 0;
 
 	/* Release the pages in case iov was mapped to pages */
 	for (; pages > 0; pages--)
@@ -1575,11 +1577,10 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 	}
 
 	err = decrypt_internal(sk, skb, dest, NULL, darg);
-	if (err < 0) {
-		if (err == -EINPROGRESS)
-			tls_advance_record_sn(sk, prot, &tls_ctx->rx);
+	if (err < 0)
 		return err;
-	}
+	if (darg->async)
+		goto decrypt_next;
 
 decrypt_done:
 	pad = padding_length(prot, skb);
@@ -1589,8 +1590,9 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 	rxm->full_len -= pad;
 	rxm->offset += prot->prepend_size;
 	rxm->full_len -= prot->overhead_size;
-	tls_advance_record_sn(sk, prot, &tls_ctx->rx);
 	tlm->decrypted = 1;
+decrypt_next:
+	tls_advance_record_sn(sk, prot, &tls_ctx->rx);
 
 	return 0;
 }
@@ -1799,13 +1801,12 @@ int tls_sw_recvmsg(struct sock *sk,
 			darg.async = false;
 
 		err = decrypt_skb_update(sk, skb, &msg->msg_iter, &darg);
-		if (err < 0 && err != -EINPROGRESS) {
+		if (err < 0) {
 			tls_err_abort(sk, -EBADMSG);
 			goto recv_end;
 		}
 
-		if (err == -EINPROGRESS)
-			async = true;
+		async |= darg.async;
 
 		/* If the type of records being processed is not known yet,
 		 * set it to record type just dequeued. If it is already known,
-- 
2.34.1

[PATCH net-next 09/10] tls: rx: use MAX_IV_SIZE for allocations

[Jakub Kicinski]

IVs are 8 or 16 bytes, no point reading out the exact value
for quantities this small.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 6b906b0cb2fd..fcbb0e078d79 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1452,7 +1452,7 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 	aead_size = sizeof(*aead_req) + crypto_aead_reqsize(ctx->aead_recv);
 	mem_size = aead_size + (nsg * sizeof(struct scatterlist));
 	mem_size = mem_size + prot->aad_size;
-	mem_size = mem_size + crypto_aead_ivsize(ctx->aead_recv);
+	mem_size = mem_size + MAX_IV_SIZE;
 
 	/* Allocate a single block of memory which contains
 	 * aead_req || sgin[] || sgout[] || aad || iv.
-- 
2.34.1

[PATCH net-next 10/10] tls: rx: only copy IV from the packet for TLS 1.2

[Jakub Kicinski]

TLS 1.3 and ChaChaPoly don't carry IV in the packet.
The code before this change would copy out iv_size
worth of whatever followed the TLS header in the packet
and then for TLS 1.3 | ChaCha overwrite that with
the sequence number. Waste of cycles especially
with TLS 1.2 being close to dead and TLS 1.3 being
the common case.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index fcbb0e078d79..9432a2c985e3 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1482,20 +1482,20 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 	}
 
 	/* Prepare IV */
-	err = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE,
-			    iv + iv_offset + prot->salt_size,
-			    prot->iv_size);
-	if (err < 0) {
-		kfree(mem);
-		return err;
-	}
 	if (prot->version == TLS_1_3_VERSION ||
-	    prot->cipher_type == TLS_CIPHER_CHACHA20_POLY1305)
+	    prot->cipher_type == TLS_CIPHER_CHACHA20_POLY1305) {
 		memcpy(iv + iv_offset, tls_ctx->rx.iv,
 		       prot->iv_size + prot->salt_size);
-	else
+	} else {
+		err = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE,
+				    iv + iv_offset + prot->salt_size,
+				    prot->iv_size);
+		if (err < 0) {
+			kfree(mem);
+			return err;
+		}
 		memcpy(iv + iv_offset, tls_ctx->rx.iv, prot->salt_size);
-
+	}
 	xor_iv_with_seq(prot, iv + iv_offset, tls_ctx->rx.rec_seq);
 
 	/* Prepare AAD */
-- 
2.34.1

Re: [PATCH net-next 00/10] tls: rx: random refactoring part 3

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 11 Apr 2022 12:19:07 -0700 you wrote:
> TLS Rx refactoring. Part 3 of 3. This set is mostly around rx_list
> and async processing. The last two patches are minor optimizations.
> A couple of features to follow.
> 
> Jakub Kicinski (10):
>   tls: rx: consistently use unlocked accessors for rx_list
>   tls: rx: reuse leave_on_list label for psock
>   tls: rx: move counting TlsDecryptErrors for sync
>   tls: rx: don't handle TLS 1.3 in the async crypto callback
>   tls: rx: assume crypto always calls our callback
>   tls: rx: treat process_rx_list() errors as transient
>   tls: rx: return the already-copied data on crypto error
>   tls: rx: use async as an in-out argument
>   tls: rx: use MAX_IV_SIZE for allocations
>   tls: rx: only copy IV from the packet for TLS 1.2
> 
> [...]

Here is the summary with links:
  - [net-next,01/10] tls: rx: consistently use unlocked accessors for rx_list
    https://git.kernel.org/netdev/net-next/c/a30295c45472
  - [net-next,02/10] tls: rx: reuse leave_on_list label for psock
    https://git.kernel.org/netdev/net-next/c/0775639ce1ca
  - [net-next,03/10] tls: rx: move counting TlsDecryptErrors for sync
    https://git.kernel.org/netdev/net-next/c/284b4d93daee
  - [net-next,04/10] tls: rx: don't handle TLS 1.3 in the async crypto callback
    https://git.kernel.org/netdev/net-next/c/72f3ad73bc86
  - [net-next,05/10] tls: rx: assume crypto always calls our callback
    https://git.kernel.org/netdev/net-next/c/1c699ffa48a1
  - [net-next,06/10] tls: rx: treat process_rx_list() errors as transient
    https://git.kernel.org/netdev/net-next/c/4dcdd971b9c7
  - [net-next,07/10] tls: rx: return the already-copied data on crypto error
    https://git.kernel.org/netdev/net-next/c/f314bfee81b1
  - [net-next,08/10] tls: rx: use async as an in-out argument
    https://git.kernel.org/netdev/net-next/c/3547a1f9d988
  - [net-next,09/10] tls: rx: use MAX_IV_SIZE for allocations
    https://git.kernel.org/netdev/net-next/c/f7d45f4b52fe
  - [net-next,10/10] tls: rx: only copy IV from the packet for TLS 1.2
    https://git.kernel.org/netdev/net-next/c/a4ae58cdb6e8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 08/10] tls: rx: use async as an in-out argument

[Gal Pressman]

On 11/04/2022 22:19, Jakub Kicinski wrote:
> Propagating EINPROGRESS thru multiple layers of functions is
> error prone. Use darg->async as an in/out argument, like we
> use darg->zc today. On input it tells the code if async is
> allowed, on output if it took place.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

I know this is not much to go on, but this patch broke our tls workflows
when device offload is enabled.
I'm still looking into it, but maybe you have an idea what might have
went wrong?

Re: [PATCH net-next 08/10] tls: rx: use async as an in-out argument

[Jakub Kicinski]

On Mon, 25 Apr 2022 10:19:45 +0300 Gal Pressman wrote:
> On 11/04/2022 22:19, Jakub Kicinski wrote:
> > Propagating EINPROGRESS thru multiple layers of functions is
> > error prone. Use darg->async as an in/out argument, like we
> > use darg->zc today. On input it tells the code if async is
> > allowed, on output if it took place.
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>  
> 
> I know this is not much to go on, but this patch broke our tls workflows
> when device offload is enabled.
> I'm still looking into it, but maybe you have an idea what might have
> went wrong?

Oof right, sorry. When packet is already decrypted by HW we'll skip 
the decrypt completely and leave async to whatever it was at input.

Something like this?

--->8---------

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index ddbe05ec5489..80094528eadb 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1562,6 +1562,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 
 	if (tlm->decrypted) {
 		darg->zc = false;
+		darg->async = false;
 		return 0;
 	}
 
@@ -1572,6 +1573,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 		if (err > 0) {
 			tlm->decrypted = 1;
 			darg->zc = false;
+			darg->async = false;
 			goto decrypt_done;
 		}
 	}

Re: [PATCH net-next 08/10] tls: rx: use async as an in-out argument

[Gal Pressman]

On 25/04/2022 17:54, Jakub Kicinski wrote:
> On Mon, 25 Apr 2022 10:19:45 +0300 Gal Pressman wrote:
>> On 11/04/2022 22:19, Jakub Kicinski wrote:
>>> Propagating EINPROGRESS thru multiple layers of functions is
>>> error prone. Use darg->async as an in/out argument, like we
>>> use darg->zc today. On input it tells the code if async is
>>> allowed, on output if it took place.
>>>
>>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>  
>> I know this is not much to go on, but this patch broke our tls workflows
>> when device offload is enabled.
>> I'm still looking into it, but maybe you have an idea what might have
>> went wrong?
> Oof right, sorry. When packet is already decrypted by HW we'll skip 
> the decrypt completely and leave async to whatever it was at input.
>
> Something like this?
>
> --->8---------
>
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index ddbe05ec5489..80094528eadb 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1562,6 +1562,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
>  
>  	if (tlm->decrypted) {
>  		darg->zc = false;
> +		darg->async = false;
>  		return 0;
>  	}
>  
> @@ -1572,6 +1573,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
>  		if (err > 0) {
>  			tlm->decrypted = 1;
>  			darg->zc = false;
> +			darg->async = false;
>  			goto decrypt_done;
>  		}
>  	}

Thank you Jakub!
I will run the patch you sent through our testing.