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

[PATCH net 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 one. splice(2) reports the empty
record as EOF and the caller tears down a connection that is still
live. recvmsg(2) neither advances nor returns, so a peer that
streams such records holds the caller in the kernel past SIGKILL
while rx_list grows without bound.

Which fix a reader gets depends on where it returns to. splice and
recvmsg return to userspace and drop the socket lock, so consuming
the record and testing signal_pending() is enough. read_sock runs
from kernel context and holds the lock across the whole call, so it
needs the return boundary a system call would otherwise supply: a
deadline armed by the first record that delivers no bytes and
disarmed by the first that delivers some (patch 1). Scoping that
cap to read_sock alone is deliberate, since a flood on the other
two paths costs the caller only its own scheduler time.

Two user-visible changes follow, both toward what a plain TCP
socket already does. splice(2) on a nonblocking socket, and
sendfile(2) from one, now return -EAGAIN where they used to block.
A splice that reaches a control record behind an empty one now
returns -EINVAL rather than the zero that was the false EOF.

No existing selftest variant reads a zero-length record back any
way but recv(2), so neither the splice path nor MSG_PEEK was
exercised against a record that decrypts to no payload. New
variants cover both.

---
Chuck Lever (9):
      net/tls: Bound time spent on no-data records in tls_sw_read_sock()
      net/tls: Consume empty data records in tls_sw_splice_read()
      net/tls: Fail tls_sw_splice_read() after a failed async decrypt
      net/tls: Honor O_NONBLOCK in tls_sw_splice_read()
      net/tls: Consume empty data records in tls_sw_recvmsg()
      selftests: tls: add peek and splice coverage for zero-length records
      selftests: tls: skip the zero_len tests when TLS is unavailable
      selftests: tls: cover splice on a nonblocking socket
      selftests: tls: cover splice after a failed decrypt

 net/tls/tls_sw.c                  | 113 +++++++++++--
 tools/testing/selftests/net/tls.c | 322 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 414 insertions(+), 21 deletions(-)
---
base-commit: 53658c6f3682967a5e76ed4bc7462c4bdcddaec3
change-id: 20260726-tls-follow-on-486f1ba8bbb0

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

[PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Chuck Lever]

An empty TLS 1.3 data record delivers no payload, so it leaves
tls_sw_read_sock() in its loop without advancing the caller's read
descriptor. A peer that streams such records keeps the receive loop
running, and the socket lock held, for as long as they arrive.

Bound a run of such records, as net_rx_action() bounds a softirq
poll. The first record that delivers no bytes arms a deadline
TLS_RX_NODATA_NS ahead; any record that delivers bytes disarms it,
so a normal stream never trips it. Breaking out with nothing copied
returns zero, which a read_sock consumer reads as "no progress"
rather than EOF, so the connection stays up. Records left queued
draw no fresh sk_data_ready() of their own, so fire the socket's
current callback before returning.

Only tls_sw_read_sock() needs this bound. Its consumers drive the
receive loop from kernel context and hold the socket lock across
the whole call, so the deadline supplies the return boundary a
system call would otherwise provide. tls_sw_splice_read() and
tls_sw_recvmsg() return to userspace and drop the lock, so a flood
there costs the caller only its own scheduler time.

Fixes: 3be28e2c9cd0 ("net/tls: Consume empty data records in tls_sw_read_sock()")
Suggested-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/tls/tls_sw.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index d4afc90fd796..d45c945a3d1d 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -37,6 +37,7 @@
 
 #include <linux/bug.h>
 #include <linux/sched/signal.h>
+#include <linux/timekeeping.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/splice.h>
@@ -2049,6 +2050,11 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	goto splice_read_end;
 }
 
+/* Bound the time that consecutive empty ingress data records keep
+ * the socket lock held without releasing it.
+ */
+#define TLS_RX_NODATA_NS NSEC_PER_MSEC
+
 int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 		     sk_read_actor_t read_actor)
 {
@@ -2057,6 +2063,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;
+	u64 nodata_deadline = 0;
 	struct sk_psock *psock;
 	size_t flushed_at = 0;
 	bool released = true;
@@ -2122,7 +2129,19 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 		 * here instead.
 		 */
 		if (rxm->full_len == 0) {
+			err = 0;
 			consume_skb(skb);
+			if (!nodata_deadline) {
+				nodata_deadline = ktime_get_ns() +
+						  TLS_RX_NODATA_NS;
+			} else if (ktime_get_ns() >= nodata_deadline) {
+				/* Queued records raise no new sk_data_ready(),
+				 * and tls_rx_reader_release() announces only to
+				 * saved_data_ready(), not the consumer's own.
+				 */
+				sk->sk_data_ready(sk);
+				break;
+			}
 			continue;
 		}
 
@@ -2133,6 +2152,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
 			goto read_sock_requeue;
 		}
 		copied += used;
