[PATCH net-next v5 00/10] net/tls: Receive-path fixes for zero-length data records

[PATCH net-next v5 00/10] net/tls: Receive-path fixes for zero-length data records

[Chuck Lever]

Commit 3be28e2c9cd0 ("net/tls: Consume empty data records in
tls_sw_read_sock()") fixed one reader. TLS 1.2 and TLS 1.3 both
permit a zero-length application_data record as a traffic-analysis
countermeasure (RFC 5246, Section 6.2.1; RFC 8446, Section 5.1), so
a peer that pads its stream emits them by design. The other two
software readers still mishandle them. splice(2) reports an empty
record as EOF, and the caller tears down a connection that is still
live. recvmsg(2) makes no progress on one, so a peer that streams
them holds the caller in the kernel past SIGKILL. With MSG_PEEK or
async decryption, each record is also queued on rx_list, which
grows without bound.

This series supersedes "[PATCH net] tls: skip empty data records in
tls_sw_splice_read()", which fixes the splice case alone:
https://lore.kernel.org/netdev/20260930052636.166007-1-qingfang.deng@linux.dev/

Which fix a reader gets depends on its caller. splice(2) and
recvmsg(2) are system calls, so consuming the record and testing
signal_pending() is enough. A signal ends the call. An in-kernel
caller of sock_recvmsg() cannot take a signal, so a flood holds
that caller as it does today. Only tls_sw_read_sock() gets a bound
of its own. Its callers hold the socket lock across the whole call
and cannot act on a signal, so nothing else can stop the loop. A
count of consecutive records that deliver no bytes supplies the
bound (patch 1). The count is scoped to read_sock deliberately. A
flood on the other two paths costs the caller CPU time and nothing
else.

The series changes user-visible behavior. splice(2) on a
nonblocking socket, and sendfile(2) from one, now return -EAGAIN
where they used to block, as they do on a plain TCP socket. A
splice that reaches a control record behind an empty one now
returns -EINVAL rather than the zero that was the false EOF. A
nonblocking recvmsg(2) or splice(2) that has copied nothing and
finds a signal pending after an empty record returns -EINTR. A
recvmsg(2) that has copied data now returns at an empty record
rather than reading the records that follow.

SO_RCVTIMEO does not bound these calls. It is applied at the
reader lock and again on each call to tls_rx_rec_wait(), so a call
that keeps retrying can wait past it. recvmsg(2) behaves this way
today. The retry added to splice(2) extends the same behavior to
that path.

Tested on x86_64. The tls selftest suite passes, 940 tests with
no skips.

---
Changes in v5:
- Restore the v3 data_ready work item in tls_sw_read_sock() (sashiko).
- Queue the work item on tls_strp_wq, not system_wq (sashiko).
- Queue the work item only when a record is left parsed at the break.
- Skip the work item's callback once the strparser is stopped.
- Reword the signal patch: -EAGAIN needs a pending signal (sashiko).
- Cap consecutive empty records in tls_sw_splice_read() (sashiko).
- Define TLS_RX_NODATA_LIMIT above tls_sw_splice_read().
- Add a zero_len_splice variant that reaches the cap.
- Link to v4: https://patch.msgid.link/20261012-tls-follow-on-v4-0-8cea429565da@kernel.org

Changes in v4:
- Drop the data_ready work item; the reader release announces (sashiko).
- Return -EAGAIN to a nonblocking reader with a signal pending (sashiko).
- Add a Fixes: tag to the low-water mark patch (sashiko).
- Add a zero_len variant that pins the low-water mark patch's behavior.
- Link to v3: https://patch.msgid.link/20261007-tls-follow-on-v3-0-6d52d98389b3@kernel.org

Changes in v3:
- Run the consumer's sk_data_ready() from a work item (sashiko).
- Move the sync decrypt of empty records to the signal patch (sashiko).
- Put the O_NONBLOCK patch before the splice retry patch (sashiko).
- Reword the O_NONBLOCK patch description to match (sashiko).
- Say that the recvmsg patch does not bound the receive loop (sashiko).
- Add a patch that returns copied data ahead of empty records (sashiko).
- Put the zero_len skip patch before the coverage patch (sashiko).
- Use poll() to check for records left on rx_list (sashiko).
- Link to v2: https://patch.msgid.link/20261001-tls-follow-on-v2-0-2dd1947bb642@kernel.org

Changes in v2:
- Bound no-data records by count, not elapsed time (Jakub).
- Drop the tls_rx_empty_data_rec() helper (Sabrina).
- Keep the strparser anchor out of tls_sw.c comments (Sabrina).
- Split the recvmsg signal test into its own patch.
- Drop tls_rx_intr_errno(); a nonblocking reader now gets -EINTR.
- Say why do_splice() misses the socket's O_NONBLOCK (Sabrina).
- Point the recvmsg patch's Fixes: at the commit that added rx_list.
- Reuse the new zero_len helpers in the existing fixture (Sabrina).
- Check errno unconditionally in the zero_len tests (Sabrina).
- Split the splice crypto-error fix and its test out (Jakub):
  https://patch.msgid.link/20260806-tls-splice-crypto-fix-v1-0-a2624005a286@kernel.org
- Link to v1: https://patch.msgid.link/20260726-tls-follow-on-v1-0-99bf4cc1c729@kernel.org

---
Chuck Lever (10):
      tls: Bound consecutive no-data records in tls_sw_read_sock()
      tls: Check for a pending signal after an empty record
      tls: Honor O_NONBLOCK in tls_sw_splice_read()
      tls: Consume empty data records in tls_sw_splice_read()
      tls: Consume empty data records in tls_sw_recvmsg()
      tls: Return copied data ahead of a run of empty records
      selftests: tls: Skip the zero_len tests when TLS is unavailable
      selftests: tls: Extend coverage for zero-length records
      selftests: tls: Cover splice on a nonblocking socket
      selftests: tls: Cover the empty-record cap in splice

 include/net/tls.h                 |   1 +
 net/tls/tls.h                     |   2 +
 net/tls/tls_strp.c                |   2 +-
 net/tls/tls_sw.c                  | 112 ++++++++++--
 tools/testing/selftests/net/tls.c | 346 ++++++++++++++++++++++++++++++++++++--
 5 files changed, 435 insertions(+), 28 deletions(-)
---
base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
change-id: 20260726-tls-follow-on-486f1ba8bbb0

Best regards,
--  
Chuck Lever <cel@kernel.org>

[PATCH net-next v5 01/10] tls: Bound consecutive no-data records in tls_sw_read_sock()

[Chuck Lever]

A zero-length application_data record does not deliver a payload, so
tls_sw_read_sock() never runs the read_actor for one and nothing
decrements desc->count. A peer that streams such records keeps the
loop running, with the socket lock held, for as long as they arrive.
The caller cannot bound the run because read_sock() has not
returned.

An LLM audit of the TLS read paths found this defect. The defect has
not been reproduced.

Stop after TLS_RX_NODATA_LIMIT consecutive empty data records. Any
record that delivers bytes resets the count. Stopping with nothing
copied returns zero, which a read_sock consumer takes as no progress
rather than EOF. A record parsed during the call but not yet
delivered needs the consumer called again. tls_rx_reader_release()
wakes only the callback saved when TLS_RX was set up. nvme-tcp
installs its own callback after the handshake, so that wake does not
reach nvme-tcp. Run the consumer's current sk_data_ready() from a work
item once the reader has returned. A direct call from inside the
reader can re-enter tls_sw_read_sock() through a strparser callback.
The work item runs on tls_strp_wq, which is WQ_MEM_RECLAIM, because
nvme-tcp over TLS can sit on the writeback path.

Fixes: 3be28e2c9cd0 ("net/tls: Consume empty data records in tls_sw_read_sock()")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 include/net/tls.h  |  1 +
 net/tls/tls.h      |  2 ++
 net/tls/tls_strp.c |  2 +-
 net/tls/tls_sw.c   | 39 ++++++++++++++++++++++++++++++++++-----
 4 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index e57bef58851e..42fcb9792bcc 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -145,6 +145,7 @@ struct tls_sw_context_rx {
 	atomic_t decrypt_pending;
 	struct sk_buff_head async_hold;
 	struct wait_queue_head wq;
+	struct work_struct data_ready_work;
 };
 
 struct tls_record_info {
diff --git a/net/tls/tls.h b/net/tls/tls.h
index 60a37bdaaa25..c9e3d02ac2c2 100644
--- a/net/tls/tls.h
+++ b/net/tls/tls.h
@@ -184,6 +184,8 @@ int tls_sw_fallback_init(struct sock *sk,
 			 struct tls_offload_context_tx *offload_ctx,
 			 struct tls_crypto_info *crypto_info);
 
+extern struct workqueue_struct *tls_strp_wq;
+
 int tls_strp_dev_init(void);
 void tls_strp_dev_exit(void);
 
diff --git a/net/tls/tls_strp.c b/net/tls/tls_strp.c
index 6cc222008d95..ca8cf8ad80a2 100644
--- a/net/tls/tls_strp.c
+++ b/net/tls/tls_strp.c
@@ -11,7 +11,7 @@
 
 #include "tls.h"
 
-static struct workqueue_struct *tls_strp_wq;
+struct workqueue_struct *tls_strp_wq;
 
 void tls_strp_abort_strp(struct tls_strparser *strp, int err)
 {
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 312e51270f29..37fcca95c046 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1996,6 +1996,8 @@ int tls_sw_recvmsg(struct sock *sk,
 	return copied ? : err;
 }
 
+#define TLS_RX_NODATA_LIMIT	16
+
 ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 			   struct pipe_inode_info *pipe,
 			   size_t len, unsigned int flags)
@@ -2070,6 +2072,20 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	goto splice_read_end;
 }
 
+static void tls_rx_data_ready_work(struct work_struct *w)
+{
+	struct tls_sw_context_rx *ctx =
+		container_of(w, struct tls_sw_context_rx, data_ready_work);
+	struct sock *sk = ctx->strp.sk;
+
+	/* A callback that finds the socket unowned reads from it. */
+	lock_sock(sk);
+	/* Past tls_strp_stop(), sk_data_ready is no longer the consumer's. */
+	if (!ctx->strp.stopped)
+		READ_ONCE(sk->sk_data_ready)(sk);
+	release_sock(sk);
+}
+
 int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 		     sk_read_actor_t read_actor)
 {
@@ -2078,6 +2094,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
 	struct strp_msg *rxm = NULL;
 	struct sk_buff *skb = NULL;
+	unsigned int nodata = 0;
 	struct sk_psock *psock;
 	size_t flushed_at = 0;
 	bool released = true;
@@ -2136,14 +2153,23 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 			goto read_sock_requeue;
 		}
 
