[PATCH net-next v4 0/9] net/tls: Receive-path fixes for zero-length data records

[PATCH net-next v4 0/9] 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, 937 tests with
no skips.

---
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 (9):
      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

 net/tls/tls_sw.c                  |  87 +++++++++++--
 tools/testing/selftests/net/tls.c | 262 +++++++++++++++++++++++++++++++++++---
 2 files changed, 322 insertions(+), 27 deletions(-)
---
base-commit: 29f144016f03608abfddcd6ebaeabafd7aeaadd2
change-id: 20260726-tls-follow-on-486f1ba8bbb0

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

[PATCH net-next v4 1/9] 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 is announced by tls_rx_reader_release(), so the consumer
is called again for that record.

Fixes: 3be28e2c9cd0 ("net/tls: Consume empty data records in tls_sw_read_sock()")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/tls/tls_sw.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 312e51270f29..a64244566af4 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2070,6 +2070,8 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	goto splice_read_end;
 }
 
+#define TLS_RX_NODATA_LIMIT	16
+
 int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 		     sk_read_actor_t read_actor)
 {
@@ -2078,6 +2080,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 +2139,19 @@ 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) {
+			err = 0;
 			consume_skb(skb);
+			/* tls_rx_reader_release() announces a record left
+			 * parsed, so the consumer is called again for it.
+			 */
+			if (++nodata >= TLS_RX_NODATA_LIMIT)
+				break;
 			continue;
 		}
 
@@ -2154,6 +2162,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;

-- 
2.55.0

[PATCH net-next v4 2/9] 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 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 a64244566af4..f2062e141512 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 v4 3/9] 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 f2062e141512..f5e329b75976 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2024,10 +2024,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;
 
@@ -2041,8 +2045,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 v4 4/9] 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. The zero_len_splice selftest
reproduces the zero return.

Consume the record and fetch the next one, as tls_sw_recvmsg()
does. Test for a pending signal before each retry, so a peer that
streams empty records cannot make the splicing task unkillable. 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 | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index f5e329b75976..0fea14224cec 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2023,6 +2023,7 @@ 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;
 	bool nonblock;
 	int chunk;
@@ -2040,12 +2041,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;
 
@@ -2057,6 +2059,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);
@@ -2068,6 +2073,20 @@ 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 (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 v4 5/9] 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 0fea14224cec..b7790b17f79c 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 v4 6/9] 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 b7790b17f79c..15a3a6d3f638 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 v4 7/9] 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 v4 8/9] 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 v4 9/9] 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

Re: [PATCH net-next v4 1/9] tls: Bound consecutive no-data records in tls_sw_read_sock()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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 relies on a wakeup that never reaches the nvme-tcp
  host (or KCM), so a pending record is not read and the receive path…
- [Low] Rejected (fixed later in the series, kept here for the validation
  policy).

--

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

This patch limits tls_sw_read_sock() to TLS_RX_NODATA_LIMIT (16)
consecutive zero-length application_data records in one call. When it
hits the limit the loop breaks, and if nothing was copied it returns
zero. The patch counts on tls_rx_reader_release() to announce any record
that is still parsed, so that the consumer calls read_sock again.

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

[ ... ]