+		nodata_deadline = 0;
 		if (used < rxm->full_len) {
 			rxm->offset += used;
 			rxm->full_len -= used;

-- 
2.54.0

Re: [PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Sabrina Dubroca]

2026-07-26, 20:33:29 -0400, Chuck Lever wrote:
> An empty TLS 1.3 data record delivers no payload, so it leaves
> tls_sw_read_sock() in its loop without advancing the caller's read
> descriptor. A peer that streams such records keeps the receive loop
> running, and the socket lock held, for as long as they arrive.

BTW, should such a peer be considered malicious and
disconnected/blocked? Of course the logic for that should be left to
whatever is calling read_sock, not to TLS itself.

> Bound a run of such records, as net_rx_action() bounds a softirq
> poll. The first record that delivers no bytes arms a deadline
> TLS_RX_NODATA_NS ahead; any record that delivers bytes disarms it,
> so a normal stream never trips it. Breaking out with nothing copied

Another thought here: I think a peer that sends "some" 0-length data
records followed by one (possibly very small) data record, and then
repeats that sequence, will not hit this "rate-limiting" of no-data
records. Is that right? And if so, is that a problem?
[I'm guessing that the caller is doing something equivalent to
recvmsg(WAITALL), so it could be stuck for a long time even with this
new bound]

> returns zero, which a read_sock consumer reads as "no progress"
> rather than EOF, so the connection stays up. Records left queued
> draw no fresh sk_data_ready() of their own, so fire the socket's
> current callback before returning.

[...]
> @@ -2122,7 +2129,19 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
>  		 * here instead.
>  		 */
>  		if (rxm->full_len == 0) {
> +			err = 0;
>  			consume_skb(skb);
> +			if (!nodata_deadline) {
> +				nodata_deadline = ktime_get_ns() +
> +						  TLS_RX_NODATA_NS;
> +			} else if (ktime_get_ns() >= nodata_deadline) {
> +				/* Queued records raise no new sk_data_ready(),
> +				 * and tls_rx_reader_release() announces only to
> +				 * saved_data_ready(), not the consumer's own.

sk->sk_data_ready is tls_data_ready at this point, no? I'm confused by
"the consumer" here.

> +				 */
> +				sk->sk_data_ready(sk);
> +				break;
> +			}
>  			continue;
>  		}

-- 
Sabrina

Re: [PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Chuck Lever]

On Thu, Jul 30, 2026, at 5:12 AM, Sabrina Dubroca wrote:
> 2026-07-26, 20:33:29 -0400, Chuck Lever wrote:

>> Bound a run of such records, as net_rx_action() bounds a softirq
>> poll. The first record that delivers no bytes arms a deadline
>> TLS_RX_NODATA_NS ahead; any record that delivers bytes disarms it,
>> so a normal stream never trips it. Breaking out with nothing copied
>
> Another thought here: I think a peer that sends "some" 0-length data
> records followed by one (possibly very small) data record, and then
> repeats that sequence, will not hit this "rate-limiting" of no-data
> records. Is that right? And if so, is that a problem?

That occurred to me too. It's right on the cusp between still
making progress and gumming things up. Neither the packet-count
limit nor the time-bound address this case.

I don't have a good answer.


> [I'm guessing that the caller is doing something equivalent to
> recvmsg(WAITALL), so it could be stuck for a long time even with this
> new bound]

Still digesting your other feedback. I plan to send a v2 to
address it.


-- 
Chuck Lever

Re: [PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Sabrina Dubroca]

2026-07-30, 09:05:17 -0400, Chuck Lever wrote:
> 
> 
> On Thu, Jul 30, 2026, at 5:12 AM, Sabrina Dubroca wrote:
> > 2026-07-26, 20:33:29 -0400, Chuck Lever wrote:
> 
> >> Bound a run of such records, as net_rx_action() bounds a softirq
> >> poll. The first record that delivers no bytes arms a deadline
> >> TLS_RX_NODATA_NS ahead; any record that delivers bytes disarms it,
> >> so a normal stream never trips it. Breaking out with nothing copied
> >
> > Another thought here: I think a peer that sends "some" 0-length data
> > records followed by one (possibly very small) data record, and then
> > repeats that sequence, will not hit this "rate-limiting" of no-data
> > records. Is that right? And if so, is that a problem?
> 
> That occurred to me too. It's right on the cusp between still
> making progress and gumming things up. Neither the packet-count
> limit nor the time-bound address this case.
> 
> I don't have a good answer.

I'm not sure that can be addressed in a generic way within ktls. Maybe
the caller needs to do its own accounting of "this read_sock/read_actor
dance has been going on for too long now, let's stop".

> > [I'm guessing that the caller is doing something equivalent to
> > recvmsg(WAITALL), so it could be stuck for a long time even with this
> > new bound]
> 
> Still digesting your other feedback. I plan to send a v2 to
> address it.

Thanks. There was also some from sashiko but I couldn't force myself
to read it.

BTW I'm not requesting you to also fix the existing misbehavior I've
mentioned (EOR and timeo).


One small thing I've just noticed in this patch:

>                 if (rxm->full_len == 0) {
> +                       err = 0;
>                         consume_skb(skb);
> +                       if (!nodata_deadline) {
> +                               nodata_deadline = ktime_get_ns() +
> +                                                 TLS_RX_NODATA_NS;
> +                       } else if (ktime_get_ns() >= nodata_deadline) {
> +                               /* Queued records raise no new sk_data_ready(),
> +                                * and tls_rx_reader_release() announces only to
> +                                * saved_data_ready(), not the consumer's own.
> +                                */
> +                               sk->sk_data_ready(sk);
> +                               break;
> +                       }
>                         continue;
>                 }
>  
>                 used = read_actor(desc, skb, rxm->offset, rxm->full_len);
>                 if (used <= 0) {
>                         if (!copied)
>                                 err = used;
>                         goto read_sock_requeue;
>                 }

If we hit the nodata_deadline, we break out of the loop, read_actor
returns 0 since it's an empty record, and we jump to
read_sock_requeue. It doesn't make much sense to requeue the empty
record? Well, we'll start the next read_sock with a record that
immediately arms the deadline, and it avoids a small additional diff
to handle that case in the "used <= 0" block. Nothing serious, I just
thought I'd mention it. I can live with the current change.

-- 
Sabrina

Re: [PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Chuck Lever]

On Thu, Jul 30, 2026, at 1:21 PM, Sabrina Dubroca wrote:
> 2026-07-30, 09:05:17 -0400, Chuck Lever wrote:
>> 
>> 
>> On Thu, Jul 30, 2026, at 5:12 AM, Sabrina Dubroca wrote:
>> > 2026-07-26, 20:33:29 -0400, Chuck Lever wrote:
>> 
>> >> Bound a run of such records, as net_rx_action() bounds a softirq
>> >> poll. The first record that delivers no bytes arms a deadline
>> >> TLS_RX_NODATA_NS ahead; any record that delivers bytes disarms it,
>> >> so a normal stream never trips it. Breaking out with nothing copied
>> >
>> > Another thought here: I think a peer that sends "some" 0-length data
>> > records followed by one (possibly very small) data record, and then
>> > repeats that sequence, will not hit this "rate-limiting" of no-data
>> > records. Is that right? And if so, is that a problem?
>> 
>> That occurred to me too. It's right on the cusp between still
>> making progress and gumming things up. Neither the packet-count
>> limit nor the time-bound address this case.
>> 
>> I don't have a good answer.
>
> I'm not sure that can be addressed in a generic way within ktls. Maybe
> the caller needs to do its own accounting of "this read_sock/read_actor
> dance has been going on for too long now, let's stop".

The consumer can, but only by killing the connection. During a run of
empty records, read_actor is never called and nothing decrements
desc->count, so the consumer never regains control. The socket lock is
held throughout, and __sk_flush_backlog() takes only sk_lock.slock, so
sk_lock.owned stays set. Anything needing lock_sock() blocks behind the
reader, kernel_sock_shutdown() included. The one channel left is a
store to sk->sk_err, which tls_rx_rec_wait() tests first in its loop.
That works, but it is terminal for the connection.

To be non-lethal, the bound has to be inside ktls. v2 of this series
will use a count rather than a deadline, as Jakub requested.


> If we hit the nodata_deadline, we break out of the loop, read_actor
> returns 0 since it's an empty record, and we jump to
> read_sock_requeue.
  
Well, but consume_skb() has already run, and the break leaves the loop
for read_sock_end, which does not requeue. read_actor is never reached
for an empty record.


> sk->sk_data_ready is tls_data_ready at this point, no? I'm confused by
> "the consumer" here.

It depends on which callback was installed first, and unfortunately
two current in-tree read_sock consumers differ in that order.

svcsock (to become a read_sock consumer soon) installs svc_data_ready
at socket setup and the handshake runs later, so ktls sits on top:
sk_data_ready is tls_data_ready and saved_data_ready is svc_data_ready.
The release-time announce already reaches that consumer.

But nvme-tcp is the reverse. It starts TLS in nvme_tcp_alloc_queue() and
installs its own callback afterward in nvme_tcp_start_queue().
saved_data_ready is sock_def_readable and the announce never queues its
io_work.

The explicit poke is for the second case. v2 of this series will rewrite
the comment to state the condition rather than name a function.


-- 
Chuck Lever

Re: [PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Jakub Kicinski]

On Sun, 26 Jul 2026 20:33:29 -0400 Chuck Lever wrote:
> +/* Bound the time that consecutive empty ingress data records keep
> + * the socket lock held without releasing it.
> + */
> +#define TLS_RX_NODATA_NS NSEC_PER_MSEC

A time bound is a bad idea in the kernel, a softirq can easily 
take >1msec and turn 2 consecutive zero length records into
a fatal error for the connection.

Re: [PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Chuck Lever]

On Mon, Aug 3, 2026, at 6:39 PM, Jakub Kicinski wrote:
> On Sun, 26 Jul 2026 20:33:29 -0400 Chuck Lever wrote:
>> +/* Bound the time that consecutive empty ingress data records keep
>> + * the socket lock held without releasing it.
>> + */
>> +#define TLS_RX_NODATA_NS NSEC_PER_MSEC
>
> A time bound is a bad idea in the kernel, a softirq can easily 
> take >1msec and turn 2 consecutive zero length records into
> a fatal error for the connection.

I don’t have a particular dog in this race, but you and Sabrina need
to thumb wrestle to decide which way this needs to go. My original
thought was to use a count of zero-length packets.

-- 
Chuck Lever

Re: [PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Jakub Kicinski]

On Mon, 03 Aug 2026 20:35:54 -0400 Chuck Lever wrote:
> On Mon, Aug 3, 2026, at 6:39 PM, Jakub Kicinski wrote:
> > On Sun, 26 Jul 2026 20:33:29 -0400 Chuck Lever wrote:  
> >> +/* Bound the time that consecutive empty ingress data records keep
> >> + * the socket lock held without releasing it.
> >> + */
> >> +#define TLS_RX_NODATA_NS NSEC_PER_MSEC  
> >
> > A time bound is a bad idea in the kernel, a softirq can easily 
> > take >1msec and turn 2 consecutive zero length records into
> > a fatal error for the connection.  
> 
> I don’t have a particular dog in this race, but you and Sabrina need
> to thumb wrestle to decide which way this needs to go. My original
> thought was to use a count of zero-length packets.

There's a time limit on rx_action processing which is super noise prone
on production systems, hence my preference to avoid any time bounds on
in kernel processing.

And looks like user space libs generally put a numeric cap on the
zero-len record count within a batch?

FWIW I'm not super convinced this is actually an issue for TLS 1.3.
DoS generally means that attacker has some leverage but here attacker
has to encrypt the records for real, so I don't really see this as
a DoS vector. But better safe than sorry, I guess.

[PATCH net 2/9] net/tls: Consume empty data records in tls_sw_splice_read()

[Chuck Lever]

A peer may send a zero-length application_data record. After
decryption such a record has full_len == 0, so tls_sw_splice_read()
splices zero bytes and returns zero. A zero return from a splice
read signals EOF, and the caller tears down a connection that is
still live.

Consume such a record and fetch the next one, as tls_sw_recvmsg()
already does. A peer that streams empty records can hold the
splicing task in that loop, so test for a pending signal where the
record is consumed.

Where an empty record precedes a close_notify, splice(2) now returns
-EINVAL from the control-record test rather than the zero it
returned before. That zero was the false EOF this patch removes, and
a splice that reaches an alert record has always reported -EINVAL.

The open-coded test in tls_sw_read_sock() becomes
tls_rx_empty_data_rec(); the tls_sw_recvmsg() paths reuse it and
tls_rx_intr_errno() in the patches that follow.

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index d45c945a3d1d..44ae0fad780d 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1788,6 +1788,24 @@ static void tls_rx_reader_unlock(struct sock *sk, struct tls_sw_context_rx *ctx)
 	release_sock(sk);
 }
 