-		/* An empty data record (legal in TLS 1.3) gives a zero
-		 * read_actor return, indistinguishable from the consumer
-		 * stalling; the used <= 0 path would requeue it at the
-		 * head of rx_list and block all later records. Consume it
-		 * here instead.
+		/* An empty data record gives a zero read_actor return,
+		 * indistinguishable from the consumer stalling; the
+		 * used <= 0 path would requeue it at the head of rx_list
+		 * and block all later records. Consume it here instead.
 		 */
 		if (rxm->full_len == 0) {
 			consume_skb(skb);
+			if (++nodata >= TLS_RX_NODATA_LIMIT) {
+				/* tls_rx_reader_release() wakes only the
+				 * callback saved when TLS_RX was set up;
+				 * nvme-tcp installs its own afterward.
+				 */
+				if (tls_strp_msg_ready(ctx))
+					queue_work(tls_strp_wq,
+						   &ctx->data_ready_work);
+				break;
+			}
 			continue;
 		}
 
@@ -2154,6 +2180,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 			goto read_sock_requeue;
 		}
 		copied += used;
+		nodata = 0;
 		if (used < rxm->full_len) {
 			rxm->offset += used;
 			rxm->full_len -= used;
@@ -2345,6 +2372,7 @@ void tls_sw_strparser_done(struct tls_context *tls_ctx)
 {
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 
+	cancel_work_sync(&ctx->data_ready_work);
 	tls_strp_done(&ctx->strp);
 }
 
@@ -2476,6 +2504,7 @@ static struct tls_sw_context_rx *init_ctx_rx(struct tls_context *ctx)
 	crypto_init_wait(&sw_ctx_rx->async_wait);
 	atomic_set(&sw_ctx_rx->decrypt_pending, 1);
 	init_waitqueue_head(&sw_ctx_rx->wq);
+	INIT_WORK(&sw_ctx_rx->data_ready_work, tls_rx_data_ready_work);
 	skb_queue_head_init(&sw_ctx_rx->rx_list);
 	skb_queue_head_init(&sw_ctx_rx->async_hold);
 

-- 
2.55.0

[PATCH net-next v5 02/10] tls: Check for a pending signal after an empty record

[Chuck Lever]

tls_rx_rec_wait() tests signal_pending() only after it sleeps, and
it skips the wait loop when a record is already parsed.
tls_sw_recvmsg() calls tls_rx_rec_wait() once per record, and a
zero-length application_data record does not advance either bound
of the receive loop. A peer that streams such records holds the
caller in recvmsg(), unresponsive to SIGKILL, for as long as they
arrive. The hang has not been reproduced.

Test for a pending signal in tls_sw_recvmsg() before it fetches the
record after an empty one. A reader that has copied data gets the
data. A nonblocking reader with a signal pending gets -EAGAIN
rather than -EINTR. Pending task work alone makes signal_pending()
true, and io_uring retries a nonblocking receive only on -EAGAIN.
The test stays out of tls_rx_rec_wait() because tls_sw_read_sock()
also calls tls_rx_rec_wait(), from callers that cannot act on a
signal.

Decrypt an empty record synchronously. The code at recv_end
overwrites err once any record has been decrypted asynchronously.
With only empty records queued, the reader then gets zero, not the
errno.

Fixes: c46234ebb4d1 ("tls: RX path for ktls")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260720-tcp-read-sock-v2-0-29545d034f3c@kernel.org?part=1
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/tls/tls_sw.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 37fcca95c046..ac4da28ed9c8 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1824,6 +1824,7 @@ int tls_sw_recvmsg(struct sock *sk,
 	bool is_peek = flags & MSG_PEEK;
 	bool rx_more = false;
 	bool released = true;
+	bool nodata = false;
 	bool zc_capable;
 
 	if (unlikely(flags & MSG_ERRQUEUE))
@@ -1858,6 +1859,18 @@ int tls_sw_recvmsg(struct sock *sk,
 		struct tls_decrypt_arg darg;
 		int to_decrypt, chunk;
 
+		/* A run of empty records advances neither loop bound, and
+		 * tls_rx_rec_wait() tests for a signal only after it sleeps.
+		 */
+		if (nodata && signal_pending(current)) {
+			long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+
+			err = -EAGAIN;
+			if (timeo)
+				err = sock_intr_errno(timeo);
+			goto recv_end;
+		}
+
 		err = tls_rx_rec_wait(sk, flags & MSG_DONTWAIT,
 				      released, !!(decrypted + copied));
 		if (err <= 0)
@@ -1874,9 +1887,12 @@ int tls_sw_recvmsg(struct sock *sk,
 		    tlm->control == TLS_RECORD_TYPE_DATA)
 			darg.zc = true;
 
-		/* Do not use async mode if record is non-data */
+		/* Do not use async mode if record is non-data, or if it
+		 * is empty: once a record has gone async, recv_end
+		 * discards the error that ends a run of empty records.
+		 */
 		if (tlm->control == TLS_RECORD_TYPE_DATA)
-			darg.async = ctx->async_capable;
+			darg.async = ctx->async_capable && to_decrypt;
 		else
 			darg.async = false;
 
@@ -1910,6 +1926,7 @@ int tls_sw_recvmsg(struct sock *sk,
 		/* TLS 1.3 may have updated the length by more than overhead */
 		rxm = strp_msg(darg.skb);
 		chunk = rxm->full_len;
+		nodata = !chunk;
 		tls_rx_rec_done(ctx);
 
 		if (!darg.zc) {

-- 
2.55.0

[PATCH net-next v5 03/10] tls: Honor O_NONBLOCK in tls_sw_splice_read()

[Chuck Lever]

tls_sw_splice_read() takes its blocking behavior from
SPLICE_F_NONBLOCK alone. When splicing from a socket to a pipe,
do_splice() sets that flag from the pipe's O_NONBLOCK, not the
socket's. On a nonblocking socket, a splice(2) call without
SPLICE_F_NONBLOCK therefore sleeps in tls_rx_rec_wait() until a
record arrives. tcp_splice_read() instead reads sock->file->f_flags
and returns -EAGAIN.

Polling first does not avoid the sleep once tls_sw_splice_read()
consumes zero-length records. tls_sw_sock_is_readable() reports
the socket readable while any record sits on rx_list, including a
zero-length record that delivers no bytes to the pipe. The
strparser announces a record before decryption, when the record's
plaintext length is unknown, so the readiness test cannot screen
such a record out. A splice that consumes the record and waits for
the next stalls every connection an event loop multiplexes.

An LLM audit of the TLS read paths found this defect. With empty
records consumed, the zero_len_splice selftest, run on a
nonblocking socket, reproduces the sleep.

Treat the socket's O_NONBLOCK as nonblocking too. A caller that
sets O_NONBLOCK on the socket and splices without SPLICE_F_NONBLOCK
now gets -EAGAIN where the splice blocked before. sendfile(2) from
a TLS socket changes the same way, because do_sendfile() leaves the
input file's O_NONBLOCK out of the splice flags. Both now behave as
they do on a plain TCP socket.

Fixes: c46234ebb4d1 ("tls: RX path for ktls")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/tls/tls_sw.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index ac4da28ed9c8..cb5a2b29bced 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2026,10 +2026,14 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	struct tls_msg *tlm;
 	struct sk_buff *skb;
 	ssize_t copied = 0;
+	bool nonblock;
 	int chunk;
 	int err;
 
-	err = tls_rx_reader_lock(sk, ctx, flags & SPLICE_F_NONBLOCK);
+	nonblock = (flags & SPLICE_F_NONBLOCK) ||
+		   (sock->file->f_flags & O_NONBLOCK);
+
+	err = tls_rx_reader_lock(sk, ctx, nonblock);
 	if (err < 0)
 		return err;
 
@@ -2043,8 +2047,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	} else {
 		struct tls_decrypt_arg darg;
 
-		err = tls_rx_rec_wait(sk, flags & SPLICE_F_NONBLOCK,
-				      true, false);
+		err = tls_rx_rec_wait(sk, nonblock, true, false);
 		if (err <= 0)
 			goto splice_read_end;
 

-- 
2.55.0

[PATCH net-next v5 04/10] tls: Consume empty data records in tls_sw_splice_read()

[Chuck Lever]

A zero-length application_data record decrypts to full_len == 0,
so tls_sw_splice_read() splices zero bytes and returns zero. A zero
return from a splice read means EOF, and the caller tears down a
connection that is still live.

Consume the record and fetch the next one, as tls_sw_recvmsg()
does. Each fetch restarts SO_RCVTIMEO under the socket lock, so a
stream of empty records could hold a blocking splice and the lock
for as long as the stream lasts. Stop after TLS_RX_NODATA_LIMIT
consecutive empty records and return -EAGAIN. The cap stands in for
the timeout the retry defeats, so a blocking splice with no timeout
set reports -EAGAIN too, as a timed-out receive would. Test for a
pending signal before each retry as well, so the splicing task
stays killable. A nonblocking splice gets -EAGAIN, as
tls_sw_recvmsg() does.

When an empty record precedes a close_notify, splice(2) now returns
-EINVAL from the control-record test instead of a false EOF. A
splice that reaches an alert record already reports -EINVAL.

Fixes: c46234ebb4d1 ("tls: RX path for ktls")
Reported-by: Sabrina Dubroca <sd@queasysnail.net>
Closes: https://lore.kernel.org/netdev/akaoXcfamBp8_mYe@krikkit/
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/tls/tls_sw.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index cb5a2b29bced..53a4af9eeeef 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2025,7 +2025,9 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	struct sock *sk = sock->sk;
 	struct tls_msg *tlm;
 	struct sk_buff *skb;
+	bool released = true;
 	ssize_t copied = 0;
+	int nodata = 0;
 	bool nonblock;
 	int chunk;
 	int err;
@@ -2042,12 +2044,13 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	if (err)
 		goto splice_read_end;
 
+retry:
 	if (!skb_queue_empty(&ctx->rx_list)) {
 		skb = __skb_dequeue(&ctx->rx_list);
 	} else {
 		struct tls_decrypt_arg darg;
 
-		err = tls_rx_rec_wait(sk, nonblock, true, false);
+		err = tls_rx_rec_wait(sk, nonblock, released, false);
 		if (err <= 0)
 			goto splice_read_end;
 
@@ -2059,6 +2062,9 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 
 		tls_rx_rec_done(ctx);
 		skb = darg.skb;
+
+		/* The retry's wait runs with the socket lock still held. */
+		released = false;
 	}
 
 	rxm = strp_msg(skb);
@@ -2070,6 +2076,24 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 		goto splice_requeue;
 	}
 
+	/* Splicing zero bytes reads as EOF to the caller. */
+	if (rxm->full_len == 0) {
+		consume_skb(skb);
+		if (++nodata >= TLS_RX_NODATA_LIMIT) {
+			err = -EAGAIN;
+			goto splice_read_end;
+		}
+		if (signal_pending(current)) {
+			long timeo = sock_rcvtimeo(sk, nonblock);
+
+			err = -EAGAIN;
+			if (timeo)
+				err = sock_intr_errno(timeo);
+			goto splice_read_end;
+		}
+		goto retry;
+	}
+
 	chunk = min_t(unsigned int, rxm->full_len, len);
 	copied = skb_splice_bits(skb, sk, rxm->offset, pipe, chunk, flags);
 	if (copied < 0)

-- 
2.55.0

[PATCH net-next v5 05/10] tls: Consume empty data records in tls_sw_recvmsg()

[Chuck Lever]

TLS 1.2 and TLS 1.3 both permit zero-length application_data
records as a traffic-analysis countermeasure (RFC 5246 Section
6.2.1 and RFC 8446 Section 5.1). A record that decrypts to
full_len == 0 advances neither len nor decrypted, so
tls_sw_recvmsg() keeps reading empty records for as long as they
arrive. The peek arm and the async arm queue each empty record on
rx_list, and a peer that streams empty records grows rx_list
without bound. The growth has not been reproduced.

Consume an empty data record as soon as the receive loop has it,
before the paths diverge on darg.zc. Freeing the skb requires its
decryption to have completed, and an empty record is already
decrypted synchronously. The new branch sets MSG_EOR itself,
because the record no longer reaches the assignment at the bottom
of the loop.

After this change, tls_sw_recvmsg() still reads empty records for
as long as they arrive, until a signal is pending.

Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260630191551.875664-1-cel@kernel.org?part=1
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/tls/tls_sw.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 53a4af9eeeef..153ff135a395 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1888,8 +1888,8 @@ int tls_sw_recvmsg(struct sock *sk,
 			darg.zc = true;
 
 		/* Do not use async mode if record is non-data, or if it
-		 * is empty: once a record has gone async, recv_end
-		 * discards the error that ends a run of empty records.
+		 * is empty: the receive loop frees an empty record's skb,
+		 * which an async decrypt would still be using.
 		 */
 		if (tlm->control == TLS_RECORD_TYPE_DATA)
 			darg.async = ctx->async_capable && to_decrypt;
@@ -1929,6 +1929,19 @@ int tls_sw_recvmsg(struct sock *sk,
 		nodata = !chunk;
 		tls_rx_rec_done(ctx);
 
+		/* Keep an empty record off rx_list. On the zero-copy path
+		 * the strparser owns darg.skb, and tls_rx_rec_done() has
+		 * released it.
+		 */
+		if (!chunk && control == TLS_RECORD_TYPE_DATA) {
+			if (!darg.zc)
+				consume_skb(darg.skb);
+
+			/* An empty record still marks a boundary. */
+			msg->msg_flags |= MSG_EOR;
+			continue;
+		}
+
 		if (!darg.zc) {
 			bool partially_consumed = chunk > len;
 			struct sk_buff *skb = darg.skb;

-- 
2.55.0

[PATCH net-next v5 06/10] tls: Return copied data ahead of a run of empty records

[Chuck Lever]

Once tls_sw_recvmsg() has copied enough data to meet the receive
low-water mark, the receive loop continues only while another
record is ready. A zero-length application_data record is consumed
without advancing len, so a peer that follows data with a stream of
empty records keeps the reader in the loop. The reader does not get
the data already copied until the stream stops or a signal is
pending. The delay has not been reproduced.

Leave the receive loop at an empty record once the low-water mark
is met. A reader that has copied nothing still reads empty records
for as long as they arrive.

Fixes: 04b25a5411f9 ("net/tls: fix no wakeup on partial reads")
Suggested-by: sashiko-bot <netdev-bot+sashiko@kernel.org>
Link: https://lore.kernel.org/r/179124254665.434549.12245680127262431659@kernel.org
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/tls/tls_sw.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 153ff135a395..9b4e66567ee8 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1939,6 +1939,12 @@ int tls_sw_recvmsg(struct sock *sk,
 
 			/* An empty record still marks a boundary. */
 			msg->msg_flags |= MSG_EOR;
+
+			/* Return data already copied instead of holding it
+			 * through a run of empty records.
+			 */
+			if (decrypted + copied >= target)
+				break;
 			continue;
 		}
 

-- 
2.55.0

[PATCH net-next v5 07/10] selftests: tls: Skip the zero_len tests when TLS is unavailable

[Chuck Lever]

FIXTURE_SETUP(zero_len) returns early when ulp_sock_pair() reports
that the TCP_ULP setsockopt failed, so on a kernel built without
CONFIG_TLS the TLS_RX setsockopt never runs. TEST_F(zero_len, test)
then sends its raw records over a plain TCP socket, which returns
the ciphertext verbatim. All eight variants FAIL on a machine that
lacks TLS, and those failures mask any real regression in the same
run.

Skip the test when the fixture recorded notls, as the other
fixtures in this file do.

Fixes: a61a3e961baf ("selftests: tls: add tests for zero-length records")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/testing/selftests/net/tls.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 9d3cd4fff062..0bc43728262e 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2639,6 +2639,9 @@ TEST_F(zero_len, test)
 	int rec_off;
 	int i;
 
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
 	for (i = 0; i < 4 && variant->recs[i]; i++)
 		EXPECT_EQ(send(self->fd, variant->recs[i]->cipher_data,
 			       variant->recs[i]->cipher_len, 0),

-- 
2.55.0

[PATCH net-next v5 08/10] selftests: tls: Extend coverage for zero-length records

[Chuck Lever]

The zero_len fixture injects raw pre-encrypted records over a
socket carrying a TLS_RX key only, and some of those records are
zero-length application_data records. Every variant reads the
records back with a plain recv(), so neither splice nor MSG_PEEK is
tested against an empty record. No variant places an empty record
between two data records, the one sequence where returning copied
data ahead of a run of empty records changes what recv() reports.

Add zero_len_splice and zero_len_peek. A zero return from splice()
means EOF, so zero_len_splice checks that a run of empty data
records does not end a live connection. zero_len_peek checks that a
peek reaches the payload behind a run of empty records and leaves
the payload in place for the read that follows. When a payload does
not follow the run, the peek must leave the socket unreadable. An
unfixed kernel returns 0 from splice(), and after the peek poll()
reports the socket readable.

Add data_0data_data to zero_len. The first recv() returns the first
record alone and leaves the second for the next call. A kernel that
reads through the empty record returns both payloads from the first
call and fails the variant.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/testing/selftests/net/tls.c | 232 +++++++++++++++++++++++++++++++++++---
 1 file changed, 215 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 0bc43728262e..8ce5a9032960 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2549,6 +2549,42 @@ static const struct raw_rec id2_data_l0 = {
 	},
 };
 
+static void zero_len_sock_pair(struct __test_metadata *_metadata,
+			       int *fd, int *cfd, bool *notls)
+{
+	struct tls_crypto_info_keys tls12;
+	int ret;
+
+	tls_crypto_info_init(TLS_1_2_VERSION, TLS_CIPHER_AES_CCM_128,
+			     &tls12, 0);
+
+	ulp_sock_pair(_metadata, fd, cfd, notls);
+	if (*notls)
+		return;
+
+	/* fd stays keyless; these fixtures send raw records over it */
+	ret = setsockopt(*cfd, SOL_TLS, TLS_RX, &tls12, tls12.len);
+	ASSERT_EQ(ret, 0);
+}
+
+/* Send a variant's records; return the last one carrying payload */
+static const struct raw_rec *
+zero_len_send_recs(struct __test_metadata *_metadata, int fd,
+		   const struct raw_rec *const *recs)
+{
+	const struct raw_rec *payload = NULL;
+	int i;
+
+	for (i = 0; i < 4 && recs[i]; i++) {
+		EXPECT_EQ(send(fd, recs[i]->cipher_data, recs[i]->cipher_len, 0),
+			  recs[i]->cipher_len);
+		if (recs[i]->plain_len)
+			payload = recs[i];
+	}
+
+	return payload;
+}
+
 FIXTURE(zero_len)
 {
 	int fd, cfd;
@@ -2609,21 +2645,15 @@ FIXTURE_VARIANT_ADD(zero_len, data_0data_0data)
 	.recv_ret = { 11, -EAGAIN, },
 };
 
+FIXTURE_VARIANT_ADD(zero_len, data_0data_data)
+{
+	.recs = { &id0_data_l11, &id1_data_l0, &id2_data_l11, },
+	.recv_ret = { 11, 11, -EAGAIN, },
+};
+
 FIXTURE_SETUP(zero_len)
 {
-	struct tls_crypto_info_keys tls12;
-	int ret;
-
-	tls_crypto_info_init(TLS_1_2_VERSION, TLS_CIPHER_AES_CCM_128,
-			     &tls12, 0);
-
-	ulp_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
-	if (self->notls)
-		return;
-
-	/* Don't install keys on fd, we'll send raw records */
-	ret = setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len);
-	ASSERT_EQ(ret, 0);
+	zero_len_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
 }
 
 FIXTURE_TEARDOWN(zero_len)
@@ -2642,10 +2672,7 @@ TEST_F(zero_len, test)
 	if (self->notls)
 		SKIP(return, "no TLS support");
 
-	for (i = 0; i < 4 && variant->recs[i]; i++)
-		EXPECT_EQ(send(self->fd, variant->recs[i]->cipher_data,
-			       variant->recs[i]->cipher_len, 0),
-			  variant->recs[i]->cipher_len);
+	zero_len_send_recs(_metadata, self->fd, variant->recs);
 
 	rec = &variant->recs[0];
 	rec_off = 0;
@@ -2671,6 +2698,177 @@ TEST_F(zero_len, test)
 	}
 };
 
+FIXTURE(zero_len_peek)
+{
+	int fd, cfd;
+	bool notls;
+};
+
+FIXTURE_VARIANT(zero_len_peek)
+{
+	const struct raw_rec *recs[4];
+	ssize_t peek_ret;
+};
+
+FIXTURE_VARIANT_ADD(zero_len_peek, 0data_0data_data)
+{
+	.recs = { &id0_data_l0, &id1_data_l0, &id2_data_l11, },
+	.peek_ret = 11,
+};
+
+FIXTURE_VARIANT_ADD(zero_len_peek, 0data_0data_0data)
+{
+	.recs = { &id0_data_l0, &id1_data_l0, &id2_data_l0, },
+	.peek_ret = -EAGAIN,
+};
+
+FIXTURE_SETUP(zero_len_peek)
+{
+	zero_len_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
+}
+
+FIXTURE_TEARDOWN(zero_len_peek)
+{
+	close(self->fd);
+	close(self->cfd);
+}
+
+/* Peeking past a run of empty data records must reach the payload
+ * behind them, and a run with no payload behind it must report EAGAIN
+ * and leave no record queued.
+ */
+TEST_F(zero_len_peek, test)
+{
+	struct pollfd pfd = { .fd = self->cfd, .events = POLLIN };
+	const struct raw_rec *payload;
+	unsigned char buf[128];
+	ssize_t ret;
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	payload = zero_len_send_recs(_metadata, self->fd, variant->recs);
+
+	if (variant->peek_ret < 0) {
+		ret = recv(self->cfd, buf, sizeof(buf),
+			   MSG_DONTWAIT | MSG_PEEK);
+		EXPECT_EQ(ret, -1);
+		EXPECT_EQ(errno, -variant->peek_ret);
+
+		/* A record left on rx_list keeps the socket readable */
+		EXPECT_EQ(poll(&pfd, 1, 0), 0);
+		return;
+	}
+
+	ret = recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT | MSG_PEEK);
+	EXPECT_EQ(ret, variant->peek_ret);
+	if (ret == variant->peek_ret)
+		EXPECT_EQ(memcmp(buf, payload->plain_data,
+				 variant->peek_ret), 0);
+
+	ret = recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT);
+	EXPECT_EQ(ret, variant->peek_ret);
+	if (ret == variant->peek_ret)
+		EXPECT_EQ(memcmp(buf, payload->plain_data,
+				 variant->peek_ret), 0);
+
+	ret = recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT);
+	EXPECT_EQ(ret, -1);
+	EXPECT_EQ(errno, EAGAIN);
+}
+
+FIXTURE(zero_len_splice)
+{
+	int fd, cfd;
+	bool notls;
+};
+
+FIXTURE_VARIANT(zero_len_splice)
+{
+	const struct raw_rec *recs[4];
+	ssize_t splice_ret;
+};
+
+FIXTURE_VARIANT_ADD(zero_len_splice, 0data_data)
+{
+	.recs = { &id0_data_l0, &id1_data_l11, },
+	.splice_ret = 11,
+};
+
+FIXTURE_VARIANT_ADD(zero_len_splice, 0data_0data_data)
+{
+	.recs = { &id0_data_l0, &id1_data_l0, &id2_data_l11, },
+	.splice_ret = 11,
+};
+
+FIXTURE_VARIANT_ADD(zero_len_splice, 0data_0data_0data)
+{
+	.recs = { &id0_data_l0, &id1_data_l0, &id2_data_l0, },
+	.splice_ret = -EAGAIN,
+};
+
+FIXTURE_VARIANT_ADD(zero_len_splice, 0data_0ctrl)
+{
+	.recs = { &id0_data_l0, &id1_ctrl_l0, },
+	.splice_ret = -EINVAL,
+};
+
+FIXTURE_SETUP(zero_len_splice)
+{
+	zero_len_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
+}
+
+FIXTURE_TEARDOWN(zero_len_splice)
+{
+	close(self->fd);
+	close(self->cfd);
+}
+
+/* Splicing must skip a run of empty data records to reach the payload
+ * behind it, since a zero-byte splice reads as EOF. Splice reports
+ * EAGAIN for a run with no payload behind it, and EINVAL for a control
+ * record behind the run.
+ */
+TEST_F(zero_len_splice, test)
+{
+	struct pollfd pfd = { .fd = self->cfd, .events = POLLIN };
+	const struct raw_rec *payload;
+	unsigned char buf[128];
+	ssize_t ret;
+	int p[2];
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	ASSERT_GE(pipe(p), 0);
+
+	payload = zero_len_send_recs(_metadata, self->fd, variant->recs);
+
+	if (variant->splice_ret < 0) {
+		ret = splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
+			     SPLICE_F_NONBLOCK);
+		EXPECT_EQ(ret, -1);
+		EXPECT_EQ(errno, -variant->splice_ret);
+	} else {
+		/* ASSERT so a zero return stops the test here; the read
+		 * below would block until the harness timeout.
+		 */
+		ASSERT_EQ(splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
+				 SPLICE_F_NONBLOCK), variant->splice_ret);
+		ret = read(p[0], buf, sizeof(buf));
+		EXPECT_EQ(ret, variant->splice_ret);
+		if (ret == variant->splice_ret)
+			EXPECT_EQ(memcmp(buf, payload->plain_data,
+					 variant->splice_ret), 0);
+
+		/* The empty records were consumed, not left queued */
+		EXPECT_EQ(poll(&pfd, 1, 0), 0);
+	}
+
+	close(p[0]);
+	close(p[1]);
+}
+
 FIXTURE(tls_err)
 {
 	int fd, cfd;

-- 
2.55.0

[PATCH net-next v5 09/10] selftests: tls: Cover splice on a nonblocking socket

[Chuck Lever]

Every existing splice call in this file either passes
SPLICE_F_NONBLOCK or runs on a blocking socket, so nothing
exercises the socket's own O_NONBLOCK on the splice path. A change
that stops consulting sock->file->f_flags puts splice(2) and
sendfile(2) back to sleeping in tls_rx_rec_wait() while a peer
streams empty records, and the suite still reports pass.

Run the zero-length record variants a second time with O_NONBLOCK
set on the socket and SPLICE_F_NONBLOCK left out of the splice
flags. Each variant expects the same result with either flag. A
kernel that ignores the socket flag sleeps in the EAGAIN variant
until the harness timeout rather than returning.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/testing/selftests/net/tls.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 8ce5a9032960..57edf6e6640f 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2829,7 +2829,11 @@ FIXTURE_TEARDOWN(zero_len_splice)
  * EAGAIN for a run with no payload behind it, and EINVAL for a control
  * record behind the run.
  */
-TEST_F(zero_len_splice, test)
+static void
+zero_len_do_splice(struct __test_metadata *_metadata,
+		   FIXTURE_DATA(zero_len_splice) *self,
+		   const FIXTURE_VARIANT(zero_len_splice) *variant,
+		   unsigned int splice_flags)
 {
 	struct pollfd pfd = { .fd = self->cfd, .events = POLLIN };
 	const struct raw_rec *payload;
@@ -2837,16 +2841,13 @@ TEST_F(zero_len_splice, test)
 	ssize_t ret;
 	int p[2];
 
-	if (self->notls)
-		SKIP(return, "no TLS support");
-
 	ASSERT_GE(pipe(p), 0);
 
 	payload = zero_len_send_recs(_metadata, self->fd, variant->recs);
 
 	if (variant->splice_ret < 0) {
 		ret = splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
-			     SPLICE_F_NONBLOCK);
+			     splice_flags);
 		EXPECT_EQ(ret, -1);
 		EXPECT_EQ(errno, -variant->splice_ret);
 	} else {
@@ -2854,7 +2855,7 @@ TEST_F(zero_len_splice, test)
 		 * below would block until the harness timeout.
 		 */
 		ASSERT_EQ(splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
-				 SPLICE_F_NONBLOCK), variant->splice_ret);
+				 splice_flags), variant->splice_ret);
 		ret = read(p[0], buf, sizeof(buf));
 		EXPECT_EQ(ret, variant->splice_ret);
 		if (ret == variant->splice_ret)
