Skip to contentSkip to conversation

tls: receive-path fixes and clean-ups

PATCH Superseded Chuck Lever 3 participants 15 messages May 26 – May 28

Subsystems (4)
Networking Network drivers Networking TLS Handshake upcall for transport layer security

v11 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Chuck Lever

author maintainer
#0 lore raw

v11 cover tls: receive-path fixes and clean-ups

I'd like to encourage in-kernel kTLS consumers (NFSD, NVMe/TCP) to coalesce on the use of read_sock. While auditing read_sock for that purpose, Hannes and Sabrina flagged a few rough edges in the receive paths.

This series is a set of clean-ups, not a performance series. Async batch decryption and its submit/deliver scaffolding were dropped during previous review: async_capable is always false for TLS 1.3, the version NFSD and NVMe/TCP both require, so async-related improvements were unreachable for the in-kernel consumers this work targets.

A subsequent series will introduce infrastructure to support KeyUpdate for in-kernel kTLS consumers, which need to handle TLS Alert messages that trigger a tlshd upcall.

---
Changes since v11:
- 1/7 has been merged into net/main
- Rebased on net-next main (Fri May 22 18:40:51 2026 -0700)

Changes since v10:
- Move fixes to front of series
- Simplify loop termination in tls_sw_read_sock() (Sabrina)

Changes since v9:
- Recast cover letter: this is a clean-up series, not a performance series (Jakub, Sabrina)
- Rephrase subject to describe the refactor (Jakub)
- Split 2/5 into two patches separating the do/while loop-structure cleanup from the partial-consume fix (Sabrina)
- Continue the loop after a partial consume to match __tcp_read_sock() semantics, instead of exiting (Jakub)
- Drop kdoc on the internal function and rename tls_strp_msg_release() to tls_strp_msg_consume() (Jakub)
- Drop kdoc on tls_strp_check_rcv() and un-wrap the "Defer notification" comment (Jakub)
- Rename tls_strp_check_rcv() parameter wake to announce, and tls_rx_msg_ready() to tls_rx_msg_maybe_announce() (Jakub)
- Drop tls_rx_handoff(); fold the per-record path back into tls_rx_rec_done() and fire the deferred announce from tls_rx_reader_release() (Jakub)
- New patch: Preserve sk_err across recvmsg() when data has been copied, so a connection abort during sk_flush_backlog() surfaces on the next read instead of vanishing when the caller returns the bytes already accumulated

Changes since v8:
- Address review comments from sashiko
- Patch 2: Requeue partially consumed skb to prevent leak
- Patch 5: Re-check sk_err so RST during flush surfaces as -ECONNRESET instead of EOF
- Address review comments from gpt-5.5
- Patch 4: Restore msg_ready early-return in tls_strp_check_rcv() so the queued strp_work doesn't double-wake the consumer
- Patch 4: Add tls_strparser msg_announced bit so the recvmsg exit-point handoff doesn't re-fire saved_data_ready() for a record BH or the worker already announced (rx_list-only drain path)

Changes since v7:
- Rebased on net-next (v7.1-rc1)

Changes since v6:
- Rebased on net-next, v5's 1/6 was merged upstream

Changes since v5:
- Patch 6: Set released = true when sk_flush_backlog() returns true, so tls_strp_msg_load() knows the socket lock was released (Sabrina)
- Patch 6: Drop Fixes tag; submit bug fix separately via net if warranted (Sabrina)
- Patch 6: Note redundant flush on cold path in commit message (Sabrina)

Changes since v4:
- Drop batch async decryption and submit/deliver restructure:
async_capable is always false for TLS 1.3, so the new code was unreachable for NFS and NVMe/TCP
- Purge async_hold directly in tls_decrypt_async_wait() and drop the tls_decrypt_async_drain() wrapper
- Merge tls_strp_check_rcv_quiet() into tls_strp_check_rcv() with a bool wake parameter; fix lost wakeup on the recvmsg exit path

Changes since v3:
- Clarify why tls_decrypt_async_drain() is separate from _wait()
- Fold tls_err_abort() into tls_rx_one_record(), drop tls_rx_decrypt_record()
- Move backlog flush into tls_rx_rec_wait() so all RX paths benefit

Changes since v2:
- Fix short read self tests

Changes since v1:
- Add C11 reference
- Extend data_ready reduction to recvmsg and splice
- Restructure read_sock and recvmsg using shared helpers

---
Changes in v11:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v10: https://patch.msgid.link/[email protected]

---
Chuck Lever (6):
tls: Avoid evaluating freed skb in tls_sw_read_sock() loop
tls: Re-present partially-consumed records in tls_sw_read_sock()
tls: Move decrypt-failure abort into tls_rx_one_record()
tls: Factor tls_strp_msg_consume() from tls_strp_msg_done()
tls: Suppress spurious saved_data_ready on all receive paths
tls: Flush backlog before waiting for a new record

 include/net/tls.h  |  5 +++++
 net/tls/tls.h      |  6 ++---
 net/tls/tls_main.c |  2 +-
 net/tls/tls_strp.c | 26 +++++++++++++++-------
 net/tls/tls_sw.c   | 64 ++++++++++++++++++++++++++++++++++++++----------------
 5 files changed, 72 insertions(+), 31 deletions(-)

