All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 00/10] tls: rx: random refactoring part 1
@ 2022-04-08  3:38 Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 01/10] tls: rx: jump to a more appropriate label Jakub Kicinski
                   ` (10 more replies)
  0 siblings, 11 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

TLS Rx refactoring. Part 1 of 3. A couple of features to follow.

Jakub Kicinski (10):
  tls: rx: jump to a more appropriate label
  tls: rx: drop pointless else after goto
  tls: rx: don't store the record type in socket context
  tls: rx: don't store the decryption status in socket context
  tls: rx: init decrypted status in tls_read_size()
  tls: rx: use a define for tag length
  tls: rx: replace 'back' with 'offset'
  tls: rx: don't issue wake ups when data is decrypted
  tls: rx: refactor decrypt_skb_update()
  tls: hw: rx: use return value of tls_device_decrypted() to carry
    status

 include/net/strparser.h |   4 ++
 include/net/tls.h       |  12 ++--
 net/tls/tls_device.c    |   6 +-
 net/tls/tls_sw.c        | 129 +++++++++++++++++++---------------------
 4 files changed, 70 insertions(+), 81 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 01/10] tls: rx: jump to a more appropriate label
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 02/10] tls: rx: drop pointless else after goto Jakub Kicinski
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

'recv_end:' checks num_async and decrypted, and is then followed
by the 'end' label. Since we know that decrypted and num_async
are 0 at the start we can jump to 'end'.

Move the init of decrypted and num_async to let the compiler
catch if I'm wrong.

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 0024a692f0f8..3b3fe455d680 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1760,6 +1760,7 @@ int tls_sw_recvmsg(struct sock *sk,
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
 	struct sk_psock *psock;
+	int num_async, pending;
 	unsigned char control = 0;
 	ssize_t decrypted = 0;
 	struct strp_msg *rxm;
@@ -1772,8 +1773,6 @@ int tls_sw_recvmsg(struct sock *sk,
 	bool is_kvec = iov_iter_is_kvec(&msg->msg_iter);
 	bool is_peek = flags & MSG_PEEK;
 	bool bpf_strp_enabled;
-	int num_async = 0;
-	int pending;
 
 	flags |= nonblock;
 
@@ -1795,12 +1794,14 @@ int tls_sw_recvmsg(struct sock *sk,
 	}
 
 	if (len <= copied)
-		goto recv_end;
+		goto end;
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
 	len = len - copied;
 	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
 
+	decrypted = 0;
+	num_async = 0;
 	while (len && (decrypted + copied < target || ctx->recv_pkt)) {
 		bool retain_skb = false;
 		bool zc = false;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 02/10] tls: rx: drop pointless else after goto
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 01/10] tls: rx: jump to a more appropriate label Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 03/10] tls: rx: don't store the record type in socket context Jakub Kicinski
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

Pointless else branch after goto makes the code harder to refactor
down the line.

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 3b3fe455d680..555269ad7db2 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1789,10 +1789,9 @@ int tls_sw_recvmsg(struct sock *sk,
 	if (err < 0) {
 		tls_err_abort(sk, err);
 		goto end;
-	} else {
-		copied = err;
 	}
 
+	copied = err;
 	if (len <= copied)
 		goto end;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 03/10] tls: rx: don't store the record type in socket context
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 01/10] tls: rx: jump to a more appropriate label Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 02/10] tls: rx: drop pointless else after goto Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 04/10] tls: rx: don't store the decryption status " Jakub Kicinski
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

Original TLS implementation was handling one record at a time.
It stashed the type of the record inside tls context (per socket
structure) for convenience. When async crypto support was added
[1] the author had to use skb->cb to store the type per-message.

The use of skb->cb overlaps with strparser, however, so a hybrid
approach was taken where type is stored in context while parsing
(since we parse a message at a time) but once parsed its copied
to skb->cb.

Recently a workaround for sockmaps [2] exposed the previously
private struct _strp_msg and started a trend of adding user
fields directly in strparser's header. This is cleaner than
storing information about an skb in the context.

This change is not strictly necessary, but IMHO the ownership
of the context field is confusing. Information naturally
belongs to the skb.

[1] commit 94524d8fc965 ("net/tls: Add support for async decryption of tls records")
[2] commit b2c4618162ec ("bpf, sockmap: sk_skb data_end access incorrect when src_reg = dst_reg")

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/strparser.h |  3 +++
 include/net/tls.h       | 10 +++-------
 net/tls/tls_sw.c        | 38 +++++++++++++++++---------------------
 3 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/include/net/strparser.h b/include/net/strparser.h
index 732b7097d78e..c271543076cf 100644
--- a/include/net/strparser.h
+++ b/include/net/strparser.h
@@ -70,6 +70,9 @@ struct sk_skb_cb {
 	 * when dst_reg == src_reg.
 	 */
 	u64 temp_reg;
+	struct tls_msg {
+		u8 control;
+	} tls;
 };
 
 static inline struct strp_msg *strp_msg(struct sk_buff *skb)
diff --git a/include/net/tls.h b/include/net/tls.h
index b6968a5b5538..c3717cd1f1cd 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -117,11 +117,6 @@ struct tls_rec {
 	u8 aead_req_ctx[];
 };
 
-struct tls_msg {
-	struct strp_msg rxm;
-	u8 control;
-};
-
 struct tx_work {
 	struct delayed_work work;
 	struct sock *sk;
@@ -152,7 +147,6 @@ struct tls_sw_context_rx {
 	void (*saved_data_ready)(struct sock *sk);
 
 	struct sk_buff *recv_pkt;
-	u8 control;
 	u8 async_capable:1;
 	u8 decrypted:1;
 	atomic_t decrypt_pending;
@@ -411,7 +405,9 @@ void tls_free_partial_record(struct sock *sk, struct tls_context *ctx);
 
 static inline struct tls_msg *tls_msg(struct sk_buff *skb)
 {
-	return (struct tls_msg *)strp_msg(skb);
+	struct sk_skb_cb *scb = (struct sk_skb_cb *)skb->cb;
+
+	return &scb->tls;
 }
 
 static inline bool tls_is_partially_sent_record(struct tls_context *ctx)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 555269ad7db2..222f8cad1e8c 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -128,10 +128,10 @@ static int skb_nsg(struct sk_buff *skb, int offset, int len)
         return __skb_nsg(skb, offset, len, 0);
 }
 
-static int padding_length(struct tls_sw_context_rx *ctx,
-			  struct tls_prot_info *prot, struct sk_buff *skb)
+static int padding_length(struct tls_prot_info *prot, struct sk_buff *skb)
 {
 	struct strp_msg *rxm = strp_msg(skb);
+	struct tls_msg *tlm = tls_msg(skb);
 	int sub = 0;
 
 	/* Determine zero-padding length */
@@ -153,7 +153,7 @@ static int padding_length(struct tls_sw_context_rx *ctx,
 			sub++;
 			back++;
 		}
-		ctx->control = content_type;
+		tlm->control = content_type;
 	}
 	return sub;
 }
@@ -187,7 +187,7 @@ static void tls_decrypt_done(struct crypto_async_request *req, int err)
 		struct strp_msg *rxm = strp_msg(skb);
 		int pad;
 
-		pad = padding_length(ctx, prot, skb);
+		pad = padding_length(prot, skb);
 		if (pad < 0) {
 			ctx->async_wait.err = pad;
 			tls_err_abort(skb->sk, pad);
@@ -1421,6 +1421,7 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
 	struct strp_msg *rxm = strp_msg(skb);
+	struct tls_msg *tlm = tls_msg(skb);
 	int n_sgin, n_sgout, nsg, mem_size, aead_size, err, pages = 0;
 	struct aead_request *aead_req;
 	struct sk_buff *unused;
@@ -1505,7 +1506,7 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 	/* Prepare AAD */
 	tls_make_aad(aad, rxm->full_len - prot->overhead_size +
 		     prot->tail_size,
-		     tls_ctx->rx.rec_seq, ctx->control, prot);
+		     tls_ctx->rx.rec_seq, tlm->control, prot);
 
 	/* Prepare sgin */
 	sg_init_table(sgin, n_sgin);
@@ -1590,7 +1591,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 			*zc = false;
 		}
 
-		pad = padding_length(ctx, prot, skb);
+		pad = padding_length(prot, skb);
 		if (pad < 0)
 			return pad;
 
@@ -1822,26 +1823,21 @@ int tls_sw_recvmsg(struct sock *sk,
 				}
 			}
 			goto recv_end;
-		} else {
-			tlm = tls_msg(skb);
-			if (prot->version == TLS_1_3_VERSION)
-				tlm->control = 0;
-			else
-				tlm->control = ctx->control;
 		}
 
 		rxm = strp_msg(skb);
+		tlm = tls_msg(skb);
 
 		to_decrypt = rxm->full_len - prot->overhead_size;
 
 		if (to_decrypt <= len && !is_kvec && !is_peek &&
-		    ctx->control == TLS_RECORD_TYPE_DATA &&
+		    tlm->control == TLS_RECORD_TYPE_DATA &&
 		    prot->version != TLS_1_3_VERSION &&
 		    !bpf_strp_enabled)
 			zc = true;
 
 		/* Do not use async mode if record is non-data */
-		if (ctx->control == TLS_RECORD_TYPE_DATA && !bpf_strp_enabled)
+		if (tlm->control == TLS_RECORD_TYPE_DATA && !bpf_strp_enabled)
 			async_capable = ctx->async_capable;
 		else
 			async_capable = false;
@@ -1856,8 +1852,6 @@ int tls_sw_recvmsg(struct sock *sk,
 		if (err == -EINPROGRESS) {
 			async = true;
 			num_async++;
-		} else if (prot->version == TLS_1_3_VERSION) {
-			tlm->control = ctx->control;
 		}
 
 		/* If the type of records being processed is not known yet,
@@ -2005,6 +1999,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 	struct strp_msg *rxm = NULL;
 	struct sock *sk = sock->sk;
+	struct tls_msg *tlm;
 	struct sk_buff *skb;
 	ssize_t copied = 0;
 	bool from_queue;
@@ -2033,14 +2028,15 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 		}
 	}
 
+	rxm = strp_msg(skb);
+	tlm = tls_msg(skb);
+
 	/* splice does not support reading control messages */
-	if (ctx->control != TLS_RECORD_TYPE_DATA) {
+	if (tlm->control != TLS_RECORD_TYPE_DATA) {
 		err = -EINVAL;
 		goto splice_read_end;
 	}
 
-	rxm = strp_msg(skb);
-
 	chunk = min_t(unsigned int, rxm->full_len, len);
 	copied = skb_splice_bits(skb, sk, rxm->offset, pipe, chunk, flags);
 	if (copied < 0)
@@ -2084,10 +2080,10 @@ bool tls_sw_sock_is_readable(struct sock *sk)
 static int tls_read_size(struct strparser *strp, struct sk_buff *skb)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(strp->sk);
-	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
 	char header[TLS_HEADER_SIZE + MAX_IV_SIZE];
 	struct strp_msg *rxm = strp_msg(skb);
+	struct tls_msg *tlm = tls_msg(skb);
 	size_t cipher_overhead;
 	size_t data_len = 0;
 	int ret;
@@ -2108,7 +2104,7 @@ static int tls_read_size(struct strparser *strp, struct sk_buff *skb)
 	if (ret < 0)
 		goto read_failure;
 
-	ctx->control = header[0];
+	tlm->control = header[0];
 
 	data_len = ((header[4] & 0xFF) | (header[3] << 8));
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 04/10] tls: rx: don't store the decryption status in socket context
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
                   ` (2 preceding siblings ...)
  2022-04-08  3:38 ` [PATCH net-next 03/10] tls: rx: don't store the record type in socket context Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 05/10] tls: rx: init decrypted status in tls_read_size() Jakub Kicinski
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

Similar justification to previous change, the information
about decryption status belongs in the skb.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/strparser.h |  1 +
 include/net/tls.h       |  1 -
 net/tls/tls_device.c    |  3 ++-
 net/tls/tls_sw.c        | 10 ++++++----
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/net/strparser.h b/include/net/strparser.h
index c271543076cf..a191486eb1e4 100644
--- a/include/net/strparser.h
+++ b/include/net/strparser.h
@@ -72,6 +72,7 @@ struct sk_skb_cb {
 	u64 temp_reg;
 	struct tls_msg {
 		u8 control;
+		u8 decrypted;
 	} tls;
 };
 
diff --git a/include/net/tls.h b/include/net/tls.h
index c3717cd1f1cd..f040edc97c50 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -148,7 +148,6 @@ struct tls_sw_context_rx {
 
 	struct sk_buff *recv_pkt;
 	u8 async_capable:1;
-	u8 decrypted:1;
 	atomic_t decrypt_pending;
 	/* protect crypto_wait with decrypt_pending*/
 	spinlock_t decrypt_compl_lock;
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 12f7b56771d9..78d979e0f298 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -948,6 +948,7 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx,
 			 struct sk_buff *skb, struct strp_msg *rxm)
 {
 	struct tls_offload_context_rx *ctx = tls_offload_ctx_rx(tls_ctx);
+	struct tls_msg *tlm = tls_msg(skb);
 	int is_decrypted = skb->decrypted;
 	int is_encrypted = !is_decrypted;
 	struct sk_buff *skb_iter;
@@ -962,7 +963,7 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx,
 				   tls_ctx->rx.rec_seq, rxm->full_len,
 				   is_encrypted, is_decrypted);
 
-	ctx->sw.decrypted |= is_decrypted;
+	tlm->decrypted |= is_decrypted;
 
 	if (unlikely(test_bit(TLS_RX_DEV_DEGRADED, &tls_ctx->flags))) {
 		if (likely(is_encrypted || is_decrypted))
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 222f8cad1e8c..167bd133b7f8 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1565,9 +1565,10 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
 	struct strp_msg *rxm = strp_msg(skb);
+	struct tls_msg *tlm = tls_msg(skb);
 	int pad, err = 0;
 
-	if (!ctx->decrypted) {
+	if (!tlm->decrypted) {
 		if (tls_ctx->rx_conf == TLS_HW) {
 			err = tls_device_decrypted(sk, tls_ctx, skb, rxm);
 			if (err < 0)
@@ -1575,7 +1576,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 		}
 
 		/* Still not decrypted after tls_device */
-		if (!ctx->decrypted) {
+		if (!tlm->decrypted) {
 			err = decrypt_internal(sk, skb, dest, NULL, chunk, zc,
 					       async);
 			if (err < 0) {
@@ -1599,7 +1600,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 		rxm->offset += prot->prepend_size;
 		rxm->full_len -= prot->overhead_size;
 		tls_advance_record_sn(sk, prot, &tls_ctx->rx);
-		ctx->decrypted = 1;
+		tlm->decrypted = 1;
 		ctx->saved_data_ready(sk);
 	} else {
 		*zc = false;
@@ -2144,8 +2145,9 @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(strp->sk);
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
+	struct tls_msg *tlm = tls_msg(skb);
 
-	ctx->decrypted = 0;
+	tlm->decrypted = 0;
 
 	ctx->recv_pkt = skb;
 	strp_pause(strp);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 05/10] tls: rx: init decrypted status in tls_read_size()
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
                   ` (3 preceding siblings ...)
  2022-04-08  3:38 ` [PATCH net-next 04/10] tls: rx: don't store the decryption status " Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 06/10] tls: rx: use a define for tag length Jakub Kicinski
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

We set the record type in tls_read_size(), can as well init
the tlm->decrypted field there.

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 167bd133b7f8..eb2e8495aa62 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2101,10 +2101,10 @@ static int tls_read_size(struct strparser *strp, struct sk_buff *skb)
 
 	/* Linearize header to local buffer */
 	ret = skb_copy_bits(skb, rxm->offset, header, prot->prepend_size);
-
 	if (ret < 0)
 		goto read_failure;
 
+	tlm->decrypted = 0;
 	tlm->control = header[0];
 
 	data_len = ((header[4] & 0xFF) | (header[3] << 8));
@@ -2145,9 +2145,6 @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(strp->sk);
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
-	struct tls_msg *tlm = tls_msg(skb);
-
-	tlm->decrypted = 0;
 
 	ctx->recv_pkt = skb;
 	strp_pause(strp);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 06/10] tls: rx: use a define for tag length
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
                   ` (4 preceding siblings ...)
  2022-04-08  3:38 ` [PATCH net-next 05/10] tls: rx: init decrypted status in tls_read_size() Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 07/10] tls: rx: replace 'back' with 'offset' Jakub Kicinski
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

TLS 1.3 has to strip padding, and it starts out 16 bytes
from the end of the record. Make it clear this is because
of the auth tag.

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

diff --git a/include/net/tls.h b/include/net/tls.h
index f040edc97c50..a01c264e5f15 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -64,6 +64,7 @@
 #define TLS_AAD_SPACE_SIZE		13
 
 #define MAX_IV_SIZE			16
+#define TLS_TAG_SIZE			16
 #define TLS_MAX_REC_SEQ_SIZE		8
 
 /* For CCM mode, the full 16-bytes of IV is made of '4' fields of given sizes.
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index eb2e8495aa62..579ccfd011a1 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -136,9 +136,9 @@ static int padding_length(struct tls_prot_info *prot, struct sk_buff *skb)
 
 	/* Determine zero-padding length */
 	if (prot->version == TLS_1_3_VERSION) {
+		int back = TLS_TAG_SIZE + 1;
 		char content_type = 0;
 		int err;
-		int back = 17;
 
 		while (content_type == 0) {
 			if (back > rxm->full_len - prot->prepend_size)
@@ -2496,7 +2496,7 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 
 	/* Sanity-check the sizes for stack allocations. */
 	if (iv_size > MAX_IV_SIZE || nonce_size > MAX_IV_SIZE ||
-	    rec_seq_size > TLS_MAX_REC_SEQ_SIZE) {
+	    rec_seq_size > TLS_MAX_REC_SEQ_SIZE || tag_size != TLS_TAG_SIZE) {
 		rc = -EINVAL;
 		goto free_priv;
 	}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 07/10] tls: rx: replace 'back' with 'offset'
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
                   ` (5 preceding siblings ...)
  2022-04-08  3:38 ` [PATCH net-next 06/10] tls: rx: use a define for tag length Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 08/10] tls: rx: don't issue wake ups when data is decrypted Jakub Kicinski
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

The padding length TLS 1.3 logic is searching for content_type from
the end of text. IMHO the code is easier to parse if we calculate
offset and decrement it rather than try to maintain positive offset
from the end of the record called "back".

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 579ccfd011a1..9729244ce3fc 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -136,22 +136,21 @@ static int padding_length(struct tls_prot_info *prot, struct sk_buff *skb)
 
 	/* Determine zero-padding length */
 	if (prot->version == TLS_1_3_VERSION) {
-		int back = TLS_TAG_SIZE + 1;
+		int offset = rxm->full_len - TLS_TAG_SIZE - 1;
 		char content_type = 0;
 		int err;
 
 		while (content_type == 0) {
-			if (back > rxm->full_len - prot->prepend_size)
+			if (offset < prot->prepend_size)
 				return -EBADMSG;
-			err = skb_copy_bits(skb,
-					    rxm->offset + rxm->full_len - back,
+			err = skb_copy_bits(skb, rxm->offset + offset,
 					    &content_type, 1);
 			if (err)
 				return err;
 			if (content_type)
 				break;
 			sub++;
-			back++;
+			offset--;
 		}
 		tlm->control = content_type;
 	}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 08/10] tls: rx: don't issue wake ups when data is decrypted
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
                   ` (6 preceding siblings ...)
  2022-04-08  3:38 ` [PATCH net-next 07/10] tls: rx: replace 'back' with 'offset' Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 09/10] tls: rx: refactor decrypt_skb_update() Jakub Kicinski
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

We inform the applications that data is available when
the record is received. Decryption happens inline inside
recvmsg or splice call. Generating another wakeup inside
the decryption handler seems pointless as someone must
be actively reading the socket if we are executing this
code.

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 9729244ce3fc..1f6bb1f67f41 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1561,7 +1561,6 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 			      bool async)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
-	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
 	struct strp_msg *rxm = strp_msg(skb);
 	struct tls_msg *tlm = tls_msg(skb);
@@ -1600,7 +1599,6 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 		rxm->full_len -= prot->overhead_size;
 		tls_advance_record_sn(sk, prot, &tls_ctx->rx);
 		tlm->decrypted = 1;
-		ctx->saved_data_ready(sk);
 	} else {
 		*zc = false;
 	}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 09/10] tls: rx: refactor decrypt_skb_update()
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
                   ` (7 preceding siblings ...)
  2022-04-08  3:38 ` [PATCH net-next 08/10] tls: rx: don't issue wake ups when data is decrypted Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08  3:38 ` [PATCH net-next 10/10] tls: hw: rx: use return value of tls_device_decrypted() to carry status Jakub Kicinski
  2022-04-08 11:10 ` [PATCH net-next 00/10] tls: rx: random refactoring part 1 patchwork-bot+netdevbpf
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

Use early return and a jump label to remove two indentation levels.
No functional changes.

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 1f6bb1f67f41..d6ac9deede7b 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1564,46 +1564,46 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
 	struct strp_msg *rxm = strp_msg(skb);
 	struct tls_msg *tlm = tls_msg(skb);
-	int pad, err = 0;
+	int pad, err;
 
-	if (!tlm->decrypted) {
-		if (tls_ctx->rx_conf == TLS_HW) {
-			err = tls_device_decrypted(sk, tls_ctx, skb, rxm);
-			if (err < 0)
-				return err;
-		}
+	if (tlm->decrypted) {
+		*zc = false;
+		return 0;
+	}
 
-		/* Still not decrypted after tls_device */
-		if (!tlm->decrypted) {
-			err = decrypt_internal(sk, skb, dest, NULL, chunk, zc,
-					       async);
-			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;
-			}
-		} else {
+	if (tls_ctx->rx_conf == TLS_HW) {
+		err = tls_device_decrypted(sk, tls_ctx, skb, rxm);
+		if (err < 0)
+			return err;
+
+		/* skip SW decryption if NIC handled it already */
+		if (tlm->decrypted) {
 			*zc = false;
+			goto decrypt_done;
 		}
+	}
 
-		pad = padding_length(prot, skb);
-		if (pad < 0)
-			return pad;
-
-		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;
-	} else {
-		*zc = false;
+	err = decrypt_internal(sk, skb, dest, NULL, chunk, zc, async);
+	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;
 	}
 
-	return err;
+decrypt_done:
+	pad = padding_length(prot, skb);
+	if (pad < 0)
+		return pad;
+
+	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;
+
+	return 0;
 }
 
 int decrypt_skb(struct sock *sk, struct sk_buff *skb,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 10/10] tls: hw: rx: use return value of tls_device_decrypted() to carry status
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
                   ` (8 preceding siblings ...)
  2022-04-08  3:38 ` [PATCH net-next 09/10] tls: rx: refactor decrypt_skb_update() Jakub Kicinski
@ 2022-04-08  3:38 ` Jakub Kicinski
  2022-04-08 11:10 ` [PATCH net-next 00/10] tls: rx: random refactoring part 1 patchwork-bot+netdevbpf
  10 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-04-08  3:38 UTC (permalink / raw)
  To: davem, pabeni
  Cc: netdev, borisp, john.fastabend, daniel, vfedorenko, Jakub Kicinski

Instead of tls_device poking into internals of the message
return 1 from tls_device_decrypted() if the device handled
the decryption.

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

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 78d979e0f298..5e4bd8ba48d3 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -948,7 +948,6 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx,
 			 struct sk_buff *skb, struct strp_msg *rxm)
 {
 	struct tls_offload_context_rx *ctx = tls_offload_ctx_rx(tls_ctx);
-	struct tls_msg *tlm = tls_msg(skb);
 	int is_decrypted = skb->decrypted;
 	int is_encrypted = !is_decrypted;
 	struct sk_buff *skb_iter;
@@ -963,11 +962,9 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx,
 				   tls_ctx->rx.rec_seq, rxm->full_len,
 				   is_encrypted, is_decrypted);
 
-	tlm->decrypted |= is_decrypted;
-
 	if (unlikely(test_bit(TLS_RX_DEV_DEGRADED, &tls_ctx->flags))) {
 		if (likely(is_encrypted || is_decrypted))
-			return 0;
+			return is_decrypted;
 
 		/* After tls_device_down disables the offload, the next SKB will
 		 * likely have initial fragments decrypted, and final ones not
@@ -982,7 +979,7 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx,
 	 */
 	if (is_decrypted) {
 		ctx->resync_nh_reset = 1;
-		return 0;
+		return is_decrypted;
 	}
 	if (is_encrypted) {
 		tls_device_core_ctrl_rx_resync(tls_ctx, ctx, sk, skb);
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index d6ac9deede7b..990450ff9537 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1575,9 +1575,8 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 		err = tls_device_decrypted(sk, tls_ctx, skb, rxm);
 		if (err < 0)
 			return err;
-
-		/* skip SW decryption if NIC handled it already */
-		if (tlm->decrypted) {
+		if (err > 0) {
+			tlm->decrypted = 1;
 			*zc = false;
 			goto decrypt_done;
 		}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 00/10] tls: rx: random refactoring part 1
  2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
                   ` (9 preceding siblings ...)
  2022-04-08  3:38 ` [PATCH net-next 10/10] tls: hw: rx: use return value of tls_device_decrypted() to carry status Jakub Kicinski
@ 2022-04-08 11:10 ` patchwork-bot+netdevbpf
  10 siblings, 0 replies; 12+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-04-08 11:10 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, pabeni, netdev, borisp, john.fastabend, daniel, vfedorenko