> @@ -2136,14 +2139,19 @@ 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) {
> +			err = 0;
>  			consume_skb(skb);
> +			/* tls_rx_reader_release() announces a record left
> +			 * parsed, so the consumer is called again for it.
> +			 */
> +			if (++nodata >= TLS_RX_NODATA_LIMIT)
> +				break;

[Severity: High]
Can this break stall the nvme-tcp receive path?

When the break fires, tls_rx_rec_done()->tls_strp_check_rcv() may have
already parsed the next record, and read_sock returns 0. The commit
message says:

  A record parsed during the call but not yet
  delivered is announced by tls_rx_reader_release(), so the consumer
  is called again for that record.

However, tls_rx_msg_maybe_announce() only calls the callback that was
saved when kTLS RX was armed:

net/tls/tls_sw.c:tls_rx_msg_maybe_announce() {
    ...
	ctx = container_of(strp, struct tls_sw_context_rx, strp);
	ctx->saved_data_ready(strp->sk);
}

net/tls/tls_sw.c:tls_sw_strparser_arm() {
    ...
	rx_ctx->saved_data_ready = sk->sk_data_ready;
	sk->sk_data_ready = tls_data_ready;
    ...
}

nvme-tcp sets up kTLS first, in nvme_tcp_alloc_queue()->nvme_tcp_start_tls().
It installs nvme_tcp_data_ready only later, in
nvme_tcp_start_queue()->nvme_tcp_setup_sock_ops(). That leaves
saved_data_ready as sock_def_readable. sock_def_readable wakes sk_wq
sleepers but never queues queue->io_work.

When read_sock returns 0, nvme_tcp_try_recv() also returns 0, and
nvme_tcp_io_work() does:

		if (!pending || !queue->rd_enabled)
			return;

So io_work is not requeued, and the parsed record waits until unrelated
TCP data arrives. On an idle queue, wouldn't the NVMe command time out
and trigger error recovery and a reconnect?

A target could trigger this by sending 16 or more empty records followed
by a data record. Each empty record is about 22 bytes in TLS 1.3, so all
of them fit in one segment. Before this patch, the continue delivered the
trailing record in the same call.

KCM looks similar. kcm_attach() replaces sk_data_ready with
psock_data_ready after TLS has saved its callback, and do_strp_work()
only retries on -ENOMEM:

	if (strp_read_sock(strp) == -ENOMEM)
		queue_work(strp_wq, &strp->work);

Could this wake the current sk->sk_data_ready, or tell the caller in
some other way that work remains, instead of relying on
saved_data_ready?

[Severity: Low]
At this point in the series, can the same break fire while ctx->rx_list
still holds decrypted records?

tls_sw_recvmsg() with MSG_PEEK puts zero-length data records on rx_list:

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

Suppose 16 or more of these are followed by real data. read_sock
consumes 16 of them, breaks with copied == 0 and returns 0.
tls_rx_msg_maybe_announce() only checks strparser state:

	if (!READ_ONCE(strp->msg_ready) || strp->msg_announced)
		return;

So nothing announces the data left on rx_list. Nothing announces a
strparser record that was already announced before the call either.

This only matters when MSG_PEEK recvmsg and read_sock are used on the
same socket, and no in-tree user does that. The later commit "tls:
Consume empty data records in tls_sw_recvmsg()" in this series also
removes the precondition: it consumes empty data records before anything
is queued on rx_list. So this only applies to the series up to that
commit.

>  			continue;
>  		}
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org