---
base-commit: c0aa5f13826dcb035bec3d6b252e6b2020fa5f88
change-id: 20260317-tls-read-sock-a0022c9df265

Best regards,

Signature · 1 line
Chuck Lever <[email protected]>
author maintainer
#1 lore raw

PATCH v11 1/6 tls: Avoid evaluating freed skb in tls_sw_read_sock() loop

From: Chuck Lever <[email protected]>

tls_sw_read_sock() ends its receive loop with while (skb), but the else branch in the body calls consume_skb(skb) before the predicate is re-evaluated. A pointer becomes indeterminate when the object it points to reaches end-of-lifetime (C2011 6.2.4p2), and using an indeterminate value is undefined behavior (Annex J.2). The pointer is not dereferenced today -- the predicate either exits the loop or skb is overwritten at the top of the next iteration -- but any future change that adds a dereference between consume_skb() and the predicate would silently introduce a use-after-free.

Replace the do/while form with an explicit for(;;) loop so termination happens through a break statement rather than predicate evaluation of a freed pointer.

Cc Sagi Grimberg <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 net/tls/tls_sw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
net/tls/tls_sw.c +3 -3
index 964ebc268ee4..8e4e57721335 100644
@@ -2383,7 +2383,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 		goto read_sock_end;
 
 	decrypted = 0;
-	do {
+	for (;;) {
 		if (!skb_queue_empty(&ctx->rx_list)) {
 			skb = __skb_dequeue(&ctx->rx_list);
 			rxm = strp_msg(skb);

@@ -2435,9 +2435,9 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 		} else {
 			consume_skb(skb);
 			if (!desc->count)
-				skb = NULL;
+				break;
 		}
-	} while (skb);
+	}
 
 read_sock_end:
 	tls_rx_reader_release(sk, ctx);
Signature · 1 line
2.54.0
Reviewed-by on v11 1/6
#2 lore raw

Re: [PATCH net-next v11 1/6] tls: Avoid evaluating freed skb in tls_sw_read_sock() loop

Quoted 17 lines · Chuck Lever

From: Chuck Lever <[email protected]>

tls_sw_read_sock() ends its receive loop with while (skb), but the else branch in the body calls consume_skb(skb) before the predicate is re-evaluated. A pointer becomes indeterminate when the object it points to reaches end-of-lifetime (C2011 6.2.4p2), and using an indeterminate value is undefined behavior (Annex J.2). The pointer is not dereferenced today -- the predicate either exits the loop or skb is overwritten at the top of the next iteration -- but any future change that adds a dereference between consume_skb() and the predicate would silently introduce a use-after-free.

Replace the do/while form with an explicit for(;;) loop so termination happens through a break statement rather than predicate evaluation of a freed pointer.

Quoted 6 lines
Cc Sagi Grimberg <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
  net/tls/tls_sw.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
maintainer Reviewed-by on v11 1/6
#3 lore raw

Re: [PATCH net-next v11 1/6] tls: Avoid evaluating freed skb in tls_sw_read_sock() loop

In reply to Chuck Lever
Quoted 17 lines · Chuck Lever

From: Chuck Lever <[email protected]>

tls_sw_read_sock() ends its receive loop with while (skb), but the else branch in the body calls consume_skb(skb) before the predicate is re-evaluated. A pointer becomes indeterminate when the object it points to reaches end-of-lifetime (C2011 6.2.4p2), and using an indeterminate value is undefined behavior (Annex J.2). The pointer is not dereferenced today -- the predicate either exits the loop or skb is overwritten at the top of the next iteration -- but any future change that adds a dereference between consume_skb() and the predicate would silently introduce a use-after-free.

Replace the do/while form with an explicit for(;;) loop so termination happens through a break statement rather than predicate evaluation of a freed pointer.

Quoted 5 lines
Cc Sagi Grimberg <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 net/tls/tls_sw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by Sabrina Dubroca <[email protected]>
Signature · 1 line
Sabrina
author maintainer
#4 lore raw

PATCH v11 2/6 tls: Re-present partially-consumed records in tls_sw_read_sock()

In reply to Chuck Lever

From: Chuck Lever <[email protected]>

The tls_sw_read_sock() loop releases the current skb whether read_actor() consumed the full record or only a prefix. When the actor takes only part of the record and leaves desc->count non-zero, the remainder is lost: skb is neither requeued nor freed, and the next iteration overwrites it during dequeue or tls_rx_rec_wait().

No mainline consumer reaches this path today. The only in-tree TLS read_sock user is nvme/tcp, whose actor nvme_tcp_recv_skb() loops until the input length is exhausted and returns either the full length or a negative error.