+/* 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).
+ */
+static bool tls_rx_empty_data_rec(int len, unsigned char control)
+{
+	return !len && control == TLS_RECORD_TYPE_DATA;
+}
+
+/* sock_intr_errno() maps the zero timeo of a reader that cannot wait
+ * to -EINTR, but such a reader has no blocking to interrupt. The rest
+ * of the receive side reports that case as -EAGAIN.
+ */
+static int tls_rx_intr_errno(long timeo)
+{
+	return timeo ? sock_intr_errno(timeo) : -EAGAIN;
+}
+
 int tls_sw_recvmsg(struct sock *sk,
 		   struct msghdr *msg,
 		   size_t len,
@@ -1991,6 +2009,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;
 	int chunk;
 	int err;
@@ -1999,13 +2018,14 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	if (err < 0)
 		return err;
 
+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, flags & SPLICE_F_NONBLOCK,
-				      true, false);
+				      released, false);
 		if (err <= 0)
 			goto splice_read_end;
 
@@ -2017,6 +2037,11 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 
 		tls_rx_rec_done(ctx);
 		skb = darg.skb;
+
+		/* The socket lock stays held to the retry, so the
+		 * anchor this wait loaded survives it.
+		 */
+		released = false;
 	}
 
 	rxm = strp_msg(skb);
@@ -2028,6 +2053,21 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 		goto splice_requeue;
 	}
 