Re: [PATCH net-next v4 2/9] 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: 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] In tls_sw_recvmsg() (net/tls/tls_sw.c), the patch adds one way
  out of a run of zero-length application_data records: `if (nodata &&…

--

tls: Check for a pending signal after an empty record

This patch adds a signal_pending() check to tls_sw_recvmsg(). The check
runs before the next record is fetched whenever the previous record was
empty, so a stream of zero-length application_data records no longer
leaves the caller unresponsive to signals. It also forces empty records
to be decrypted synchronously, so the error that ends such a run is not
overwritten at recv_end.

Closes: https://sashiko.dev/#/patchset/20260720-tcp-read-sock-v2-0-29545d034f3c@kernel.org?part=1

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index a64244566af44..f2062e141512c 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;
> +		}
> +
>  		err = tls_rx_rec_wait(sk, flags & MSG_DONTWAIT,
>  				      released, !!(decrypted + copied));
>  		if (err <= 0)

[Severity: High]
Can callers that never get a signal still be held in tls_sw_recvmsg()
for as long as the peer keeps sending empty records?

An empty record changes neither len nor decrypted, so the loop condition
stays true:

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

tls_rx_rec_done() then parses the next queued record. That means
tls_rx_rec_wait() skips its while (!tls_strp_msg_ready(ctx)) loop and
returns 1 straight away.

The !timeo -EAGAIN exit, the SO_RCVTIMEO sleep and the signal check in
tls_rx_rec_wait() all sit inside that skipped loop.

The commit message says:

    A nonblocking reader gets -EAGAIN rather than -EINTR.

>From the code, though, a MSG_DONTWAIT reader that has copied nothing gets
-EAGAIN here only if a signal happens to be pending. With no signal, the
loop keeps consuming empty records. A blocking reader also never has its
SO_RCVTIMEO enforced.

Some in-kernel callers use MSG_DONTWAIT from contexts that do not take
signals:

    svc_tcp_sock_recvmsg()
        sock_recvmsg(sock, msg, MSG_DONTWAIT)    (nfsd threads)

    nvmet_tcp_try_recv_pdu() / nvmet_tcp_try_recv_data()
        kernel_recvmsg(..., MSG_DONTWAIT)        (nvmet-tcp io_work)

    xs_sock_recvmsg()                            (sunrpc client receive)

For these callers, nodata && signal_pending(current) is never true. While
they are stuck, ctx->reader_present stays set, so every other reader of
the socket waits in tls_rx_reader_acquire(). The peer only needs to
finish a handshake and then stream zero-length application_data records.

The earlier patch in this series, "tls: Bound consecutive no-data records
in tls_sw_read_sock()", limits the same kind of run with
TLS_RX_NODATA_LIMIT, and that limit works whether or not a signal is
pending.

Would a similar count limit in tls_sw_recvmsg() cover the callers that
the signal test misses?

The later patch "tls: Return copied data ahead of a run of empty records"
adds a break once decrypted + copied >= target. Its commit message says
"A reader that has copied nothing still reads empty records for as long
as they arrive", so the case where nothing has been copied still seems
open at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org

Re: [PATCH net-next v4 3/9] 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] The commit message of "tls: Honor O_NONBLOCK in
  tls_sw_splice_read()" describes a poll-then-splice stall and names a…
- [Low] At this commit no selftest covers the new behavior in
  tls_sw_splice_read(): splice(2) without SPLICE_F_NONBLOCK on a TLS…

Pre-existing issues:
- [Medium] At this commit, tls_sw_splice_read() splices a zero-length
  decrypted DATA record as chunk = 0.

--

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. The combined value is
passed to tls_rx_reader_lock() and tls_rx_rec_wait(). As a result,
splice(2) and sendfile(2) on a nonblocking TLS socket return -EAGAIN
instead of sleeping, the same way tcp_splice_read() does.

> 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.

[Severity: Low]
This isn't a bug, but do these paragraphs describe code that exists at
this commit?

At this commit tls_sw_splice_read() has no retry loop. It splices a
zero-length record as zero bytes, consumes it, and returns 0. That means
there is no second wait in tls_rx_rec_wait() that could stall.

Also, tools/testing/selftests/net/tls.c has no zero_len_splice fixture
yet.

Both come later in the series:

- "tls: Consume empty data records in tls_sw_splice_read()" adds the
  retry loop.
- "selftests: tls: Extend coverage for zero-length records" adds
  zero_len_splice.

The current wording ("once ...", "With empty records consumed ...")
already hints at this. Would it be clearer to say that the stall only
appears once the following patch is applied?

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index f2062e141512c..f5e329b75976b 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2024,10 +2024,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);

[Severity: Low]
This isn't a bug, but does any selftest at this commit cover the new
condition? That would be splice(2) without SPLICE_F_NONBLOCK on a TLS
socket that has O_NONBLOCK set.

TEST_F(tls, nonblocking) sets O_NONBLOCK on cfd but only calls recv().
TEST_F(zero_len, test) never calls splice().

Later in the series, "selftests: tls: Cover splice on a nonblocking
socket" adds TEST_F(zero_len_splice, nonblock_socket). That test sets
O_NONBLOCK on cfd and calls zero_len_do_splice() with splice_flags = 0,
which closes this gap.

> +
> +	err = tls_rx_reader_lock(sk, ctx, nonblock);
>  	if (err < 0)
>  		return err;
>  
> @@ -2041,8 +2045,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;
>  

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. What
happens later in tls_sw_splice_read() when the decrypted DATA record has
zero length?

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);
	if (copied < 0)
		goto splice_requeue;

	if (copied < rxm->full_len) {
    ...
	consume_skb(skb);

splice_read_end:
	tls_rx_reader_unlock(sk, ctx);
	return copied ? : err;
}

When rxm->full_len == 0, chunk and copied are both 0. The record is
consumed and the function returns 0.

Will splice(2) and sendfile(2) callers read that 0 as EOF? RFC 5246 and
RFC 8446 both allow empty application_data records, so a remote peer
could send one and make a live connection look closed to the reader.

The next patch in the series, "tls: Consume empty data records in
tls_sw_splice_read()", fixes this by consuming the zero-length record
and jumping back to retry.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org

Re: [PATCH net-next v4 4/9] tls: Consume empty data records in tls_sw_splice_read()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] In tls_sw_splice_read() (net/tls/tls_sw.c), the new `goto
  retry` loop after an empty application_data record calls…
- [Medium] The new `goto retry` loop in tls_sw_splice_read() keeps
  lock_sock() ownership and the TLS reader lock (taken by…

--

tls: Consume empty data records in tls_sw_splice_read()

This commit makes tls_sw_splice_read() consume zero-length
application_data records and loop back for the next record. Before, it
returned zero, which the caller treats as EOF. A signal_pending() check
before each retry keeps the splicing task killable.

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

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index f5e329b75976b..0fea14224cec4 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -2057,6 +2059,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;

[Severity: Medium]
The socket lock is held across the retry. If the peer keeps sending empty
records, how long can this loop keep lock_sock() ownership and the TLS
reader lock?

After each empty record, tls_rx_rec_done() calls tls_strp_check_rcv().
That parses the next record from sk_receive_queue. On the retry,
tls_rx_rec_wait() sees tls_strp_msg_ready() as true and skips its whole
wait loop:

tls_rx_rec_wait() {
    ...
	while (!tls_strp_msg_ready(ctx)) {
		...
		if (sk_flush_backlog(sk))
			released = true;
		...
		ret = sk_wait_event(sk, &timeo,
				    tls_strp_msg_ready(ctx), &wait);
		...
	}
    ...
}

So the loop never reaches sk_flush_backlog() or the lock release inside
sk_wait_event().

Each pass does a synchronous decrypt, with an skb allocation, a
kmalloc() and an AEAD operation, and there is no cond_resched(). The
only scheduling point is the cond_resched() in __release_sock() during
a backlog flush. That flush runs only once the receive queue no longer
holds a complete record.

While this runs, other lock_sock() users of the socket are held off,
such as a tls_sw_sendmsg() thread or setsockopt(). So are TCP timers
deferred to the socket owner. On PREEMPT_NONE kernels the CPU may also
go a long time without rescheduling. The signal_pending() check makes
the task killable, but it does not limit how long the lock is held.

Another commit in the same series, "tls: Bound consecutive no-data
records in tls_sw_read_sock()", fixes the same loop shape in the sibling
reader:

tls_sw_read_sock() {
    ...
			if (++nodata >= TLS_RX_NODATA_LIMIT)
				break;
			continue;
    ...
}

Would something similar help here? One option is a cap on consecutive
empty records that returns -EAGAIN or -EINTR. Another is to release the
socket and call cond_resched() before the goto retry.

>  	}
>  
>  	rxm = strp_msg(skb);
> @@ -2068,6 +2073,20 @@ 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 (signal_pending(current)) {
> +			long timeo = sock_rcvtimeo(sk, nonblock);
> +
> +			err = -EAGAIN;
> +			if (timeo)
> +				err = sock_intr_errno(timeo);
> +			goto splice_read_end;
> +		}
> +		goto retry;

[Severity: Medium]
Can this retry make SO_RCVTIMEO restart with every empty record?

Each goto retry calls tls_rx_rec_wait() again. tls_rx_rec_wait()
computes a new timeout on every call, so time already spent waiting is
lost:

tls_rx_rec_wait() {
    ...
	timeo = sock_rcvtimeo(sk, nonblock);
    ...
}

Suppose the peer sends an empty application_data record a little more
often than every SO_RCVTIMEO interval. Each record wakes sk_wait_event()
and decrypts to full_len == 0. It is then consumed, and the wait starts
over with a full timeout. A blocking splice(2) or sendfile(2) that never
moves any data would then never return -EAGAIN.

A nonblocking caller only gets -EAGAIN from inside the
!tls_strp_msg_ready() loop in tls_rx_rec_wait():

		if (!timeo)
			return -EAGAIN;

What if empty records keep arriving at least as fast as they are
decrypted? Could a SPLICE_F_NONBLOCK or O_NONBLOCK splice then stay in
the kernel until a signal arrives? That looks like the event-loop stall
that "tls: Honor O_NONBLOCK in tls_sw_splice_read()", earlier in the
series, set out to prevent.

tls_sw_recvmsg() also restarts the timeout for each record. Before this
patch, though, splice handled at most one record per call.

The loop has no iteration cap. tls_sw_read_sock() already limits this
case with TLS_RX_NODATA_LIMIT. Would a matching limit make sense in
tls_sw_splice_read()?

> +	}
> +
>  	chunk = min_t(unsigned int, rxm->full_len, len);
>  	copied = skb_splice_bits(skb, sk, rxm->offset, pipe, chunk, flags);
>  	if (copied < 0)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org

Re: [PATCH net-next v4 6/9] 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: 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] This patch fixes only the case where the reader has already
  met the low-water mark.

--

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

This commit makes tls_sw_recvmsg() leave its receive loop at a
zero-length application_data record once the bytes copied so far meet the
receive low-water mark. Before this change, a peer that sent a stream of
empty records after data could keep the reader in the loop. The reader
held the copied data until the stream stopped or a signal was pending.

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 b7790b17f79c6..15a3a6d3f6380 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: Medium]

Can a non-blocking reader still be held here when it has copied some data
but is still below target?

target is computed without regard to MSG_DONTWAIT:

	target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);

So with MSG_DONTWAIT|MSG_WAITALL, or with MSG_DONTWAIT and SO_RCVLOWAT set
above the bytes copied so far, a reader can end up with
0 < decrypted + copied < target.

In that state the new check is false, so the empty record goes to
continue. Empty records advance neither len nor decrypted, so the loop
condition stays true:

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

For a non-blocking reader, the usual way out with its partial data is the
-EAGAIN return in tls_rx_rec_wait():

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

That check only runs when no record is ready. If the peer has an empty
record ready each time, tls_rx_rec_wait() returns 1. The empty record is
then consumed and the loop runs again. Without the empty records, the
same reader would get -EAGAIN right away, and recv_end would return the
copied bytes.

The reader keeps the copied data until the run of empty records stops or
the nodata && signal_pending() check fires. That is the same delay this
patch fixes for readers that have met target.

The commit message says:

    A reader that has copied nothing still reads empty records
    for as long as they arrive.

This reads as if readers that have copied nothing are the only case left.
The subject line also does not hold for the readers described above.

Later patches in this series do not appear to touch net/, so this case
would still be present after the series.

Would it make sense to also break for a non-blocking read that has
already copied some data? For example:

	if (decrypted + copied >= target ||
	    ((flags & MSG_DONTWAIT) && decrypted + copied))
		break;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org

Re: [PATCH net-next v4 8/9] selftests: tls: Extend coverage for zero-length records

[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: TEST_F(zero_len_splice, test) in
  tools/testing/selftests/net/tls.c passes SPLICE_F_NONBLOCK to every…

--

selftests: tls: Extend coverage for zero-length records

This adds two fixtures to tls.c, zero_len_splice and zero_len_peek. They
check splice() and MSG_PEEK against runs of zero-length application_data
records. It also adds a data_0data_data variant to zero_len, which checks
that the first recv() stops at the empty record.

> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> index 0bc43728262e0..8ce5a90329600 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c

[ ... ]

> @@ -2671,6 +2698,177 @@ TEST_F(zero_len, test)

[ ... ]

> +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);

[Severity: Low]
This isn't a bug, but it is a gap in test coverage. Both splice() calls in
TEST_F(zero_len_splice, test) pass SPLICE_F_NONBLOCK, and nothing sets
O_NONBLOCK on self->cfd.

Can this test reach the socket O_NONBLOCK check in tls_sw_splice_read()?

net/tls/tls_sw.c:tls_sw_splice_read() {
    ...
	nonblock = (flags & SPLICE_F_NONBLOCK) ||
		   (sock->file->f_flags & O_NONBLOCK);
    ...
}

The commit "tls: Honor O_NONBLOCK in tls_sw_splice_read()" adds the
sock->file->f_flags check. Its message gives "the zero_len_splice selftest,
run on a nonblocking socket" as the reproducer. In this version, every
splice() call in the test is nonblocking because of SPLICE_F_NONBLOCK alone.

A later patch in the series, "selftests: tls: Cover splice on a nonblocking
socket", moves this body into zero_len_do_splice(), which takes a
splice_flags argument. That patch also adds TEST_F(zero_len_splice,
nonblock_socket), which sets O_NONBLOCK on self->cfd and passes 0 as the
splice flags. The pipe comes from a plain pipe(), so the socket's O_NONBLOCK
is the only thing that makes those calls nonblocking.

So the path is covered by the end of the series. The gap only shows up when
the series is applied up to this patch and no further.

> +		ret = read(p[0], buf, sizeof(buf));
> +		EXPECT_EQ(ret, variant->splice_ret);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org