@@ -2869,6 +2870,32 @@ TEST_F(zero_len_splice, test)
 	close(p[1]);
 }
 
+TEST_F(zero_len_splice, test)
+{
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	zero_len_do_splice(_metadata, self, variant, SPLICE_F_NONBLOCK);
+}
+
+/* The socket's O_NONBLOCK governs the record wait, as on a plain TCP
+ * socket, so a splice without SPLICE_F_NONBLOCK behaves like one that
+ * sets it.
+ */
+TEST_F(zero_len_splice, nonblock_socket)
+{
+	int sflags;
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	sflags = fcntl(self->cfd, F_GETFL, 0);
+	ASSERT_GE(sflags, 0);
+	ASSERT_EQ(fcntl(self->cfd, F_SETFL, sflags | O_NONBLOCK), 0);
+
+	zero_len_do_splice(_metadata, self, variant, 0);
+}
+
 FIXTURE(tls_err)
 {
 	int fd, cfd;

-- 
2.55.0

[PATCH net-next v5 10/10] selftests: tls: Cover the empty-record cap in splice

[Chuck Lever]

tls_sw_splice_read() stops after TLS_RX_NODATA_LIMIT consecutive
empty data records and reports EAGAIN with the next record still
queued. No zero_len_splice variant reaches the cap: the raw_rec
fixtures carry empty records at sequence numbers 0 to 2 only.

Add a variant that sends sixteen empty records ahead of a payload
record. The empty records differ only in sequence number and AEAD
tag, so a table of tags and a sender that fills in the header and
nonce stand in for sixteen raw_rec fixtures. The first splice
reports EAGAIN at the cap, and the second splices the payload.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/testing/selftests/net/tls.c | 116 ++++++++++++++++++++++++++++++++------
 1 file changed, 100 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 57edf6e6640f..38443675bc2f 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2549,6 +2549,61 @@ static const struct raw_rec id2_data_l0 = {
 	},
 };
 