+	/* Splicing an empty data record delivers zero bytes, which the
+	 * caller reads as EOF. tls_rx_rec_wait() skips its signal check
+	 * while a record is parsed, so test for a signal here.
+	 */
+	if (tls_rx_empty_data_rec(rxm->full_len, tlm->control)) {
+		long timeo = sock_rcvtimeo(sk, flags & SPLICE_F_NONBLOCK);
+
+		consume_skb(skb);
+		if (signal_pending(current)) {
+			err = tls_rx_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)
@@ -2122,13 +2162,12 @@ 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) {
+		if (tls_rx_empty_data_rec(rxm->full_len, tlm->control)) {
 			err = 0;
 			consume_skb(skb);
 			if (!nodata_deadline) {

-- 
2.54.0

Re: [PATCH net 2/9] net/tls: Consume empty data records in tls_sw_splice_read()

[Sabrina Dubroca]

2026-07-26, 20:33:30 -0400, Chuck Lever wrote:
> +/* 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).
> + */
> +static bool tls_rx_empty_data_rec(int len, unsigned char control)
> +{
> +	return !len && control == TLS_RECORD_TYPE_DATA;
> +}

I'm not convinced by this helper. The record type check is redundant
for splice and read_sock, and it doesn't save much for recvmsg.

If we're going to keep it, I'd rather pass it the skb and fetch the
length and record type directly in the helper, since that's anyway
what all callers are passing.

[...]
> @@ -2017,6 +2037,11 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  
>  		tls_rx_rec_done(ctx);
>  		skb = darg.skb;
> +
> +		/* The socket lock stays held to the retry, so the
> +		 * anchor this wait loaded survives it.

I'm quite confused by this comment. Do you mean "We haven't released
the lock, so don't tell tls_rx_rec_wait that we have if we retry" ?

Either way, I don't think we should be leaking mentions of the
"anchor" outside of strp.c. Whatever tls_rx_rec_wait() does with the
"released" argument isn't tls_sw_splice_read()'s business.

> +		 */
> +		released = false;
>  	}
>  
>  	rxm = strp_msg(skb);
> @@ -2028,6 +2053,21 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  		goto splice_requeue;
>  	}
>  
> +	/* Splicing an empty data record delivers zero bytes, which the
> +	 * caller reads as EOF. tls_rx_rec_wait() skips its signal check
> +	 * while a record is parsed, so test for a signal here.
> +	 */
> +	if (tls_rx_empty_data_rec(rxm->full_len, tlm->control)) {
> +		long timeo = sock_rcvtimeo(sk, flags & SPLICE_F_NONBLOCK);
> +
> +		consume_skb(skb);
> +		if (signal_pending(current)) {
> +			err = tls_rx_intr_errno(timeo);
> +			goto splice_read_end;
> +		}
> +		goto retry;

This looping (and the existing one in the other RX handlers) is making
rcvtimeo a bit pointless AFAICT:

 - we apply rcvtimeo to tls_rx_reader_acquire/tls_rx_reader_lock
 - if that worked (maybe consuming almost the full duration), we keep going
 - we apply rcvtimeo (from "0") it tls_rx_rec_wait
 - keep going again, so maybe we've already consumed close to 2*rcvtimeo
 - decrypt does its thing
 - if we're getting a bunch of 0-length records spaced "just right",
   we keep waiting ~rcvtimeo and never stop. with recvmsg(), if we're
   getting some data but not enough to fill the user's buffer, we'll
   also keep going "too long".

Am I reading this wrong?

If not, that behavior doesn't look desirable. At least it doesn't seem
to match the doc for SO_RCVTIMEO:

    If an input or output function blocks for this period of time, and
    data has been sent or received, the return value of that function
    will be the amount of data transferred; if no data has been
    transferred and the timeout has been reached, then -1 is returned
    with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for
    connect(2)) just as if the socket was specified to be nonblocking.


-- 
Sabrina

Re: [PATCH net 2/9] net/tls: Consume empty data records in tls_sw_splice_read()

[Chuck Lever]

On Thu, Jul 30, 2026, at 6:38 AM, Sabrina Dubroca wrote:
> 2026-07-26, 20:33:30 -0400, Chuck Lever wrote:
>> +/* 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).
>> + */
>> +static bool tls_rx_empty_data_rec(int len, unsigned char control)
>> +{
>> +	return !len && control == TLS_RECORD_TYPE_DATA;
>> +}
>
> I'm not convinced by this helper. The record type check is redundant
> for splice and read_sock, and it doesn't save much for recvmsg.

Agreed. v2 of the series will drop it.


> If we're going to keep it, I'd rather pass it the skb and fetch the
> length and record type directly in the helper
  
That doesn't work for recvmsg. The length has to be read before
tls_rx_rec_done(), because on the zero-copy path darg.skb is the
strparser anchor and that call releases it. The type it needs is the
accumulated control, not the skb's. So the three sites will have to
open-code it.


> I don't think we should be leaking mentions of the "anchor" outside of
> strp.c.

I will reword it.


> This looping (and the existing one in the other RX handlers) is making
> rcvtimeo a bit pointless AFAICT [...] Am I reading this wrong?

You are not. rcvtimeo is applied at the reader lock and again in
tls_rx_rec_wait(), and neither bounds the loop as a whole.


-- 
Chuck Lever

[PATCH net 3/9] net/tls: Fail tls_sw_splice_read() after a failed async decrypt

[Chuck Lever]

When an async decrypt fails, tls_decrypt_done() records the error in
ctx->async_wait.err and calls tls_err_abort(), which stores it in
sk_err. tls_sw_recvmsg() and tls_sw_read_sock() each read
async_wait.err once they hold the reader lock and fail the call: a
record that did not authenticate breaks the connection.

tls_sw_splice_read() has no such check, and sk_err does not stand in
for one. tls_rx_rec_wait() tests sk_err only inside the loop it
skips whenever a record is already parsed, and the first reader to
reach sock_error() clears it, while async_wait.err persists. A
splice therefore keeps delivering records on a connection that
recvmsg() and read_sock() refuse to read.

Read async_wait.err in tls_sw_splice_read() as the other two readers
do.

Fixes: f314bfee81b1 ("tls: rx: return the already-copied data on crypto error")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/tls/tls_sw.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 44ae0fad780d..99e9a9aa995c 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2018,6 +2018,11 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	if (err < 0)
 		return err;
 
+	/* If crypto failed the connection is broken */
+	err = ctx->async_wait.err;
+	if (err)
+		goto splice_read_end;
+
 retry:
 	if (!skb_queue_empty(&ctx->rx_list)) {
 		skb = __skb_dequeue(&ctx->rx_list);

-- 
2.54.0

Re: [PATCH net 3/9] net/tls: Fail tls_sw_splice_read() after a failed async decrypt

[Jakub Kicinski]

On Sun, 26 Jul 2026 20:33:31 -0400 Chuck Lever wrote:
> When an async decrypt fails, tls_decrypt_done() records the error in
> ctx->async_wait.err and calls tls_err_abort(), which stores it in
> sk_err. tls_sw_recvmsg() and tls_sw_read_sock() each read
> async_wait.err once they hold the reader lock and fail the call: a
> record that did not authenticate breaks the connection.
> 
> tls_sw_splice_read() has no such check, and sk_err does not stand in
> for one. tls_rx_rec_wait() tests sk_err only inside the loop it
> skips whenever a record is already parsed, and the first reader to
> reach sock_error() clears it, while async_wait.err persists. A
> splice therefore keeps delivering records on a connection that
> recvmsg() and read_sock() refuse to read.
> 
> Read async_wait.err in tls_sw_splice_read() as the other two readers
> do.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

[PATCH net 4/9] net/tls: Honor O_NONBLOCK in tls_sw_splice_read()

[Chuck Lever]

tls_sw_splice_read() currently derives its blocking behavior from
SPLICE_F_NONBLOCK alone; the socket's own O_NONBLOCK is invisible
to it. A splice(2) call without SPLICE_F_NONBLOCK on a nonblocking
socket therefore sleeps in tls_rx_rec_wait() until a record
arrives, where tcp_splice_read() reads sock->file->f_flags and
returns -EAGAIN.

The sleep is reachable through poll. tls_sw_sock_is_readable()
reports a socket readable while any record sits on rx_list,
including a zero-length data record that delivers no bytes to the
pipe. The splice path consumes it and waits for the next one. The
readiness test cannot screen such a record out, since the strparser
announces a record before decryption, when the plaintext length is
not yet known. An event loop that polls, then splices, stalls on
that connection and starves every other one it multiplexes.

Note that a caller that sets O_NONBLOCK and then splices without
SPLICE_F_NONBLOCK, taking that flag to govern only the pipe, now
gets -EAGAIN where it previously blocked. 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 then 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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 99e9a9aa995c..f85d8a639731 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2011,10 +2011,14 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	struct sk_buff *skb;
 	bool released = true;
 	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;
 
@@ -2029,8 +2033,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,
-				      released, false);
+		err = tls_rx_rec_wait(sk, nonblock, released, false);
 		if (err <= 0)
 			goto splice_read_end;
 
@@ -2063,7 +2066,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	 * while a record is parsed, so test for a signal here.
 	 */
 	if (tls_rx_empty_data_rec(rxm->full_len, tlm->control)) {
-		long timeo = sock_rcvtimeo(sk, flags & SPLICE_F_NONBLOCK);
+		long timeo = sock_rcvtimeo(sk, nonblock);
 
 		consume_skb(skb);
 		if (signal_pending(current)) {

-- 
2.54.0

Re: [PATCH net 4/9] net/tls: Honor O_NONBLOCK in tls_sw_splice_read()

[Sabrina Dubroca]

2026-07-26, 20:33:32 -0400, Chuck Lever wrote:
> tls_sw_splice_read() currently derives its blocking behavior from
> SPLICE_F_NONBLOCK alone; the socket's own O_NONBLOCK is invisible
> to it. A splice(2) call without SPLICE_F_NONBLOCK on a nonblocking

Is that needed?

ssize_t do_splice(struct file *in, loff_t *off_in, struct file *out,
		  loff_t *off_out, size_t len, unsigned int flags)
{
[...]
		if ((in->f_flags | out->f_flags) & O_NONBLOCK)
			flags |= SPLICE_F_NONBLOCK;


We're not going through do_splice()?

-- 
Sabrina

Re: [PATCH net 4/9] net/tls: Honor O_NONBLOCK in tls_sw_splice_read()

[Chuck Lever]

On Thu, Jul 30, 2026, at 6:59 AM, Sabrina Dubroca wrote:
> 2026-07-26, 20:33:32 -0400, Chuck Lever wrote:
>> tls_sw_splice_read() currently derives its blocking behavior from
>> SPLICE_F_NONBLOCK alone; the socket's own O_NONBLOCK is invisible
>> to it. A splice(2) call without SPLICE_F_NONBLOCK on a nonblocking
>
> Is that needed?
>
> ssize_t do_splice(struct file *in, loff_t *off_in, struct file *out,
>               loff_t *off_out, size_t len, unsigned int flags)
> {
> [...]
>             if ((in->f_flags | out->f_flags) & O_NONBLOCK)
>                     flags |= SPLICE_F_NONBLOCK;
>
> We're not going through do_splice()?

That line is in the "ipipe && opipe" branch, which never involves a
socket. Splicing from a socket to a pipe takes the "else if (opipe)"
branch, and the test there is out->f_flags, the pipe's. The socket is
in, so its O_NONBLOCK is not folded into SPLICE_F_NONBLOCK on this
path.


-- 
Chuck Lever

[PATCH net 5/9] net/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; RFC 8446, Section 5.1). Such a record decrypts to
full_len == 0, so every arm of the receive loop reaches
"decrypted += chunk" and "len -= chunk" with chunk == 0: len
never reaches zero, and tls_strp_msg_ready() holds the second
loop term true while the peer keeps records arriving. The peek
arm and the async arm also queue each record on rx_list, which
then grows without bound. tls_rx_rec_wait() returns without
waiting whenever a record is already parsed, so its signal check
never runs, and no other test in the loop consults
signal_pending(). A peer streaming empty records therefore holds
the caller in recvmsg(), unresponsive to SIGKILL, until it stops.

Consume an empty data record as soon as the receive loop has it,
before the paths diverge on darg.zc, and test for a pending
signal there. Freeing the record's skb requires its decryption to
have completed, so an empty record is no longer decrypted
asynchronously, and the new branch sets MSG_EOR itself because
the record no longer reaches the assignment at the bottom of the
loop. Bytes already received take precedence over the signal:
they are returned, and the signal is handled when the caller next
enters the kernel.

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index f85d8a639731..0e76b31b1291 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1877,9 +1877,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: the receive loop frees an empty record's skb,
+		 * so its decryption must have completed.
+		 */
 		if (tlm->control == TLS_RECORD_TYPE_DATA)
-			darg.async = ctx->async_capable;
+			darg.async = ctx->async_capable && to_decrypt;
 		else
 			darg.async = false;
 
@@ -1915,6 +1918,27 @@ int tls_sw_recvmsg(struct sock *sk,
 		chunk = rxm->full_len;
 		tls_rx_rec_done(ctx);
 
+		/* An empty record advances neither loop bound, so a flood
+		 * of them can be interrupted only here. On the zero-copy
+		 * path darg.skb is the strparser anchor, already released
+		 * by tls_rx_rec_done().
+		 */
+		if (tls_rx_empty_data_rec(chunk, control)) {
+			long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+
+			if (!darg.zc)
+				consume_skb(darg.skb);
+
+			/* An empty record still marks a boundary. */
+			msg->msg_flags |= MSG_EOR;
+
+			if (signal_pending(current)) {
+				err = tls_rx_intr_errno(timeo);
+				goto recv_end;
+			}
+			continue;
+		}
+
 		if (!darg.zc) {
 			bool partially_consumed = chunk > len;
 			struct sk_buff *skb = darg.skb;

-- 
2.54.0

Re: [PATCH net 5/9] net/tls: Consume empty data records in tls_sw_recvmsg()

[Sabrina Dubroca]

2026-07-26, 20:33:33 -0400, Chuck Lever wrote:
> 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; RFC 8446, Section 5.1). Such a record decrypts to
> full_len == 0, so every arm of the receive loop reaches
> "decrypted += chunk" and "len -= chunk" with chunk == 0: len
> never reaches zero, and tls_strp_msg_ready() holds the second
> loop term true while the peer keeps records arriving. The peek
> arm and the async arm also queue each record on rx_list, which
> then grows without bound. tls_rx_rec_wait() returns without
> waiting whenever a record is already parsed, so its signal check
> never runs

So we should just move the signal check to the top of
tls_rx_rec_wait()'s loop? (just after all the existing error handling
code)

Then we don't need to add custom code everywhere, and we don't need to
do that much special handling for 0-length records.

> and no other test in the loop consults
> signal_pending(). A peer streaming empty records therefore holds
> the caller in recvmsg(), unresponsive to SIGKILL, until it stops.
> 

[...]
> +		/* An empty record advances neither loop bound, so a flood
> +		 * of them can be interrupted only here. On the zero-copy
> +		 * path darg.skb is the strparser anchor, already released
> +		 * by tls_rx_rec_done().
> +		 */
> +		if (tls_rx_empty_data_rec(chunk, control)) {
> +			long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> +
> +			if (!darg.zc)
> +				consume_skb(darg.skb);

I don't see why you need this special handling. Could you explain that?

> +			/* An empty record still marks a boundary. */
> +			msg->msg_flags |= MSG_EOR;

(note: I know that ship has sailed)

I don't think the way ktls handles MSG_EOR on RX makes any sense,
outside of non-DATA records. For non-DATA records, we only ever
process one per call, so it works out.

For DATA records, We set MSG_EOR when we process the end of a record,
and then we process part of the next record. MSG_EOR is still set,
eventhough we're not finishing the read at a record
boundary. Userspace has no way of knowing where that record boundary
was, or how many there may have been.

-- 
Sabrina

Re: [PATCH net 5/9] net/tls: Consume empty data records in tls_sw_recvmsg()

[Chuck Lever]

On Thu, Jul 30, 2026, at 8:40 AM, Sabrina Dubroca wrote:
> 2026-07-26, 20:33:33 -0400, Chuck Lever wrote:
>> 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; RFC 8446, Section 5.1). Such a record decrypts to
>> full_len == 0, so every arm of the receive loop reaches
>> "decrypted += chunk" and "len -= chunk" with chunk == 0: len
>> never reaches zero, and tls_strp_msg_ready() holds the second
>> loop term true while the peer keeps records arriving. The peek
>> arm and the async arm also queue each record on rx_list, which
>> then grows without bound. tls_rx_rec_wait() returns without
>> waiting whenever a record is already parsed, so its signal check
>> never runs
>
> So we should just move the signal check to the top of
> tls_rx_rec_wait()'s loop? (just after all the existing error handling
> code)

It appears that only the signal check can move. sk_err and sk_shutdown
are tested under !tls_strp_msg_ready() so that an already-parsed record
is delivered before an error or EOF is reported. Hoisting those would
let an error preempt deliverable data.

However, hoisting signal_pending() alone is safe, since the record stays
parsed for the next call. It is also just two call sites rather than
everywhere: recvmsg and splice. Let's target that consolidation against
net-next rather than net.


> > +                 if (!darg.zc)
> > +                         consume_skb(darg.skb);
>
> I don't see why you need this special handling. Could you explain that?

On the zero-copy path darg.skb is the strparser anchor, which
tls_rx_rec_done() has already released. Freeing it again would be a
double free. On the other path it is a freshly allocated clear_skb
that nothing else owns.

Or, were you requesting the placement of a documenting comment?


> I don't think the way ktls handles MSG_EOR on RX makes any sense,
> outside of non-DATA records.
  
Agreed.


-- 
Chuck Lever

Re: [PATCH net 5/9] net/tls: Consume empty data records in tls_sw_recvmsg()

[Jakub Kicinski]

On Sun, 26 Jul 2026 20:33:33 -0400 Chuck Lever wrote:
> -		/* 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: the receive loop frees an empty record's skb,
> +		 * so its decryption must have completed.
> +		 */
>  		if (tlm->control == TLS_RECORD_TYPE_DATA)
> -			darg.async = ctx->async_capable;
> +			darg.async = ctx->async_capable && to_decrypt;

I vaguely recall that we depend on all records being async
uniformly within a single recvmsg call

Re: [PATCH net 5/9] net/tls: Consume empty data records in tls_sw_recvmsg()

[Chuck Lever]

On Mon, Aug 3, 2026, at 6:47 PM, Jakub Kicinski wrote:
> On Sun, 26 Jul 2026 20:33:33 -0400 Chuck Lever wrote:
>> -		/* 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: the receive loop frees an empty record's skb,
>> +		 * so its decryption must have completed.
>> +		 */
>>  		if (tlm->control == TLS_RECORD_TYPE_DATA)
>> -			darg.async = ctx->async_capable;
>> +			darg.async = ctx->async_capable && to_decrypt;
>
> I vaguely recall that we depend on all records being async
> uniformly within a single recvmsg call

That holds, but it keys off the accumulated async flag rather than
darg.async, so a synchronous record among async ones is still queued
on rx_list. Empty records return before that block and copy nothing.


-- 
Chuck Lever

[PATCH net 6/9] selftests: tls: add peek and splice 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 its record table already holds
zero-length application_data records. Every variant reads them back
with a plain recv(), so neither the splice path nor MSG_PEEK is
exercised against a record that decrypts to no payload.

Add zero_len_splice. An empty data record splices zero bytes, and a
zero return from splice() means EOF, so a peer that sends one ends a
connection that is still live. Its variants expect the payload's
length when one sits behind a run of empty records, EAGAIN when
nothing does, and EINVAL when a control record does; an unfixed
kernel reports 0 for all three. Add zero_len_peek, which checks that
a peek reaches the payload behind such a run and leaves it in place
for the read that follows. That one reproduces no failure: the
unbounded rx_list growth it accompanies needs a sustained flood that
three fixed-sequence records cannot supply.

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

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index cbdd3ea28b99..089e940ac43d 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2573,6 +2573,219 @@ TEST_F(zero_len, test)
 	}
 };
 
+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_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
+ * rather than the zero return that means EOF.
+ */
+TEST_F(zero_len_peek, test)
+{
+	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);
+		if (ret == -1)
+			EXPECT_EQ(errno, -variant->peek_ret);
+		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);
+
+	/* Peeking left the payload in place for the read that follows */
+	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);
+	if (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);
+}
+
+/* An empty data record splices zero bytes, which a splice caller reads
+ * as EOF. Splicing must skip past such a record to the payload behind
+ * it, and report EAGAIN when a run of them has no payload behind it.
+ * A control record behind the run reports EINVAL, the error splice
+ * already reports for a control record it meets first.
+ */
+TEST_F(zero_len_splice, test)
+{
+	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);
+		if (ret == -1)
+			EXPECT_EQ(errno, -variant->splice_ret);
+	} else {
+		/* Assert: a zero return, which is what an unfixed kernel
+		 * gives here, leaves the pipe empty, and the read below
+		 * would then 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);
+
+		/* Reaching the payload consumed the empty records ahead
+		 * of it rather than leaving them on the receive queue
+		 */
+		ret = recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT);
+		EXPECT_EQ(ret, -1);
+		if (ret == -1)
+			EXPECT_EQ(errno, EAGAIN);
+	}
+
+	close(p[0]);
+	close(p[1]);
+}
+
 FIXTURE(tls_err)
 {
 	int fd, cfd;

-- 
2.54.0

Re: [PATCH net 6/9] selftests: tls: add peek and splice coverage for zero-length records

[Sabrina Dubroca]

2026-07-26, 20:33:34 -0400, Chuck Lever wrote:
> +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;
> +}