The path becomes reachable with the upcoming NFSD svcsock receive built on read_sock_cmsg. Its data actor, svc_tcp_recv_actor(), parses an RPC fragment stream incrementally and returns at fragment boundaries. When a TLS record carries the tail of one RPC fragment plus the head of the next, the actor returns fewer bytes than offered while leaving desc->count non-zero, and without re-presentation the trailing fragment header vanishes.

__tcp_read_sock() handles the equivalent case for plain TCP by leaving the unread bytes available for the next iteration to re-present, via sequence-number re-lookup. Adopt the same loop-level behavior: when read_actor() consumes only part of the record, update rxm->offset and rxm->full_len and requeue the skb to the head of rx_list so the next iteration re-presents the unread bytes. Switch the open-ended for-loop to "while (desc->count)" so the partial- and full-consume arms share a single exit check and read_actor() is not re-invoked once desc->count is exhausted.

Cc Sagi Grimberg <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 net/tls/tls_sw.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
net/tls/tls_sw.c +2 -5
index 8e4e57721335..fd8d3c979368 100644
@@ -2383,7 +2383,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 		goto read_sock_end;
 
 	decrypted = 0;
-	for (;;) {
+	while (desc->count) {
 		if (!skb_queue_empty(&ctx->rx_list)) {
 			skb = __skb_dequeue(&ctx->rx_list);
 			rxm = strp_msg(skb);

@@ -2430,12 +2430,9 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 		if (used < rxm->full_len) {
 			rxm->offset += used;
 			rxm->full_len -= used;
-			if (!desc->count)
-				goto read_sock_requeue;
+			__skb_queue_head(&ctx->rx_list, skb);
 		} else {
 			consume_skb(skb);
-			if (!desc->count)
-				break;
 		}
 	}
Signature · 1 line
2.54.0
Reviewed-by on v11 2/6
#5 lore raw

Re: [PATCH net-next v11 2/6] tls: Re-present partially-consumed records in tls_sw_read_sock()

Quoted 34 lines · Chuck Lever

From: Chuck Lever <[email protected]>

The tls_sw_read_sock() loop releases the current skb whether read_actor() consumed the full record or only a prefix. When the actor takes only part of the record and leaves desc->count non-zero, the remainder is lost: skb is neither requeued nor freed, and the next iteration overwrites it during dequeue or tls_rx_rec_wait().

No mainline consumer reaches this path today. The only in-tree TLS read_sock user is nvme/tcp, whose actor nvme_tcp_recv_skb() loops until the input length is exhausted and returns either the full length or a negative error.

The path becomes reachable with the upcoming NFSD svcsock receive built on read_sock_cmsg. Its data actor, svc_tcp_recv_actor(), parses an RPC fragment stream incrementally and returns at fragment boundaries. When a TLS record carries the tail of one RPC fragment plus the head of the next, the actor returns fewer bytes than offered while leaving desc->count non-zero, and without re-presentation the trailing fragment header vanishes.

__tcp_read_sock() handles the equivalent case for plain TCP by leaving the unread bytes available for the next iteration to re-present, via sequence-number re-lookup. Adopt the same loop-level behavior: when read_actor() consumes only part of the record, update rxm->offset and rxm->full_len and requeue the skb to the head of rx_list so the next iteration re-presents the unread bytes. Switch the open-ended for-loop to "while (desc->count)" so the partial- and full-consume arms share a single exit check and read_actor() is not re-invoked once desc->count is exhausted.

Quoted 6 lines
Cc Sagi Grimberg <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
  net/tls/tls_sw.c | 7 ++-----
  1 file changed, 2 insertions(+), 5 deletions(-)
Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
maintainer Reviewed-by on v11 2/6
#6 lore raw

Re: [PATCH net-next v11 2/6] tls: Re-present partially-consumed records in tls_sw_read_sock()

In reply to Chuck Lever
Quoted 34 lines · Chuck Lever

From: Chuck Lever <[email protected]>

The tls_sw_read_sock() loop releases the current skb whether read_actor() consumed the full record or only a prefix. When the actor takes only part of the record and leaves desc->count non-zero, the remainder is lost: skb is neither requeued nor freed, and the next iteration overwrites it during dequeue or tls_rx_rec_wait().

No mainline consumer reaches this path today. The only in-tree TLS read_sock user is nvme/tcp, whose actor nvme_tcp_recv_skb() loops until the input length is exhausted and returns either the full length or a negative error.

The path becomes reachable with the upcoming NFSD svcsock receive built on read_sock_cmsg. Its data actor, svc_tcp_recv_actor(), parses an RPC fragment stream incrementally and returns at fragment boundaries. When a TLS record carries the tail of one RPC fragment plus the head of the next, the actor returns fewer bytes than offered while leaving desc->count non-zero, and without re-presentation the trailing fragment header vanishes.

__tcp_read_sock() handles the equivalent case for plain TCP by leaving the unread bytes available for the next iteration to re-present, via sequence-number re-lookup. Adopt the same loop-level behavior: when read_actor() consumes only part of the record, update rxm->offset and rxm->full_len and requeue the skb to the head of rx_list so the next iteration re-presents the unread bytes. Switch the open-ended for-loop to "while (desc->count)" so the partial- and full-consume arms share a single exit check and read_actor() is not re-invoked once desc->count is exhausted.

Quoted 5 lines
Cc Sagi Grimberg <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 net/tls/tls_sw.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Reviewed-by Sabrina Dubroca <[email protected]>
Signature · 1 line
Sabrina
author maintainer
#7 lore raw

PATCH v11 3/6 tls: Move decrypt-failure abort into tls_rx_one_record()

In reply to Chuck Lever

From: Chuck Lever <[email protected]>

Three receive paths -- recvmsg, read_sock, and splice_read -- each follow tls_rx_one_record() with the same tls_err_abort() call. Consolidate the abort into tls_rx_one_record() so the decrypt-and-abort sequence lives in one place.

A tls_check_pending_rekey() failure after successful decryption no longer triggers tls_err_abort(). That path fires only when skb_copy_bits() fails on a valid skb, which is not a realistic scenario.

Suggested-by Sabrina Dubroca <[email protected]>
Reviewed-by Hannes Reinecke <[email protected]>
Reviewed-by Sabrina Dubroca <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 net/tls/tls_sw.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)
net/tls/tls_sw.c +9 -10
index fd8d3c979368..798f2535ddf7 100644
@@ -1827,6 +1827,9 @@ static int tls_check_pending_rekey(struct sock *sk, struct tls_context *ctx,
 	return 0;
 }
 
+/* On decrypt failure the connection is aborted (sk_err set) before
+ * returning a negative errno.
+ */
 static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
 			     struct tls_decrypt_arg *darg)
 {

@@ -1838,8 +1841,10 @@ static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
 	err = tls_decrypt_device(sk, msg, tls_ctx, darg);
 	if (!err)
 		err = tls_decrypt_sw(sk, tls_ctx, msg, darg);
-	if (err < 0)
+	if (err < 0) {
+		tls_err_abort(sk, -EBADMSG);
 		return err;
+	}
 
 	rxm = strp_msg(darg->skb);
 	rxm->offset += prot->prepend_size;

@@ -2150,10 +2155,8 @@ int tls_sw_recvmsg(struct sock *sk,
 			darg.async = false;
 
 		err = tls_rx_one_record(sk, msg, &darg);
-		if (err < 0) {
-			tls_err_abort(sk, -EBADMSG);
+		if (err < 0)
 			goto recv_end;
-		}
 
 		async |= darg.async;
 

@@ -2312,10 +2315,8 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 		memset(&darg.inargs, 0, sizeof(darg.inargs));
 
 		err = tls_rx_one_record(sk, NULL, &darg);
-		if (err < 0) {
-			tls_err_abort(sk, -EBADMSG);
+		if (err < 0)
 			goto splice_read_end;
-		}
 
 		tls_rx_rec_done(ctx);
 		skb = darg.skb;

@@ -2398,10 +2399,8 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 			memset(&darg.inargs, 0, sizeof(darg.inargs));
 
 			err = tls_rx_one_record(sk, NULL, &darg);
-			if (err < 0) {
-				tls_err_abort(sk, -EBADMSG);
+			if (err < 0)
 				goto read_sock_end;
-			}
 
 			released = tls_read_flush_backlog(sk, prot, INT_MAX,
 							  0, decrypted,
Signature · 1 line
2.54.0
author maintainer
#8 lore raw

PATCH v11 4/6 tls: Factor tls_strp_msg_consume() from tls_strp_msg_done()

In reply to Chuck Lever

From: Chuck Lever <[email protected]>

tls_strp_msg_done() conflates releasing the current record with checking for the next one via tls_strp_check_rcv(). A subsequent patch needs to release a record without immediately triggering that check, so the release step is separated into tls_strp_msg_consume(). tls_strp_msg_done() is preserved as a wrapper for existing callers.

Reviewed-by Hannes Reinecke <[email protected]>
Reviewed-by Alistair Francis <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 net/tls/tls.h      |  1 +
 net/tls/tls_strp.c | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)
net/tls/tls.h +1 -0
index 12f44cb649c9..cb0091e03f41 100644
@@ -194,6 +194,7 @@ int tls_strp_init(struct tls_strparser *strp, struct sock *sk);
 void tls_strp_data_ready(struct tls_strparser *strp);
 
 void tls_strp_check_rcv(struct tls_strparser *strp);
+void tls_strp_msg_consume(struct tls_strparser *strp);
 void tls_strp_msg_done(struct tls_strparser *strp);
 
 int tls_rx_msg_size(struct tls_strparser *strp, struct sk_buff *skb);
net/tls/tls_strp.c +10 -1
index c72e88317627..e7aaee6efe6e 100644
@@ -581,7 +581,12 @@ static void tls_strp_work(struct work_struct *w)
 	release_sock(strp->sk);
 }
 
-void tls_strp_msg_done(struct tls_strparser *strp)
+/* Release the current record without triggering a check for the
+ * next record. Callers must invoke tls_strp_check_rcv() before
+ * releasing the socket lock, or queued data will stall until the
+ * next tls_strp_data_ready() event.
+ */
+void tls_strp_msg_consume(struct tls_strparser *strp)
 {
 	WARN_ON(!strp->stm.full_len);
 

@@ -592,7 +597,11 @@ void tls_strp_msg_done(struct tls_strparser *strp)
 
 	WRITE_ONCE(strp->msg_ready, 0);
 	memset(&strp->stm, 0, sizeof(strp->stm));
+}
 
+void tls_strp_msg_done(struct tls_strparser *strp)
+{
+	tls_strp_msg_consume(strp);
 	tls_strp_check_rcv(strp);
 }
Signature · 1 line
2.54.0
maintainer Reviewed-by on v11 4/6
#9 lore raw

Re: [PATCH net-next v11 4/6] tls: Factor tls_strp_msg_consume() from tls_strp_msg_done()

Quoted 9 lines · Chuck Lever

From: Chuck Lever <[email protected]>

tls_strp_msg_done() conflates releasing the current record with checking for the next one via tls_strp_check_rcv(). A subsequent patch needs to release a record without immediately triggering that check, so the release step is separated into tls_strp_msg_consume(). tls_strp_msg_done() is preserved as a wrapper for existing callers.

Quoted 7 lines
Reviewed-by Hannes Reinecke <[email protected]>
Reviewed-by Alistair Francis <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 net/tls/tls.h      |  1 +
 net/tls/tls_strp.c | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)