+/* TLS 1.2, AES_CCM, data, seqno:16, plaintext: 'Hello world' */
+static const struct raw_rec id16_data_l11 = {
+	.plain_len = 11,
+	.plain_data = {
+		0x48, 0x65, 0x6c, 0x6c, 0x6f, 0x20, 0x77, 0x6f,
+		0x72, 0x6c, 0x64,
+	},
+	.cipher_len = 40,
+	.cipher_data = {
+		0x17, 0x03, 0x03, 0x00, 0x23, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x10, 0xd3, 0x50, 0x90,
+		0xe1, 0x2a, 0x77, 0xc9, 0x38, 0x95, 0xef, 0xa4,
+		0x6f, 0x5e, 0xb6, 0x52, 0x05, 0x4b, 0x01, 0x44,
+		0x06, 0xc2, 0x51, 0x3f, 0x5e, 0xa5, 0x3e, 0x65,
+	},
+};
+
+/* TLS 1.2, AES_CCM, data, seqno:0-15, plaintext: '' -- the AEAD tag
+ * of each; the header and explicit nonce are built at send time
+ */
+static const unsigned char nodata_tags[16][16] = {
+	{ 0xc5, 0x37, 0x90, 0x70, 0x45, 0x89, 0xfb, 0x5c,
+	  0xc7, 0x89, 0x03, 0x68, 0x80, 0xd3, 0xd8, 0xcc, },
+	{ 0xce, 0xfc, 0x86, 0xc8, 0xf0, 0x55, 0xf9, 0x47,
+	  0x3f, 0x74, 0xdc, 0xc9, 0xbf, 0xfe, 0x5b, 0xb1, },
+	{ 0xc3, 0xca, 0x26, 0x22, 0xe4, 0x25, 0xfb, 0x5f,
+	  0x6d, 0xbf, 0x83, 0x30, 0x48, 0x69, 0x1a, 0x47, },
+	{ 0x4d, 0xfc, 0xe2, 0xc3, 0xfe, 0x11, 0x80, 0x85,
+	  0x94, 0x53, 0x79, 0x1d, 0x31, 0x14, 0x39, 0xec, },
+	{ 0x1e, 0x63, 0xa1, 0xfa, 0xef, 0xf1, 0x9e, 0x5c,
+	  0xee, 0x39, 0xcf, 0x41, 0x27, 0x36, 0xf5, 0x69, },
+	{ 0xa3, 0x49, 0x72, 0x01, 0x1a, 0xff, 0x1f, 0x71,
+	  0x51, 0x57, 0x80, 0xd2, 0x81, 0xee, 0xef, 0x75, },
+	{ 0x95, 0x92, 0x72, 0xda, 0x7a, 0x4e, 0x68, 0x91,
+	  0x02, 0x09, 0xfc, 0xe8, 0x2b, 0x3a, 0x54, 0x05, },
+	{ 0x41, 0x00, 0x73, 0xcf, 0xd3, 0xb3, 0xa7, 0x19,
+	  0x7d, 0x0f, 0x6d, 0x9d, 0xf6, 0x57, 0xee, 0xdd, },
+	{ 0xb5, 0x24, 0x17, 0x2d, 0xac, 0xcf, 0x5f, 0xac,
+	  0x2d, 0x24, 0xf2, 0x8c, 0xaf, 0xf3, 0xd7, 0x78, },
+	{ 0xf8, 0x23, 0xde, 0x09, 0x08, 0x7c, 0x75, 0x7c,
+	  0x23, 0x3a, 0xb6, 0x46, 0xc2, 0xfc, 0xac, 0xdb, },
+	{ 0x72, 0x01, 0x24, 0x35, 0x9f, 0x13, 0x92, 0xb2,
+	  0x93, 0xd9, 0xb4, 0x2e, 0xfc, 0xfc, 0x95, 0xa7, },
+	{ 0x2f, 0x0f, 0x94, 0xcf, 0x1a, 0x0f, 0x1a, 0xf1,
+	  0xe2, 0x6a, 0x97, 0x21, 0x12, 0x5f, 0x24, 0x39, },
+	{ 0xb4, 0xb9, 0xe1, 0x0a, 0x9c, 0x28, 0xbb, 0xf6,
+	  0xd9, 0xd7, 0x39, 0xf6, 0xfa, 0xcd, 0x47, 0xd6, },
+	{ 0xe8, 0xb9, 0x01, 0x90, 0x2e, 0x72, 0xa1, 0xd5,
+	  0x13, 0xdc, 0x28, 0xa7, 0x91, 0x04, 0x9b, 0xcb, },
+	{ 0x73, 0x67, 0x17, 0x6e, 0x50, 0xee, 0x28, 0x3b,
+	  0x6e, 0x2b, 0x32, 0xdd, 0xa1, 0x55, 0x97, 0x7a, },
+	{ 0xaf, 0xd4, 0xed, 0x91, 0xc3, 0x91, 0x21, 0x55,
+	  0xd8, 0xef, 0x23, 0xbe, 0xbf, 0x7b, 0xf2, 0x36, },
+};
+
 static void zero_len_sock_pair(struct __test_metadata *_metadata,
 			       int *fd, int *cfd, bool *notls)
 {
@@ -2585,6 +2640,21 @@ zero_len_send_recs(struct __test_metadata *_metadata, int fd,
 	return payload;
 }
 
+/* Send @count empty data records with sequence numbers 0 to @count - 1 */
+static void
+zero_len_send_nodata(struct __test_metadata *_metadata, int fd,
+		     unsigned int count)
+{
+	unsigned char rec[29] = { 0x17, 0x03, 0x03, 0x00, 0x18, };
+	unsigned int seq;
+
+	for (seq = 0; seq < count; seq++) {
+		rec[12] = seq;
+		memcpy(rec + 13, nodata_tags[seq], 16);
+		EXPECT_EQ(send(fd, rec, sizeof(rec), 0), sizeof(rec));
+	}
+}
+
 FIXTURE(zero_len)
 {
 	int fd, cfd;
@@ -2785,6 +2855,7 @@ FIXTURE(zero_len_splice)
 
 FIXTURE_VARIANT(zero_len_splice)
 {
+	unsigned int nodata;
 	const struct raw_rec *recs[4];
 	ssize_t splice_ret;
 };
@@ -2813,6 +2884,14 @@ FIXTURE_VARIANT_ADD(zero_len_splice, 0data_0ctrl)
 	.splice_ret = -EINVAL,
 };
 
+/* TLS_RX_NODATA_LIMIT empty records ahead of the payload */
+FIXTURE_VARIANT_ADD(zero_len_splice, 0data_x16_data)
+{
+	.nodata = 16,
+	.recs = { &id16_data_l11, },
+	.splice_ret = -EAGAIN,
+};
+
 FIXTURE_SETUP(zero_len_splice)
 {
 	zero_len_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
@@ -2827,7 +2906,8 @@ FIXTURE_TEARDOWN(zero_len_splice)
 /* Splicing must skip a run of empty data records to reach the payload
  * behind it, since a zero-byte splice reads as EOF. Splice reports
  * EAGAIN for a run with no payload behind it, and EINVAL for a control
- * record behind the run.
+ * record behind the run. A run at the cap reports EAGAIN with the
+ * payload left for the next splice.
  */
 static void
 zero_len_do_splice(struct __test_metadata *_metadata,
@@ -2843,29 +2923,33 @@ zero_len_do_splice(struct __test_metadata *_metadata,
 
 	ASSERT_GE(pipe(p), 0);
 
+	zero_len_send_nodata(_metadata, self->fd, variant->nodata);
 	payload = zero_len_send_recs(_metadata, self->fd, variant->recs);
 
+	ret = splice(self->cfd, NULL, p[1], NULL, sizeof(buf), splice_flags);
 	if (variant->splice_ret < 0) {
-		ret = splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
-			     splice_flags);
 		EXPECT_EQ(ret, -1);
 		EXPECT_EQ(errno, -variant->splice_ret);
-	} else {
-		/* ASSERT so a zero return stops the test here; the read
-		 * below would block until the harness timeout.
-		 */
-		ASSERT_EQ(splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
-				 splice_flags), variant->splice_ret);
-		ret = read(p[0], buf, sizeof(buf));
-		EXPECT_EQ(ret, variant->splice_ret);
-		if (ret == variant->splice_ret)
-			EXPECT_EQ(memcmp(buf, payload->plain_data,
-					 variant->splice_ret), 0);
+		if (!payload)
+			goto out;
 
-		/* The empty records were consumed, not left queued */
-		EXPECT_EQ(poll(&pfd, 1, 0), 0);
+		ret = splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
+			     splice_flags);
 	}
 
+	/* ASSERT so a zero return stops the test here; the read
+	 * below would block until the harness timeout.
+	 */
+	ASSERT_EQ(ret, payload->plain_len);
+	ret = read(p[0], buf, sizeof(buf));
+	EXPECT_EQ(ret, payload->plain_len);
+	if (ret == payload->plain_len)
+		EXPECT_EQ(memcmp(buf, payload->plain_data, ret), 0);
+
+	/* The empty records were consumed, not left queued */
+	EXPECT_EQ(poll(&pfd, 1, 0), 0);
+
+out:
 	close(p[0]);
 	close(p[1]);
 }