nit: those are identical to the existing FIXTURE_SETUP(zero_len) and
the TX part of TEST_F(zero_len, test), so they should also be used
there.

Or since you're reusing similar variants (for both zero_len_peek and
zero_len_splice), maybe just add the new TEST_F chunks as subtests of
the existing zero_len FIXTURE? There's only partial overlap in the
current version, but there should be a set of {recv_ret, peek_ret,
splice_ret} that works for each.


-- 
Sabrina

[PATCH net 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 hands the
ciphertext back verbatim: every one of the eight variants reports
FAIL on a machine that merely lacks TLS, and those failures mask any
real regression in the same run.

Skip the test when the fixture recorded notls, the convention the
other fixtures in this file already follow.

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 089e940ac43d..af47d9a6098c 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2544,6 +2544,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.54.0

[PATCH net 8/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. The
two forms are required to reach the same outcome, so the existing
per-variant expectations carry over unchanged. A kernel that ignores
the socket flag sleeps in the EAGAIN variant until the harness
timeout rather than returning.

Only the flags the two forms pass differ, so the body of the
existing test moves into zero_len_do_splice() and both call it. The
check that a successful splice consumed the empty records ahead of
the payload now covers the nonblocking socket as well.

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

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index af47d9a6098c..7c178541edf4 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2743,23 +2743,24 @@ FIXTURE_TEARDOWN(zero_len_splice)
  * A control record behind the run reports EINVAL, the error splice
  * already reports for a control record it meets first.
  */
-TEST_F(zero_len_splice, test)
+static void
+zero_len_do_splice(struct __test_metadata *_metadata,
+		   struct _test_data_zero_len_splice *self,
+		   const struct _fixture_variant_zero_len_splice *variant,
+		   unsigned int splice_flags)
 {
 	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);
+			     splice_flags);
 		EXPECT_EQ(ret, -1);
 		if (ret == -1)
 			EXPECT_EQ(errno, -variant->splice_ret);
@@ -2769,7 +2770,7 @@ TEST_F(zero_len_splice, test)
 		 * would then 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)