Reviewed-by Sabrina Dubroca <[email protected]>
Signature · 1 line
Sabrina
author maintainer
#10 lore raw

PATCH v11 5/6 tls: Suppress spurious saved_data_ready on all receive paths

In reply to Chuck Lever

From: Chuck Lever <[email protected]>

Each record release via tls_strp_msg_done() triggered tls_strp_check_rcv(), which called tls_rx_msg_ready() and fired saved_data_ready(). During a multi-record receive, the first N-1 wakeups are pure overhead: the caller is already running and will pick up subsequent records on the next loop iteration. The recvmsg and splice_read paths share this waste.

Suppress per-record notifications and emit a single one on reader exit. tls_rx_rec_done() releases the current record and parses the next without announcing; tls_strp_check_rcv() gains a bool announce parameter so callers can request the quiet form. tls_rx_reader_release() fires the deferred announce on exit through tls_rx_msg_maybe_announce(), an idempotent helper that calls saved_data_ready() only when a record is parsed and has not yet been announced.

To keep the final notification idempotent against records that the BH or the worker has already announced, tls_strparser gains a msg_announced bit. tls_rx_msg_maybe_announce() sets the bit when firing saved_data_ready(); the bit is cleared whenever the parsed record is wiped, by tls_strp_msg_consume() on consumption or by tls_strp_msg_load() when the lower socket loses bytes from under the parse. A second call for the same parsed record -- as when recvmsg() satisfies the request from ctx->rx_list without touching the strparser -- becomes a no-op.