-- 
2.55.0

Re: [PATCH net-next v5 01/10] tls: Bound consecutive no-data records in tls_sw_read_sock()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new limit only counts consecutive empty records, so a peer
  can get around it by mixing in small records.
- [Medium] When tls_sw_read_sock() stops at TLS_RX_NODATA_LIMIT, it
  queues data_ready_work only if tls_strp_msg_ready(ctx) is true…
- [Low] (Baseline: not confirmed after checking the end of the series.)
  At this commit, the wake condition at the new TLS_RX_NODATA_LIMIT exit…

--

tls: Bound consecutive no-data records in tls_sw_read_sock()

This patch makes tls_sw_read_sock() stop after TLS_RX_NODATA_LIMIT (16)
consecutive zero-length application_data records. Any record that delivers
bytes resets the count. When the loop stops and a parsed record is still
pending, a work item on tls_strp_wq calls the socket's current
sk_data_ready(), so consumers such as nvme-tcp get woken again.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 312e51270f293..37fcca95c0467 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -2136,14 +2153,23 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
>  			goto read_sock_requeue;
>  		}
>  

[ ... ]

>  		if (rxm->full_len == 0) {
>  			consume_skb(skb);
> +			if (++nodata >= TLS_RX_NODATA_LIMIT) {
> +				/* tls_rx_reader_release() wakes only the
> +				 * callback saved when TLS_RX was set up;
> +				 * nvme-tcp installs its own afterward.
> +				 */
> +				if (tls_strp_msg_ready(ctx))
> +					queue_work(tls_strp_wq,
> +						   &ctx->data_ready_work);

[Severity: Medium]
What happens when this limit is hit after the parse of the next record
was deferred to strp->work?

After the 16th empty record, tls_rx_rec_done()->tls_strp_check_rcv() can
reach tls_strp_read_copy() if the receive queue layout forces copy mode.
That path calls alloc_page(strp->sk->sk_allocation), and
nvme_tcp_alloc_queue() sets sk_allocation to GFP_ATOMIC. If the
allocation fails:

net/tls/tls_strp.c:tls_strp_check_rcv() {
    ...
	if (tls_strp_read_sock(strp) == -ENOMEM)
		queue_work(tls_strp_wq, &strp->work);
    ...
}

then msg_ready stays false. The new break skips data_ready_work and
returns 0, so nvme_tcp_io_work() sees no progress and exits.

When strp->work parses the record later, tls_rx_msg_maybe_announce()
sends the announcement here:

	ctx->saved_data_ready(strp->sk);

That is the pre-TLS callback, not nvme_tcp_data_ready(), which does not
chain to it. Only nvme_tcp_data_ready() schedules io_work.

Could the received record then sit unread until unrelated traffic or an
I/O timeout runs io_work again?

Before this patch, the loop went back into tls_rx_rec_wait(), which
retries tls_strp_check_rcv() right away. Hitting this stall needed two
allocation failures in a row.

[Severity: Low]
Should this wake condition also look at ctx->rx_list?
tls_sw_sock_is_readable() treats either of these as readable:

	return tls_strp_msg_ready(ctx) ||
		!skb_queue_empty(&ctx->rx_list);

At this commit, tls_sw_recvmsg() with MSG_PEEK can use put_on_rx_list to
leave 16 or more empty data records, followed by a non-empty record, on
rx_list while msg_ready is false. kcm_attach() only checks the family and
protocol, so a read_sock consumer can then be attached to that kTLS
socket.

In that case tls_sw_read_sock() would consume the 16 empty records from
rx_list, break without queueing data_ready_work, and return 0. The
decrypted payload would stay on rx_list.

The later patch in this series, "tls: Consume empty data records in
tls_sw_recvmsg()", appears to keep empty data records off rx_list. With
it, rx_list would be empty whenever this limit fires, so the issue goes
away at the end of the series. It still seems reachable with this commit
alone.

> +				break;
> +			}
>  			continue;
>  		}
>  
> @@ -2154,6 +2180,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
>  			goto read_sock_requeue;
>  		}
>  		copied += used;
> +		nodata = 0;