@@ -2789,6 +2790,34 @@ 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 own O_NONBLOCK governs the record wait, as it does on a
+ * plain TCP socket, so a splice that omits SPLICE_F_NONBLOCK reaches
+ * the same outcome as one that sets it. An unfixed kernel derives the
+ * wait from SPLICE_F_NONBLOCK alone and sleeps here until the harness
+ * timeout.
+ */
+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.54.0

[PATCH net 9/9] selftests: tls: cover splice after a failed decrypt

[Chuck Lever]

Nothing in this file splices a socket whose last decrypt failed, so
the check that fails tls_sw_splice_read() on a broken connection can
be removed without a test noticing. Such a splice hands the
application plaintext that recvmsg() and read_sock() already refuse
to return.

Extend the bad_auth pattern: corrupt an authenticated record, confirm
recvmsg() reports EBADMSG, then splice the same socket and require
EBADMSG again. A synchronous decrypt fails again on the still-queued
record, so only an asynchronous decrypt, which needs TLS 1.2 and an
AEAD advertising CRYPTO_ALG_ASYNC, reaches EBADMSG solely through the
recorded-failure check.

bad_auth builds the same corrupted record, so its construction moves
into a helper the two tests share.

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

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 7c178541edf4..8b68dbfcc592 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -24,6 +24,7 @@
 #include "kselftest_harness.h"
 
 #define TLS_PAYLOAD_MAX_LEN 16384