Hello:

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

On Thu,  7 Apr 2022 20:38:13 -0700 you wrote:
> TLS Rx refactoring. Part 1 of 3. A couple of features to follow.
> 
> Jakub Kicinski (10):
>   tls: rx: jump to a more appropriate label
>   tls: rx: drop pointless else after goto
>   tls: rx: don't store the record type in socket context
>   tls: rx: don't store the decryption status in socket context
>   tls: rx: init decrypted status in tls_read_size()
>   tls: rx: use a define for tag length
>   tls: rx: replace 'back' with 'offset'
>   tls: rx: don't issue wake ups when data is decrypted
>   tls: rx: refactor decrypt_skb_update()
>   tls: hw: rx: use return value of tls_device_decrypted() to carry
>     status
> 
> [...]

Here is the summary with links:
  - [net-next,01/10] tls: rx: jump to a more appropriate label
    https://git.kernel.org/netdev/net-next/c/bfc06e1aaa13
  - [net-next,02/10] tls: rx: drop pointless else after goto
    https://git.kernel.org/netdev/net-next/c/d5123edd10cf
  - [net-next,03/10] tls: rx: don't store the record type in socket context
    https://git.kernel.org/netdev/net-next/c/c3f6bb74137c
  - [net-next,04/10] tls: rx: don't store the decryption status in socket context
    https://git.kernel.org/netdev/net-next/c/7dc59c33d62c
  - [net-next,05/10] tls: rx: init decrypted status in tls_read_size()
    https://git.kernel.org/netdev/net-next/c/863533e316b2
  - [net-next,06/10] tls: rx: use a define for tag length
    https://git.kernel.org/netdev/net-next/c/a8340cc02bee
  - [net-next,07/10] tls: rx: replace 'back' with 'offset'
    https://git.kernel.org/netdev/net-next/c/5deee41b19b3
  - [net-next,08/10] tls: rx: don't issue wake ups when data is decrypted
    https://git.kernel.org/netdev/net-next/c/5dbda02d322d
  - [net-next,09/10] tls: rx: refactor decrypt_skb_update()
    https://git.kernel.org/netdev/net-next/c/3764ae5ba661
  - [net-next,10/10] tls: hw: rx: use return value of tls_device_decrypted() to carry status
    https://git.kernel.org/netdev/net-next/c/71471ca32505

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



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-04-08 11:10 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-08  3:38 [PATCH net-next 00/10] tls: rx: random refactoring part 1 Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 01/10] tls: rx: jump to a more appropriate label Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 02/10] tls: rx: drop pointless else after goto Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 03/10] tls: rx: don't store the record type in socket context Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 04/10] tls: rx: don't store the decryption status " Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 05/10] tls: rx: init decrypted status in tls_read_size() Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 06/10] tls: rx: use a define for tag length Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 07/10] tls: rx: replace 'back' with 'offset' Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 08/10] tls: rx: don't issue wake ups when data is decrypted Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 09/10] tls: rx: refactor decrypt_skb_update() Jakub Kicinski
2022-04-08  3:38 ` [PATCH net-next 10/10] tls: hw: rx: use return value of tls_device_decrypted() to carry status Jakub Kicinski
2022-04-08 11:10 ` [PATCH net-next 00/10] tls: rx: random refactoring part 1 patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.