With no remaining callers, tls_strp_msg_done() is removed.

Signed-off-by Chuck Lever <[email protected]>
---
 include/net/tls.h  |  5 +++++
 net/tls/tls.h      |  5 ++---
 net/tls/tls_main.c |  2 +-
 net/tls/tls_strp.c | 23 ++++++++++++-----------
 net/tls/tls_sw.c   | 27 ++++++++++++++++++++++++---
 5 files changed, 44 insertions(+), 18 deletions(-)
include/net/tls.h +5 -0
index ebd2550280ae..3811943288b3 100644
@@ -111,11 +111,16 @@ struct tls_sw_context_tx {
 struct tls_strparser {
 	struct sock *sk;
 
+	/* Bitfield word and msg_ready are serialized by the lower
+	 * socket lock; BH and worker contexts both acquire it.
+	 */
 	u32 mark : 8;
 	u32 stopped : 1;
 	u32 copy_mode : 1;
 	u32 mixed_decrypted : 1;
 
+	u32 msg_announced : 1;
+
 	bool msg_ready;
 
 	struct strp_msg stm;
net/tls/tls.h +2 -3
index cb0091e03f41..60a37bdaaa25 100644
@@ -193,12 +193,11 @@ void tls_strp_stop(struct tls_strparser *strp);
 int tls_strp_init(struct tls_strparser *strp, struct sock *sk);
 void tls_strp_data_ready(struct tls_strparser *strp);
 
-void tls_strp_check_rcv(struct tls_strparser *strp);
+void tls_strp_check_rcv(struct tls_strparser *strp, bool announce);
 void tls_strp_msg_consume(struct tls_strparser *strp);
-void tls_strp_msg_done(struct tls_strparser *strp);
 
 int tls_rx_msg_size(struct tls_strparser *strp, struct sk_buff *skb);
-void tls_rx_msg_ready(struct tls_strparser *strp);
+void tls_rx_msg_maybe_announce(struct tls_strparser *strp);
 
 bool tls_strp_msg_load(struct tls_strparser *strp, bool force_refresh);
 int tls_strp_msg_cow(struct tls_sw_context_rx *ctx);
net/tls/tls_main.c +1 -1
index fd39acf41a61..c10a3fd7fc17 100644
@@ -769,7 +769,7 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
 	} else {
 		struct tls_sw_context_rx *rx_ctx = tls_sw_ctx_rx(ctx);
 
-		tls_strp_check_rcv(&rx_ctx->strp);
+		tls_strp_check_rcv(&rx_ctx->strp, true);
 	}
 	return 0;
net/tls/tls_strp.c +12 -11
index e7aaee6efe6e..61b10c697ecc 100644
@@ -368,7 +368,6 @@ static int tls_strp_copyin(read_descriptor_t *desc, struct sk_buff *in_skb,
 		desc->count = 0;
 
 		WRITE_ONCE(strp->msg_ready, 1);
-		tls_rx_msg_ready(strp);
 	}
 
 	return ret;

@@ -492,6 +491,7 @@ bool tls_strp_msg_load(struct tls_strparser *strp, bool force_refresh)
 	if (!strp->copy_mode && force_refresh) {
 		if (unlikely(tcp_inq(strp->sk) < strp->stm.full_len)) {
 			WRITE_ONCE(strp->msg_ready, 0);
+			strp->msg_announced = 0;
 			memset(&strp->stm, 0, sizeof(strp->stm));
 			return false;
 		}

@@ -539,18 +539,24 @@ static int tls_strp_read_sock(struct tls_strparser *strp)
 		return tls_strp_read_copy(strp, false);
 
 	WRITE_ONCE(strp->msg_ready, 1);
-	tls_rx_msg_ready(strp);
 
 	return 0;
 }
 
-void tls_strp_check_rcv(struct tls_strparser *strp)
+/* Parse queued data. When @announce is true and parsing produces a
+ * newly-ready record, fire the consumer notification. Callers that
+ * need to notify a waiter about a record parsed by another path
+ * should invoke tls_rx_msg_maybe_announce() directly.
+ */
+void tls_strp_check_rcv(struct tls_strparser *strp, bool announce)
 {
 	if (unlikely(strp->stopped) || strp->msg_ready)
 		return;
 
 	if (tls_strp_read_sock(strp) == -ENOMEM)
 		queue_work(tls_strp_wq, &strp->work);
+	else if (announce && strp->msg_ready)
+		tls_rx_msg_maybe_announce(strp);
 }
 
 /* Lower sock lock held */

@@ -568,7 +574,7 @@ void tls_strp_data_ready(struct tls_strparser *strp)
 		return;
 	}
 