+#define TLS_HDR_LEN 5
 #define SOL_TLS 282
 
 static int fips_enabled;
@@ -2883,28 +2884,85 @@ TEST_F(tls_err, bad_rec)
 	EXPECT_EQ(errno, EAGAIN);
 }
 
+/* Encrypt a record on the TX-only socket pair, corrupt its last
+ * byte, and hand the result to the socket under test. cfd carries a
+ * byte stream, so one recv() can return part of a record: take the
+ * fragment length from the record header and wait for the remainder.
+ */
+static void tls_send_bad_auth(struct __test_metadata *_metadata,
+			      int fd, int cfd, int fd2)
+{
+	char buf[128];
+	int len;
+
+	memrnd(buf, sizeof(buf) / 2);
+	ASSERT_EQ(send(fd, buf, sizeof(buf) / 2, 0), sizeof(buf) / 2);
+
+	ASSERT_EQ(recv(cfd, buf, TLS_HDR_LEN, MSG_WAITALL), TLS_HDR_LEN);
+
+	len = ((unsigned char)buf[3] << 8) | (unsigned char)buf[4];
+	ASSERT_GT(len, 0);
+	ASSERT_LE(len, (int)sizeof(buf) - TLS_HDR_LEN);
+
+	ASSERT_EQ(recv(cfd, buf + TLS_HDR_LEN, len, MSG_WAITALL), len);
+
+	buf[TLS_HDR_LEN + len - 1]++;
+
+	ASSERT_EQ(send(fd2, buf, TLS_HDR_LEN + len, 0), TLS_HDR_LEN + len);
+}
+
 TEST_F(tls_err, bad_auth)
 {
 	char buf[128];
-	int n;
 
 	if (self->notls)
 		SKIP(return, "no TLS support");
 
-	memrnd(buf, sizeof(buf) / 2);
-	EXPECT_EQ(send(self->fd, buf, sizeof(buf) / 2, 0), sizeof(buf) / 2);
-	n = recv(self->cfd, buf, sizeof(buf), 0);
-	EXPECT_GT(n, sizeof(buf) / 2);
+	tls_send_bad_auth(_metadata, self->fd, self->cfd, self->fd2);
 
-	buf[n - 1]++;
-
-	EXPECT_EQ(send(self->fd2, buf, n, 0), n);
 	EXPECT_EQ(recv(self->cfd2, buf, sizeof(buf), 0), -1);
 	EXPECT_EQ(errno, EBADMSG);
 	EXPECT_EQ(recv(self->cfd2, buf, sizeof(buf), 0), -1);
 	EXPECT_EQ(errno, EBADMSG);
 }
 