[Severity: High]
Can a peer get around the bound by resetting nodata here?

The commit message says:

    A zero-length application_data record does not deliver a payload, so
    tls_sw_read_sock() never runs the read_actor for one and nothing
    decrements desc->count.

That implies desc->count bounds the loop for non-empty records. For
nvme-tcp it doesn't seem to:

drivers/nvme/host/tcp.c:nvme_tcp_try_recv() {
    ...
	rd_desc.count = 1;
	lock_sock(sk);
	queue->nr_cqe = 0;
	consumed = sock->ops->read_sock(sk, &rd_desc, nvme_tcp_recv_skb);
    ...
}

nvme_tcp_recv_skb() never decrements desc->count.

The loop's input is also not fixed when it starts. tls_rx_rec_wait()
calls sk_flush_backlog() when no record is ready, and
tls_read_flush_backlog() flushes the backlog periodically inside the
loop.

Suppose the peer keeps repeating "15 empty records, then one 1-byte data
record the consumer accepts". Each delivered byte sets nodata back to 0,
so the ++nodata >= TLS_RX_NODATA_LIMIT check never fires.

Wouldn't tls_sw_read_sock() then keep running with the socket lock held
for as long as the peer keeps sending? That is the case the commit
message says this patch closes.

Would this need a cap on the total number of records per call, or no
backlog flushing inside read_sock, rather than a count of consecutive
empty records?