-	tls_strp_check_rcv(strp);
+	tls_strp_check_rcv(strp, true);
 }
 
 static void tls_strp_work(struct work_struct *w)

@@ -577,7 +583,7 @@ static void tls_strp_work(struct work_struct *w)
 		container_of(w, struct tls_strparser, work);
 
 	lock_sock(strp->sk);
-	tls_strp_check_rcv(strp);
+	tls_strp_check_rcv(strp, true);
 	release_sock(strp->sk);
 }
 

@@ -596,15 +602,10 @@ void tls_strp_msg_consume(struct tls_strparser *strp)
 		tls_strp_flush_anchor_copy(strp);
 
 	WRITE_ONCE(strp->msg_ready, 0);
+	strp->msg_announced = 0;
 	memset(&strp->stm, 0, sizeof(strp->stm));
 }
 
-void tls_strp_msg_done(struct tls_strparser *strp)
-{
-	tls_strp_msg_consume(strp);
-	tls_strp_check_rcv(strp);
-}
-
 void tls_strp_stop(struct tls_strparser *strp)
 {
 	strp->stopped = 1;
net/tls/tls_sw.c +24 -3
index 798f2535ddf7..df4cdf11f784 100644
@@ -1401,7 +1401,10 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
 			return ret;
 
 		if (!skb_queue_empty(&sk->sk_receive_queue)) {
-			tls_strp_check_rcv(&ctx->strp);
+			/* Defer notification to the exit point; this thread
+			 * will consume the record directly.
+			 */
+			tls_strp_check_rcv(&ctx->strp, false);
 			if (tls_strp_msg_ready(ctx))
 				break;
 		}

@@ -1887,9 +1890,13 @@ static int tls_record_content_type(struct msghdr *msg, struct tls_msg *tlm,
 	return 1;
 }
 
+/* The deferred announce is fired once on reader exit by
+ * tls_rx_reader_release().
+ */
 static void tls_rx_rec_done(struct tls_sw_context_rx *ctx)
 {
-	tls_strp_msg_done(&ctx->strp);
+	tls_strp_msg_consume(&ctx->strp);
+	tls_strp_check_rcv(&ctx->strp, false);
 }
 
 /* This function traverses the rx_list in tls receive context to copies the

@@ -2044,6 +2051,12 @@ static int tls_rx_reader_lock(struct sock *sk, struct tls_sw_context_rx *ctx,
 
 static void tls_rx_reader_release(struct sock *sk, struct tls_sw_context_rx *ctx)
 {
+	/* Fire any deferred announce once per reader so that a record
+	 * parsed but not yet announced becomes visible to the next
+	 * reader. The call is idempotent through msg_announced.
+	 */
+	tls_rx_msg_maybe_announce(&ctx->strp);
+
 	if (unlikely(ctx->reader_contended)) {
 		if (wq_has_sleeper(&ctx->wq))
 			wake_up(&ctx->wq);

@@ -2520,10 +2533,18 @@ int tls_rx_msg_size(struct tls_strparser *strp, struct sk_buff *skb)
 	return ret;
 }
 
-void tls_rx_msg_ready(struct tls_strparser *strp)
+/* Fire saved_data_ready() at most once per parsed record. The
+ * msg_announced bit is cleared by tls_strp_msg_consume() when the
+ * record is consumed, arming the next announcement.
+ */
+void tls_rx_msg_maybe_announce(struct tls_strparser *strp)
 {
 	struct tls_sw_context_rx *ctx;
 
+	if (!READ_ONCE(strp->msg_ready) || strp->msg_announced)
+		return;
+	strp->msg_announced = 1;
+
 	ctx = container_of(strp, struct tls_sw_context_rx, strp);
 	ctx->saved_data_ready(strp->sk);
 }
Signature · 1 line
2.54.0
Reviewed-by on v11 5/6
#11 lore raw

Re: [PATCH net-next v11 5/6] tls: Suppress spurious saved_data_ready on all receive paths

Quoted 31 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Each record release via tls_strp_msg_done() triggered tls_strp_check_rcv(), which called tls_rx_msg_ready() and fired saved_data_ready(). During a multi-record receive, the first N-1 wakeups are pure overhead: the caller is already running and will pick up subsequent records on the next loop iteration. The recvmsg and splice_read paths share this waste.

Suppress per-record notifications and emit a single one on reader exit. tls_rx_rec_done() releases the current record and parses the next without announcing; tls_strp_check_rcv() gains a bool announce parameter so callers can request the quiet form. tls_rx_reader_release() fires the deferred announce on exit through tls_rx_msg_maybe_announce(), an idempotent helper that calls saved_data_ready() only when a record is parsed and has not yet been announced.

To keep the final notification idempotent against records that the BH or the worker has already announced, tls_strparser gains a msg_announced bit. tls_rx_msg_maybe_announce() sets the bit when firing saved_data_ready(); the bit is cleared whenever the parsed record is wiped, by tls_strp_msg_consume() on consumption or by tls_strp_msg_load() when the lower socket loses bytes from under the parse. A second call for the same parsed record -- as when recvmsg() satisfies the request from ctx->rx_list without touching the strparser -- becomes a no-op.

With no remaining callers, tls_strp_msg_done() is removed.

Quoted 9 lines
Signed-off-by Chuck Lever <[email protected]>
---
  include/net/tls.h  |  5 +++++
  net/tls/tls.h      |  5 ++---
  net/tls/tls_main.c |  2 +-
  net/tls/tls_strp.c | 23 ++++++++++++-----------
  net/tls/tls_sw.c   | 27 ++++++++++++++++++++++++---
  5 files changed, 44 insertions(+), 18 deletions(-)
Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
maintainer Reviewed-by on v11 5/6
#12 lore raw

Re: [PATCH net-next v11 5/6] tls: Suppress spurious saved_data_ready on all receive paths

In reply to Chuck Lever
Quoted 31 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Each record release via tls_strp_msg_done() triggered tls_strp_check_rcv(), which called tls_rx_msg_ready() and fired saved_data_ready(). During a multi-record receive, the first N-1 wakeups are pure overhead: the caller is already running and will pick up subsequent records on the next loop iteration. The recvmsg and splice_read paths share this waste.

Suppress per-record notifications and emit a single one on reader exit. tls_rx_rec_done() releases the current record and parses the next without announcing; tls_strp_check_rcv() gains a bool announce parameter so callers can request the quiet form. tls_rx_reader_release() fires the deferred announce on exit through tls_rx_msg_maybe_announce(), an idempotent helper that calls saved_data_ready() only when a record is parsed and has not yet been announced.

To keep the final notification idempotent against records that the BH or the worker has already announced, tls_strparser gains a msg_announced bit. tls_rx_msg_maybe_announce() sets the bit when firing saved_data_ready(); the bit is cleared whenever the parsed record is wiped, by tls_strp_msg_consume() on consumption or by tls_strp_msg_load() when the lower socket loses bytes from under the parse. A second call for the same parsed record -- as when recvmsg() satisfies the request from ctx->rx_list without touching the strparser -- becomes a no-op.

With no remaining callers, tls_strp_msg_done() is removed.

Quoted 8 lines
Signed-off-by Chuck Lever <[email protected]>
---
 include/net/tls.h  |  5 +++++
 net/tls/tls.h      |  5 ++---
 net/tls/tls_main.c |  2 +-
 net/tls/tls_strp.c | 23 ++++++++++++-----------
 net/tls/tls_sw.c   | 27 ++++++++++++++++++++++++---
 5 files changed, 44 insertions(+), 18 deletions(-)
Reviewed-by Sabrina Dubroca <[email protected]>
Signature · 1 line
Sabrina
author maintainer
#13 lore raw

PATCH v11 6/6 tls: Flush backlog before waiting for a new record

In reply to Chuck Lever

From: Chuck Lever <[email protected]>

While lock_sock is held, incoming TCP segments land on sk->sk_backlog rather than sk->sk_receive_queue. tls_rx_rec_wait() inspects only sk_receive_queue, so backlog data remains invisible. For non-blocking callers (read_sock, and recvmsg or splice_read with MSG_DONTWAIT) this causes a spurious -EAGAIN. For blocking callers it forces an unnecessary sleep/wakeup cycle.

Flush the backlog inside tls_rx_rec_wait() before checking sk_receive_queue so the strparser can parse newly-arrived segments immediately. On the next loop iteration tls_read_flush_backlog() may redundantly flush, but this path is cold and the cost is negligible.

Backlog processing can run tcp_reset(), which calls tcp_done_with_error() to set sk->sk_err = ECONNRESET and then tcp_done() to set sk->sk_shutdown = SHUTDOWN_MASK. The pre-existing top-of-loop sk_err check already ran before the flush, so the freshly-set error would be masked by the next-line sk_shutdown test returning 0 (EOF). Re-check sk_err immediately before the sk_shutdown test so a connection abort surfaces as -ECONNRESET rather than a clean EOF.

Suggested-by Sabrina Dubroca <[email protected]>
Reviewed-by Hannes Reinecke <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 net/tls/tls_sw.c | 9 +++++++++
 1 file changed, 9 insertions(+)
net/tls/tls_sw.c +9 -0
index df4cdf11f784..d2f31623511a 100644
@@ -1400,6 +1400,8 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
 		if (ret < 0)
 			return ret;
 
+		if (sk_flush_backlog(sk))
+			released = true;
 		if (!skb_queue_empty(&sk->sk_receive_queue)) {
 			/* Defer notification to the exit point; this thread
 			 * will consume the record directly.

@@ -1409,6 +1411,13 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
 				break;
 		}
 
+		/* sk_flush_backlog() can run tcp_reset(), which sets
+		 * sk_err and then sk_shutdown via tcp_done(). Recheck
+		 * sk_err here so a connection abort surfaces as the
+		 * actual error rather than a clean EOF.
+		 */
+		if (sk->sk_err)
+			return -READ_ONCE(sk->sk_err);
 		if (sk->sk_shutdown & RCV_SHUTDOWN)
 			return 0;
Signature · 1 line
2.54.0
maintainer
#14 lore raw

Re: [PATCH net-next v11 6/6] tls: Flush backlog before waiting for a new record

Quoted 14 lines · Chuck Lever
net/tls/tls_sw.c +2 -0
index df4cdf11f784..d2f31623511a 100644
@@ -1400,6 +1400,8 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
 		if (ret < 0)
 			return ret;
 
+		if (sk_flush_backlog(sk))
+			released = true;
 		if (!skb_queue_empty(&sk->sk_receive_queue)) {
 			/* Defer notification to the exit point; this thread
 			 * will consume the record directly.

net/tls/tls_sw.c tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock, · line 1420

Quoted 10 lines
net/tls/tls_sw.c +7 -0
@@ -1409,6 +1411,13 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
 				break;
 		}
 
+		/* sk_flush_backlog() can run tcp_reset(), which sets
+		 * sk_err and then sk_shutdown via tcp_done(). Recheck
+		 * sk_err here so a connection abort surfaces as the
+		 * actual error rather than a clean EOF.
+		 */
+		if (sk->sk_err)
+			return -READ_ONCE(sk->sk_err);

This should have the same "if (has_copied)" handling as the top of the loop got in f508262ae9f2 ("tls: Preserve sk_err across recvmsg() when data has been copied") ?

Signature · 1 line
Sabrina