+/* A record that did not authenticate breaks the connection for every
+ * reader, splice included.
+ *
+ * The two decrypt paths reach that result differently. A synchronous
+ * decrypt leaves the record parsed, so the splice re-runs the decrypt
+ * and fails on the record itself; the ctx->async_wait.err check in
+ * tls_sw_splice_read() is not what stops it. Only an asynchronous
+ * decrypt, which needs a TLS 1.2 socket and an AEAD advertising
+ * CRYPTO_ALG_ASYNC, consumes the record before the failure is
+ * recorded, leaving that check the sole reason the splice fails.
+ */
+TEST_F(tls_err, bad_auth_splice)
+{
+	char buf[128];
+	ssize_t ret;
+	int p[2];
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	tls_send_bad_auth(_metadata, self->fd, self->cfd, self->fd2);
+
+	EXPECT_EQ(recv(self->cfd2, buf, sizeof(buf), 0), -1);
+	EXPECT_EQ(errno, EBADMSG);
+
+	ASSERT_GE(pipe(p), 0);
+
+	ret = splice(self->cfd2, NULL, p[1], NULL, sizeof(buf),
+		     SPLICE_F_NONBLOCK);
+	EXPECT_EQ(ret, -1);
+	if (ret == -1)
+		EXPECT_EQ(errno, EBADMSG);
+
+	close(p[0]);
+	close(p[1]);
+}
+
 TEST_F(tls_err, bad_in_large_read)
 {
 	char txt[3][64];
@@ -3160,7 +3218,6 @@ static size_t parse_tls_records(struct __test_metadata *_metadata,
 {
 	const __u8 *rec = rx_buf;
 	size_t total_plaintext_rx = 0;
-	const __u8 rec_header_len = 5;
 
 	while (rec < rx_buf + rx_len) {
 		__u16 record_payload_len;
@@ -3180,7 +3237,7 @@ static size_t parse_tls_records(struct __test_metadata *_metadata,
 
 		/* Plaintext must not exceed the specified limit */
 		ASSERT_LE(plaintext_len, max_payload_len);
-		rec += rec_header_len + record_payload_len;
+		rec += TLS_HDR_LEN + record_payload_len;
 	}
 
 	return total_plaintext_rx;

-- 
2.54.0

Re: [PATCH net 9/9] selftests: tls: cover splice after a failed decrypt

[Sabrina Dubroca]

2026-07-26, 20:33:37 -0400, Chuck Lever wrote:
> Nothing in this file splices a socket whose last decrypt failed, so
> the check that fails tls_sw_splice_read() on a broken connection can
> be removed without a test noticing. Such a splice hands the
> application plaintext that recvmsg() and read_sock() already refuse
> to return.
> 
> Extend the bad_auth pattern: corrupt an authenticated record, confirm
> recvmsg() reports EBADMSG, then splice the same socket and require
> EBADMSG again. A synchronous decrypt fails again on the still-queued
> record, so only an asynchronous decrypt, which needs TLS 1.2 and an
> AEAD advertising CRYPTO_ALG_ASYNC, reaches EBADMSG solely through the
> recorded-failure check.

async crypto is going away soon, I don't think it's worth mentioning
it. (but first I'm finishing the surgery to get rid of skmsg)

> bad_auth builds the same corrupted record, so its construction moves
> into a helper the two tests share.
> 
> Signed-off-by: Chuck Lever <cel@kernel.org>
> ---
>  tools/testing/selftests/net/tls.c | 77 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 67 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> index 7c178541edf4..8b68dbfcc592 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c
> @@ -24,6 +24,7 @@
>  #include "kselftest_harness.h"
>  
>  #define TLS_PAYLOAD_MAX_LEN 16384
> +#define TLS_HDR_LEN 5
>  #define SOL_TLS 282
>  
>  static int fips_enabled;
> @@ -2883,28 +2884,85 @@ TEST_F(tls_err, bad_rec)
>  	EXPECT_EQ(errno, EAGAIN);
>  }
>  
> +/* Encrypt a record on the TX-only socket pair, corrupt its last
> + * byte, and hand the result to the socket under test. cfd carries a
> + * byte stream, so one recv() can return part of a record: take the
> + * fragment length from the record header and wait for the remainder.
> + */

I'm not a fan of adding a 5-line comment for every 10-line function.

[...]
> +TEST_F(tls_err, bad_auth_splice)
> +{
> +	char buf[128];
> +	ssize_t ret;
> +	int p[2];
> +
> +	if (self->notls)
> +		SKIP(return, "no TLS support");
> +
> +	tls_send_bad_auth(_metadata, self->fd, self->cfd, self->fd2);
> +
> +	EXPECT_EQ(recv(self->cfd2, buf, sizeof(buf), 0), -1);
> +	EXPECT_EQ(errno, EBADMSG);
> +
> +	ASSERT_GE(pipe(p), 0);
> +
> +	ret = splice(self->cfd2, NULL, p[1], NULL, sizeof(buf),
> +		     SPLICE_F_NONBLOCK);
> +	EXPECT_EQ(ret, -1);
> +	if (ret == -1)

nit: The rest of the tls tests just check errno when an error was
expected. If we didn't get -1, the errno check will probably fail too,
but at this point the test has already failed so IMO it's not worth
adding extra conditionals.

> +		EXPECT_EQ(errno, EBADMSG);
> +
> +	close(p[0]);
> +	close(p[1]);
> +}

-- 
Sabrina

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

[Jakub Kicinski]

On Sun, 26 Jul 2026 20:33:28 -0400 Chuck Lever wrote:
>       net/tls: Bound time spent on no-data records in tls_sw_read_sock()
>       net/tls: Consume empty data records in tls_sw_splice_read()
>       net/tls: Fail tls_sw_splice_read() after a failed async decrypt
>       net/tls: Honor O_NONBLOCK in tls_sw_splice_read()
>       net/tls: Consume empty data records in tls_sw_recvmsg()
>       selftests: tls: add peek and splice coverage for zero-length records
>       selftests: tls: skip the zero_len tests when TLS is unavailable
>       selftests: tls: cover splice on a nonblocking socket
>       selftests: tls: cover splice after a failed decrypt

Could we get patches 3 and 9 out as a separate series? 
These look uncontroversial. The zero-length record handling looks
awkward TBH.