>  		if (used < rxm->full_len) {
>  			rxm->offset += used;
>  			rxm->full_len -= used;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org

Re: [PATCH net-next v5 02/10] tls: Check for a pending signal after an empty record

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] tls_sw_recvmsg() (net/tls/tls_sw.c) still has no bound on a
  run of zero-length application_data records for any caller that has no…

--

tls: Check for a pending signal after an empty record

The patch adds a signal_pending() check to the tls_sw_recvmsg() receive
loop. The check runs before the next record is fetched after an empty
one, so a stream of zero-length application_data records can no longer
hold a signaled caller in recvmsg(). Empty records are also decrypted
synchronously, so recv_end does not discard the errno.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 37fcca95c0467..ac4da28ed9c89 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -1858,6 +1859,18 @@ int tls_sw_recvmsg(struct sock *sk,
>  		struct tls_decrypt_arg darg;
>  		int to_decrypt, chunk;
>  
> +		/* A run of empty records advances neither loop bound, and
> +		 * tls_rx_rec_wait() tests for a signal only after it sleeps.
> +		 */
> +		if (nodata && signal_pending(current)) {
> +			long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> +
> +			err = -EAGAIN;
> +			if (timeo)
> +				err = sock_intr_errno(timeo);
> +			goto recv_end;
> +		}
> +

[Severity: Medium]
Can a run of empty records still hold tls_sw_recvmsg() indefinitely
when the caller never has a signal pending?

An empty record sets chunk to 0. Neither len nor decrypted moves, so the
loop condition stays true for a reader that has copied nothing:

    while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) {

On each pass, tls_rx_rec_wait() skips its wait loop and returns 1 while
tls_strp_msg_ready() is true. It never reaches either of these exits:

    if (!timeo)
        return -EAGAIN;
    ...
    ret = sk_wait_event(sk, &timeo, ...);

It also recomputes timeo with sock_rcvtimeo() on every call. So
MSG_DONTWAIT, O_NONBLOCK and SO_RCVTIMEO all have no effect while empty
records keep arriving. The socket also stays owned through
tls_rx_reader_lock() the whole time.

The only new exit is signal_pending(). A remote peer cannot make that
true for in-kernel MSG_DONTWAIT readers on kTLS sockets, for example:

net/sunrpc/svcsock.c:svc_tcp_sock_recvmsg() {
    ...
	ret = sock_recvmsg(sock, msg, MSG_DONTWAIT);
    ...
}

nfsd threads run that one. xs_sock_recvmsg() in net/sunrpc/xprtsock.c
is another: xs_stream_data_receive() calls it with MSG_DONTWAIT from the
xprtiod workqueue. Userspace nonblocking event loops that are not being
signaled are in the same position.

The commit message says the tls_sw_read_sock() callers "cannot act on a
signal", and the recvmsg callers above cannot either. Yet
tls_sw_read_sock() and tls_sw_splice_read() each cap consecutive empty
records:

    if (++nodata >= TLS_RX_NODATA_LIMIT) {

tls_sw_recvmsg() keeps only a boolean for the last record:

    nodata = !chunk;

The splice commit later in the series justifies its cap this way: "Each
fetch restarts SO_RCVTIMEO under the socket lock, so a stream of empty
records could hold ... the lock for as long as the stream lasts". Does
the same reasoning apply to recvmsg?

A later commit in the series, "tls: Return copied data ahead of a run of
empty records", handles a reader that has already copied target bytes:
it breaks out of the loop on an empty record. A reader that has copied
nothing still loops. Later commit messages in the series say so: "A
reader that has copied nothing still reads empty records for as long as
they arrive".

Should tls_sw_recvmsg() get the same TLS_RX_NODATA_LIMIT cap as its
sibling read paths?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org

Re: [PATCH net-next v5 03/10] tls: Honor O_NONBLOCK in tls_sw_splice_read()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: the commit message for "tls: Honor O_NONBLOCK in
  tls_sw_splice_read()" justifies the patch partly with an event-loop…

--

tls: Honor O_NONBLOCK in tls_sw_splice_read()

This commit makes tls_sw_splice_read() treat the socket's O_NONBLOCK flag
as nonblocking, in addition to SPLICE_F_NONBLOCK. Splice and sendfile from
a nonblocking TLS socket then return -EAGAIN instead of sleeping in
tls_rx_rec_wait(), as they already do on a plain TCP socket.

> Polling first does not avoid the sleep once tls_sw_splice_read()
> consumes zero-length records. tls_sw_sock_is_readable() reports
> the socket readable while any record sits on rx_list, including a
> zero-length record that delivers no bytes to the pipe. The
> strparser announces a record before decryption, when the record's
> plaintext length is unknown, so the readiness test cannot screen
> such a record out. A splice that consumes the record and waits for
> the next stalls every connection an event loop multiplexes.

[Severity: Low]
This isn't a bug, but could the commit message say that this paragraph
describes behavior added later in the series?

At this commit, tls_sw_splice_read() has no retry loop. An empty record is
spliced as 0 bytes and consumed, and the function returns 0 without waiting
again:

net/tls/tls_sw.c:tls_sw_splice_read() {
    ...
	chunk = min_t(unsigned int, rxm->full_len, len);
	copied = skb_splice_bits(skb, sk, rxm->offset, pipe, chunk, flags);
    ...
}

The "consumes the record and waits for the next" path is added by the next
patch, "tls: Consume empty data records in tls_sw_splice_read()".

> An LLM audit of the TLS read paths found this defect. With empty
> records consumed, the zero_len_splice selftest, run on a
> nonblocking socket, reproduces the sleep.

The zero_len_splice selftest also doesn't exist in
tools/testing/selftests/net/tls.c at this commit. The only zero_len test
here uses recv. zero_len_splice is added later by "selftests: tls: Extend
coverage for zero-length records".

Both claims are worded conditionally ("once ... consumes zero-length
records", "With empty records consumed"). Putting this patch first in the
series also looks deliberate. The O_NONBLOCK problem in the first paragraph
exists on its own since c46234ebb4d1, so the Fixes: tag looks right.

Since this patch carries a Fixes: tag and may be backported alone, would it
help to say that the stall scenario and the zero_len_splice reference
depend on later patches in the series?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org

Re: [PATCH net-next v5 04/10] tls: Consume empty data records in tls_sw_splice_read()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] tls_sw_splice_read() (net/tls/tls_sw.c) now returns -EAGAIN
  from a blocking splice(2) or sendfile(2) once TLS_RX_NODATA_LIMIT (16)…

--

tls: Consume empty data records in tls_sw_splice_read()

With this patch, tls_sw_splice_read() consumes a zero-length
application_data record and fetches the next one. Before, it returned
zero, which the caller reads as EOF. It now stops after
TLS_RX_NODATA_LIMIT consecutive empty records and returns -EAGAIN. It
also checks for a pending signal before each retry.

Closes: https://lore.kernel.org/netdev/akaoXcfamBp8_mYe@krikkit/

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index cb5a2b29bced7..53a4af9eeeefa 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -2042,12 +2044,13 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  	if (err)
>  		goto splice_read_end;
>  
> +retry:
>  	if (!skb_queue_empty(&ctx->rx_list)) {
>  		skb = __skb_dequeue(&ctx->rx_list);
>  	} else {
>  		struct tls_decrypt_arg darg;
>  
> -		err = tls_rx_rec_wait(sk, nonblock, true, false);
> +		err = tls_rx_rec_wait(sk, nonblock, released, false);
>  		if (err <= 0)
>  			goto splice_read_end;

[ ... ]

> @@ -2070,6 +2076,24 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  		goto splice_requeue;
>  	}
>  
> +	/* Splicing zero bytes reads as EOF to the caller. */
> +	if (rxm->full_len == 0) {
> +		consume_skb(skb);
> +		if (++nodata >= TLS_RX_NODATA_LIMIT) {
> +			err = -EAGAIN;
> +			goto splice_read_end;
> +		}

[Severity: Medium]
Can this return -EAGAIN from a blocking splice(2) or sendfile(2) on a
socket that has no SO_RCVTIMEO set?

The nodata counter counts records, not elapsed time, and nothing resets
it during the call. Each goto retry goes back through tls_rx_rec_wait(),
which recomputes the timeout and can sleep:

net/tls/tls_sw.c:tls_rx_rec_wait() {
    ...
	timeo = sock_rcvtimeo(sk, nonblock);
    ...
		ret = sk_wait_event(sk, &timeo,
				    tls_strp_msg_ready(ctx), &wait);
    ...
}

On a blocking socket with no receive timeout, timeo is
MAX_SCHEDULE_TIMEOUT. sk_wait_event() drops the socket lock while it
sleeps. So 16 empty records spread over minutes or hours would still
end the call with -EAGAIN, even though the lock was not held the whole
time.

The commit message says:

    The cap stands in for the timeout the retry defeats, so a blocking
    splice with no timeout set reports -EAGAIN too, as a timed-out
    receive would.

If no timeout is set, is there a timeout for the retry to defeat?

splice(2) documents EAGAIN only for SPLICE_F_NONBLOCK or O_NONBLOCK
descriptors. tcp_splice_read() returns -EAGAIN only when timeo is 0.
RFC 8446 section 5.1 and RFC 5246 section 6.2.1 allow a peer to send
zero-length application_data records, so a remote peer can trigger
this. A blocking caller that treats an unexpected EAGAIN as fatal would
still tear down a live connection, just after 16 empty records instead
of 1.

The new selftest variant 0data_x16_data shows the cap also fires when a
data record is already queued right behind the 16th empty record. Later
patches in the series do not change this splice code, and the final
selftest expects this EAGAIN.

> +		if (signal_pending(current)) {
> +			long timeo = sock_rcvtimeo(sk, nonblock);
> +
> +			err = -EAGAIN;
> +			if (timeo)
> +				err = sock_intr_errno(timeo);
> +			goto splice_read_end;
> +		}
> +		goto retry;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org

Re: [PATCH net-next v5 05/10] tls: Consume empty data records in tls_sw_recvmsg()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message gives a reason for the new MSG_EOR: "The new
  branch sets MSG_EOR itself, because the record no longer reaches the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  tls_sw_recvmsg() still has no limit on how many consecutive empty…
- [Medium] This problem wasn't introduced by this patch, but
  tls_sw_recvmsg() still returns 0 (read as EOF) when empty data records…

--

tls: Consume empty data records in tls_sw_recvmsg()

This commit adds a branch to tls_sw_recvmsg() that frees a zero-length
application_data record right after it is decrypted. The branch runs
before the zero-copy, peek and async paths diverge, so empty records are
no longer queued on rx_list. The branch sets MSG_EOR itself and then
continues the receive loop.

Closes: https://sashiko.dev/#/patchset/20260630191551.875664-1-cel@kernel.org?part=1

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 53a4af9eeeefa..153ff135a395e 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -1929,6 +1929,19 @@ int tls_sw_recvmsg(struct sock *sk,
>  		nodata = !chunk;
>  		tls_rx_rec_done(ctx);
>  
> +		/* Keep an empty record off rx_list. On the zero-copy path
> +		 * the strparser owns darg.skb, and tls_rx_rec_done() has
> +		 * released it.
> +		 */
> +		if (!chunk && control == TLS_RECORD_TYPE_DATA) {

[Severity: Medium]

This isn't a bug introduced by this patch, but can tls_sw_recvmsg() still
return 0 when an empty data record is followed by a non-data record?

When this branch runs, tls_record_content_type() has already set control
to TLS_RECORD_TYPE_DATA for the empty record and emitted a
TLS_GET_RECORD_TYPE cmsg. The next record might be an alert such as
close_notify, or a TLS 1.3 handshake record such as KeyUpdate or
NewSessionTicket. In that case tls_record_content_type() hits this:

net/tls/tls_sw.c:tls_record_content_type() {
    ...
	} else if (*control != tlm->control) {
		return 0;
	}
    ...
}

and tls_sw_recvmsg() takes the err <= 0 branch with err == 0:

		err = tls_record_content_type(msg, tls_msg(darg.skb), &control);
		if (err <= 0) {
			DEBUG_NET_WARN_ON_ONCE(darg.zc);
			tls_rx_rec_done(ctx);
put_on_rx_list_err:
			__skb_queue_tail(&ctx->rx_list, darg.skb);
			goto recv_end;
		}

Nothing has been copied and async is false, so "return copied ? : err;"
returns 0 with a DATA cmsg attached. Callers such as OpenSSL treat that
as EOF. The control record stays on rx_list, so the next recvmsg() does
return it.

The sibling patch "tls: Consume empty data records in
tls_sw_splice_read()" removes this false EOF for splice. The upstream
selftest variant zero_len/0data_0data_ctrl currently expects recv_ret
{0, 11, -EAGAIN}, so the recvmsg behaviour is encoded there.

Could the empty record be consumed before control is latched? If not,
could the commit message say that recvmsg keeps this behaviour?

> +			if (!darg.zc)
> +				consume_skb(darg.skb);
> +
> +			/* An empty record still marks a boundary. */
> +			msg->msg_flags |= MSG_EOR;

[Severity: Low]

The commit message says:

  "The new branch sets MSG_EOR itself, because the record no longer
  reaches the assignment at the bottom of the loop."

Is that accurate for the peek and async arms? Before this patch, an
empty record on the peek arm went through this:

			if (is_peek) {
				peeked += chunk;
				goto put_on_rx_list;
			}

The async arm also queued the skb and hit continue. Neither arm reached
the msg->msg_flags |= MSG_EOR assignment at the bottom of the loop.

This branch now runs before those arms. So a MSG_PEEK recvmsg() with
synchronous crypto that reaches an empty data record now reports
MSG_EOR, which it did not do before. On the async arm there is little
visible change, because process_rx_list() at recv_end already sets
MSG_EOR for fully copied records.

Could the commit message mention this flag change on the peek path?

> +			continue;

[Severity: High]

This is a pre-existing issue, but should this branch limit the number of
consecutive empty records, like the other readers in this series do?

There is no counter before this continue. TLS_RX_NODATA_LIMIT is defined
after tls_sw_recvmsg(), and only tls_sw_splice_read() and
tls_sw_read_sock() use it:

		if (++nodata >= TLS_RX_NODATA_LIMIT) {
			err = -EAGAIN;
			goto splice_read_end;
		}

The later patch "tls: Return copied data ahead of a run of empty
records" adds a break once decrypted + copied >= target. That patch also
says "A reader that has copied nothing still reads empty records for as
long as they arrive."

Empty records advance neither len nor decrypted. While decrypted + copied
< target, this loop condition stays true:

	while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) {

The only exit from the run is then:

		if (nodata && signal_pending(current)) {

While tls_strp_msg_ready() is true, tls_rx_rec_wait() returns 1 without
sleeping or checking for signals. It also recomputes timeo on every
call. Because decrypted + copied never advances, tls_read_flush_backlog()
does not see progress either, and sk_flush_backlog() keeps sk_lock
owned.

As a result, a peer that streams empty records keeps the reader inside
recvmsg() with the socket lock owned. That defeats MSG_DONTWAIT and
SO_RCVTIMEO, and blocks sendmsg, setsockopt and close on the socket.

Kernel callers cannot use the signal exit. For example, nfsd reaches
this loop through:

svc_tcp_sock_recvmsg()
    sock_recvmsg(sock, msg, MSG_DONTWAIT)
        tls_sw_recvmsg()

The read_sock patch in this series treats the same behaviour as a bug:
"The caller cannot bound the run because read_sock() has not returned".

Would the same TLS_RX_NODATA_LIMIT cap make sense in tls_sw_recvmsg()?

> +		}
> +
>  		if (!darg.zc) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org

Re: [PATCH net-next v5 06/10] tls: Return copied data ahead of a run of empty records

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new early exit in tls_sw_recvmsg() (net/tls/tls_sw.c:1946,
  `if (decrypted + copied >= target) break;`) only fires once the low-…

--

tls: Return copied data ahead of a run of empty records

This patch makes tls_sw_recvmsg() leave its receive loop at a zero-length
application_data record once the receive low-water mark has been met. The
goal is to stop a stream of empty records from holding back data that has
already been copied. A reader that has copied nothing keeps reading empty
records for as long as they arrive.

Link: https://lore.kernel.org/r/179124254665.434549.12245680127262431659@kernel.org

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 153ff135a395e..9b4e66567ee8d 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1939,6 +1939,12 @@ int tls_sw_recvmsg(struct sock *sk,
>  
>  			/* An empty record still marks a boundary. */
>  			msg->msg_flags |= MSG_EOR;
> +
> +			/* Return data already copied instead of holding it
> +			 * through a run of empty records.
> +			 */
> +			if (decrypted + copied >= target)
> +				break;
>  			continue;

[Severity: High]
Does this leave the same stall in place for a reader that is still below
target?

When SO_RCVLOWAT is above the bytes copied so far, or MSG_WAITALL makes
target equal len, the check above is false. The empty record then takes the
continue. An empty record changes neither len nor decrypted, so the loop
condition stays true:

    while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) {

Each pass calls tls_rx_rec_wait(). That function recomputes the timeout on
every call and skips its wait loop while a record is ready:

    timeo = sock_rcvtimeo(sk, nonblock);

    while (!tls_strp_msg_ready(ctx)) {
        ...
        if (!timeo)
            return -EAGAIN;
        ...
    }

As a result, MSG_DONTWAIT, O_NONBLOCK and SO_RCVTIMEO never come into
play. The only way out is this check at the top of the loop:

    if (nodata && signal_pending(current)) {

So if a peer keeps validly encrypted empty application_data records ready,
can a nonblocking or timed reader with partial data stay in the kernel?
It would hold the socket and reader locks and withhold the data it has
already copied.

The commit message describes that outcome as "the reader does not get the
data already copied until the stream stops or a signal is pending". The
same applies to a reader that has copied nothing, which the commit message
mentions.

tls_sw_splice_read() and tls_sw_read_sock() in the same file already stop a
run of empty records with a counter:

    if (++nodata >= TLS_RX_NODATA_LIMIT) {
        err = -EAGAIN;
        goto splice_read_end;
    }

Would a consecutive-empty-record cap like TLS_RX_NODATA_LIMIT in
tls_sw_recvmsg() also cover the below-target and zero-copied readers? It
would also cover the case this patch handles. TLS_RX_NODATA_LIMIT is
currently defined after tls_sw_recvmsg(), so it would need to move up.

>  		}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org