[PATCH net 0/5] net: tls: break the connection after a burst of empty data records

[PATCH net 0/5] net: tls: break the connection after a burst of empty data records

[Jakub Kicinski]

Deal with overload of zero-length TLS records. This is the next iteration
of Chuck's series. I tried to make it as simple as possible - break the
connection if sender floods us with zero-length records. Consume / discard
those records even on PEEK to avoid infinite rx_list growth.

https://lore.kernel.org/20261019-tls-follow-on-v6-0-ab01c4ecca0c@kernel.org

Chuck Lever (3):
  tls: Consume empty data records in tls_sw_splice_read()
  selftests: tls: Skip the zero_len tests when TLS is unavailable
  selftests: tls: Extend coverage for zero-length records

Jakub Kicinski (2):
  tls: break the connection after a burst of empty data records
  selftests: tls: Cover the cap on consecutive empty records

 include/net/tls.h                 |   1 +
 net/tls/tls_sw.c                  |  77 ++++---
 tools/testing/selftests/net/tls.c | 348 ++++++++++++++++++++++++++++--
 3 files changed, 385 insertions(+), 41 deletions(-)

-- 
2.56.0

[PATCH net 1/5] tls: break the connection after a burst of empty data records

[Jakub Kicinski]

TLS permits zero-length application_data records, and RFC 8446
suggests them for generating cover traffic. Nothing bounds how many of
them a reader processes back to back, though. They advance neither
loop bound in tls_sw_recvmsg() and tls_sw_read_sock(), so a peer
streaming them faster than the reader decrypts them keeps it in the
kernel for as long as they arrive. recvmsg() does not check for signals
while records are ready, and its peek and async paths queue every empty
record on rx_list. read_sock consumers like nvme-tcp hold the socket
lock throughout and never get control back.

Attempt the simplest possible fix, which is to break the connection
after 128 consecutive empty data records without sleeping or returning
to the caller.

The strparser is stopped before it parses the next record, so the reader
gets the error from tls_rx_rec_wait() and no later record is delivered.
Returning to the caller and keeping the connection alive would need a
way to wake read_sock consumers which installed their own sk_data_ready
after the handshake, a lot of code for a peer that is either broken or
malicious.

Consume empty records in recvmsg() rather than queuing them on rx_list,
so that repeated MSG_PEEK cannot pile them up past the per-read count.
Decrypt them synchronously, too. Once a recvmsg() call has decrypted a
record asynchronously, recv_end replaces the error with the result of
process_rx_list(), so a call that saw only empty records returns 0, a
false EOF, instead of the error or -EAGAIN.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260720-tcp-read-sock-v2-0-29545d034f3c@kernel.org?part=1
Closes: https://sashiko.dev/#/patchset/20260630191551.875664-1-cel@kernel.org?part=1
Link: https://lore.kernel.org/all/20261019-tls-follow-on-v6-0-ab01c4ecca0c@kernel.org/
Fixes: c46234ebb4d1 ("tls: RX path for ktls")
Fixes: 3be28e2c9cd0 ("net/tls: Consume empty data records in tls_sw_read_sock()")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/tls.h |  1 +
 net/tls/tls_sw.c  | 29 +++++++++++++++++++++++++----
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index e57bef58851e..0b001b8ab1c3 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -139,6 +139,7 @@ struct tls_sw_context_rx {
 	u8 zc_capable:1;
 	u8 reader_contended:1;
 	bool key_update_pending;
+	u8 empty_run;
 
 	struct tls_strparser strp;
 
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 771e02bff519..adf3b26b8a75 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1146,6 +1146,7 @@ tls_rx_rec_wait(struct sock *sk, bool nonblock, bool released, bool has_copied)
 		if (!timeo)
 			return -EAGAIN;
 
+		ctx->empty_run = 0;
 		released = true;
 		add_wait_queue(sk_sleep(sk), &wait);
 		sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
@@ -1565,6 +1566,7 @@ static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
 			     struct tls_decrypt_arg *darg)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
+	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 	struct tls_prot_info *prot = &tls_ctx->prot_info;
 	struct strp_msg *rxm;
 	int err;
@@ -1582,6 +1584,19 @@ static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
 	rxm->full_len -= prot->overhead_size;
 	tls_advance_record_sn(sk, prot, &tls_ctx->rx);
 
+	/* A peer may send empty data records, e.g. as cover traffic, but
+	 * a burst of them traps the reader in the kernel. Treat more than
+	 * 128 in a row, without the reader sleeping or returning to the
+	 * caller in between, as a protocol violation. Stop the strparser
+	 * before the next record gets parsed, so the reader finds the
+	 * error in tls_rx_rec_wait().
+	 */
+	if (rxm->full_len ||
+	    tls_msg(darg->skb)->control != TLS_RECORD_TYPE_DATA)
+		ctx->empty_run = 0;
+	else if (unlikely(++ctx->empty_run > 128))
+		tls_strp_abort_strp(&ctx->strp, -EBADMSG);
+
 	return tls_check_pending_rekey(sk, tls_ctx, darg->skb);
 }
 
@@ -1761,6 +1776,7 @@ static int tls_rx_reader_acquire(struct sock *sk, struct tls_sw_context_rx *ctx,
 	}
 
 	WRITE_ONCE(ctx->reader_present, 1);
+	ctx->empty_run = 0;
 
 	return 0;
 }
@@ -1874,9 +1890,11 @@ 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 empty,
+		 * recv_end would report an error after empty records as EOF.
+		 */
 		if (tlm->control == TLS_RECORD_TYPE_DATA)
-			darg.async = ctx->async_capable;
+			darg.async = ctx->async_capable && to_decrypt;
 		else
 			darg.async = false;
 
@@ -1918,7 +1936,10 @@ int tls_sw_recvmsg(struct sock *sk,
 
 			DEBUG_NET_WARN_ON_ONCE(darg.skb == ctx->strp.anchor);
 
-			if (async) {
+			/* Consume empty records, even when peeking, so that
+			 * they cannot pile up on rx_list.
+			 */
+			if (async && chunk) {
 				/* TLS 1.2-only, to_decrypt must be text len */
 				chunk = min_t(int, to_decrypt, len);
 				async_copy_bytes += chunk;
@@ -1939,7 +1960,7 @@ int tls_sw_recvmsg(struct sock *sk,
 			if (err < 0)
 				goto put_on_rx_list_err;
 
-			if (is_peek) {
+			if (is_peek && chunk) {
 				peeked += chunk;
 				goto put_on_rx_list;
 			}
-- 
2.56.0

Re: [PATCH net 1/5] tls: break the connection after a burst of empty data records

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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] The empty-record cap triggers on any backlog of empty records,
  not only on a fast burst.
- [Medium] In tls_sw_recvmsg() the MSG_PEEK path now frees zero-length
  records of every content type, not only empty application_data records.
- [Medium] The new cap 'breaks the connection' through
  tls_strp_abort_strp(&ctx->strp, -EBADMSG).

--

tls: break the connection after a burst of empty data records

This patch adds a per-read counter of consecutive empty application_data
records. When the counter passes 128, it stops the strparser with
-EBADMSG. It also makes recvmsg() decrypt empty records synchronously and
consume them, even with MSG_PEEK, instead of queuing them on rx_list.

Link: https://lore.kernel.org/all/20261019-tls-follow-on-v6-0-ab01c4ecca0c@kernel.org/

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 771e02bff5199..adf3b26b8a75f 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -1582,6 +1584,19 @@ static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
>  	rxm->full_len -= prot->overhead_size;
>  	tls_advance_record_sn(sk, prot, &tls_ctx->rx);
>  
> +	/* A peer may send empty data records, e.g. as cover traffic, but
> +	 * a burst of them traps the reader in the kernel. Treat more than
> +	 * 128 in a row, without the reader sleeping or returning to the
> +	 * caller in between, as a protocol violation. Stop the strparser
> +	 * before the next record gets parsed, so the reader finds the
> +	 * error in tls_rx_rec_wait().
> +	 */
> +	if (rxm->full_len ||
> +	    tls_msg(darg->skb)->control != TLS_RECORD_TYPE_DATA)
> +		ctx->empty_run = 0;
> +	else if (unlikely(++ctx->empty_run > 128))
> +		tls_strp_abort_strp(&ctx->strp, -EBADMSG);

[Severity: Medium]
Can this cap also trip on a slow, legitimate sender when the local
application just stops reading for a while?

ctx->empty_run is reset in only two places. One is
tls_rx_reader_acquire(), at the start of each read call. The other is
tls_rx_rec_wait(), just before the reader sleeps:

net/tls/tls_sw.c:tls_rx_rec_wait() {
    ...
		if (!timeo)
			return -EAGAIN;

		ctx->empty_run = 0;
		released = true;
    ...
}

Suppose records pile up in sk_receive_queue while the application is
idle. The next recvmsg() finds every queued record already parsable, so
tls_rx_rec_wait() returns 1 without reaching that reset.
tls_rx_one_record() then counts every queued empty data record within
that single call.

For example, take a TLS 1.3 peer that sends padding-only cover traffic at
one record per second, which RFC 8446 allows. After about two minutes
without a read, more than 128 empty records would be queued. The next
read would get EBADMSG, and the strparser would stay stopped.

The commit message and the new comment describe this as a reaction to "a
burst" from a peer that is "either broken or malicious". Is the idle
reader backlog case meant to be covered as well? Also, no MIB counter is
incremented when this triggers.

[Severity: Medium]
Does this leave the socket in a state where only the first read learns
that the connection is broken?

tls_strp_abort_strp() sets strp->stopped, writes sk->sk_err once and
calls sk_error_report(). It does not set RCV_SHUTDOWN or SOCK_DONE, and
it records no persistent TLS error.

In the burst case nothing has been copied yet, so tls_rx_rec_wait() runs
with has_copied == false and returns sock_error(sk), which clears
sk_err:

	if (sk->sk_err) {
		if (has_copied)
			return -READ_ONCE(sk->sk_err);
		return sock_error(sk);
	}

After that, tls_strp_check_rcv() returns early because strp->stopped is
set, so msg_ready never becomes true again:

net/tls/tls_strp.c:tls_strp_check_rcv() {
	if (unlikely(strp->stopped) || strp->msg_ready)
		return;
    ...
}

A later blocking recvmsg() or splice() would then sleep in
sk_wait_event() on tls_strp_msg_ready(ctx). It would wake only on
SO_RCVTIMEO, a signal, or a FIN/RST from the peer, even if the peer keeps
sending data. A nonblocking reader would get -EAGAIN forever.

tls_sk_poll() also clears EPOLLIN, because rx_list is empty and
tls_strp_msg_ready() is false:

net/tls/tls_main.c:tls_sk_poll() {
    ...
	if ((skb_queue_empty_lockless(&ctx->rx_list) &&
	     !tls_strp_msg_ready(ctx)) ||
	    READ_ONCE(ctx->key_update_pending))
		mask &= ~(EPOLLIN | EPOLLRDNORM);
    ...
}

EPOLLERR also goes away once sk_err has been consumed. Could an
application that reads SO_ERROR on EPOLLERR before calling recv(), or
that retries after EBADMSG, end up hanging?

This seems to differ from the subject line and from this part of the
commit message:

  "The strparser is stopped before it parses the next record, so the
  reader gets the error from tls_rx_rec_wait() and no later record is
  delivered."

The existing read_failure abort in tls_rx_msg_size() has the same
weakness. This patch, though, adds a new trigger that legal records can
reach, and its stated behaviour depends on this mechanism. Does the abort
need a persistent error or EOF state, so that later reads and poll() see
the failure?

> +
>  	return tls_check_pending_rekey(sk, tls_ctx, darg->skb);
>  }
>  

[ ... ]

> @@ -1918,7 +1936,10 @@ int tls_sw_recvmsg(struct sock *sk,
>  
>  			DEBUG_NET_WARN_ON_ONCE(darg.skb == ctx->strp.anchor);
>  
> -			if (async) {
> +			/* Consume empty records, even when peeking, so that
> +			 * they cannot pile up on rx_list.
> +			 */
> +			if (async && chunk) {
>  				/* TLS 1.2-only, to_decrypt must be text len */
>  				chunk = min_t(int, to_decrypt, len);
>  				async_copy_bytes += chunk;
> @@ -1939,7 +1960,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  			if (err < 0)
>  				goto put_on_rx_list_err;
>  
> -			if (is_peek) {
> +			if (is_peek && chunk) {
>  				peeked += chunk;
>  				goto put_on_rx_list;
>  			}

[Severity: Medium]
Does this also consume zero-length records that are not application data
when MSG_PEEK is set?

tls_rx_msg_size() accepts a zero-length non-data record (alert,
handshake, or any other type). In TLS 1.2 that means data_len ==
cipher_overhead. In TLS 1.3 it means the plaintext is only the inner
content type byte.

Such a record can only be the first one in a recvmsg() call. So
darg.async and async are both false, and chunk == rxm->full_len == 0.

Both of the new checks are then false:

	if (async && chunk) {
	...
	if (is_peek && chunk) {

so execution reaches consume_skb(skb) even though the caller passed
MSG_PEEK.

Before this patch, the record went onto rx_list. The next non-peek
recvmsg() then returned it, with its TLS_GET_RECORD_TYPE cmsg, through
process_rx_list(). Now the peek returns the cmsg and frees the record,
and the next real read returns the following record instead.

Non-data records don't seem to be part of the pile-up problem.
tls_rx_one_record() resets empty_run for them, and the loop already
breaks after a non-data record:

		if (control != TLS_RECORD_TYPE_DATA)
			break;

Should the consume path be limited to data records? For example:

	if (is_peek && (chunk || control != TLS_RECORD_TYPE_DATA))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109175836.1246840-1-kuba%40kernel.org

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

[Jakub Kicinski]

From: Chuck Lever <cel@kernel.org>

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

Consume the record and fetch the next one, as tls_sw_read_sock()
does. The cap on consecutive empty records bounds the loop.

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>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Jakub:
 - rewrite as a loop to avoid the non-error-handling goto
---
 net/tls/tls_sw.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index adf3b26b8a75..b05b9e76bd25 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2044,32 +2044,40 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	if (err)
 		goto splice_read_end;
 
-	if (!skb_queue_empty(&ctx->rx_list)) {
-		skb = __skb_dequeue(&ctx->rx_list);
-	} else {
-		struct tls_decrypt_arg darg;
+	for (;;) {
+		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);
-		if (err <= 0)
-			goto splice_read_end;
+			err = tls_rx_rec_wait(sk, nonblock, true, false);
+			if (err <= 0)
+				goto splice_read_end;
 
-		memset(&darg.inargs, 0, sizeof(darg.inargs));
+			memset(&darg.inargs, 0, sizeof(darg.inargs));
 
-		err = tls_rx_one_record(sk, NULL, &darg);
-		if (err < 0)
-			goto splice_read_end;
+			err = tls_rx_one_record(sk, NULL, &darg);
+			if (err < 0)
+				goto splice_read_end;
 
-		tls_rx_rec_done(ctx);
-		skb = darg.skb;
-	}
+			tls_rx_rec_done(ctx);
+			skb = darg.skb;
+		}
 
-	rxm = strp_msg(skb);
-	tlm = tls_msg(skb);
+		rxm = strp_msg(skb);
+		tlm = tls_msg(skb);
 
-	/* splice does not support reading control messages */
-	if (tlm->control != TLS_RECORD_TYPE_DATA) {
-		err = -EINVAL;
-		goto splice_requeue;
+		/* splice does not support reading control messages */
+		if (tlm->control != TLS_RECORD_TYPE_DATA) {
+			err = -EINVAL;
+			goto splice_requeue;
+		}
+
+		/* Splicing zero bytes would read as EOF */
+		if (rxm->full_len)
+			break;
+
+		consume_skb(skb);
 	}
 
 	chunk = min_t(unsigned int, rxm->full_len, len);
-- 
2.56.0

[PATCH net 3/5] selftests: tls: Skip the zero_len tests when TLS is unavailable

[Jakub Kicinski]

From: Chuck Lever <cel@kernel.org>

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>
Signed-off-by: Jakub Kicinski <kuba@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 c4203401c4bf..ca47576220d3 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2670,6 +2670,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.56.0

[PATCH net 4/5] selftests: tls: Extend coverage for zero-length records

[Jakub Kicinski]

From: Chuck Lever <cel@kernel.org>

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 recvmsg() without MSG_PEEK, so neither splice nor
MSG_PEEK is tested against an empty record.

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. An unfixed kernel
returns 0 from splice().

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

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index ca47576220d3..ed7feeac1cc4 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2580,6 +2580,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 gets no keys, since 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 and 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;
@@ -2642,19 +2678,7 @@ FIXTURE_VARIANT_ADD(zero_len, data_0data_0data)
 
 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)
@@ -2673,10 +2697,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;
@@ -2702,6 +2723,172 @@ 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.
+ */
+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);
+		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);
+
+	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 before the read
+		 * below blocks 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.56.0

Re: [PATCH net 4/5] 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] The new MSG_PEEK test, TEST_F(zero_len_peek, test) in
  tools/testing/selftests/net/tls.c, cannot catch a regression of the…

--

selftests: tls: Extend coverage for zero-length records

This commit adds two TLS selftest fixtures, zero_len_peek and
zero_len_splice. Both inject pre-encrypted zero-length application_data
records and check that MSG_PEEK and splice() reach the payload behind
them, or return EAGAIN/EINVAL when there is no payload. It also moves the
zero_len socket setup and record send loop into shared helpers.

> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> index ca47576220d3e..ed7feeac1cc41 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c

[ ... ]

> +/* 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.
> + */
> +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);
> +		EXPECT_EQ(errno, -variant->peek_ret);
> +		return;
> +	}

[Severity: Low]

Can this test catch a regression of the peek change made earlier in this
series by "tls: break the connection after a burst of empty data records"?

That change replaced "if (is_peek) goto put_on_rx_list;" in
tls_sw_recvmsg() with:

net/tls/tls_sw.c:tls_sw_recvmsg() {
    ...
			/* Consume empty records, even when peeking, so that
			 * they cannot pile up on rx_list.
			 */
    ...
			if (is_peek && chunk) {
				peeked += chunk;
				goto put_on_rx_list;
			}
    ...
}

On a sync-crypto kernel with the old behaviour, both variants still seem
to pass.

For 0data_0data_data, the peek queues all three skbs on rx_list, including
the two empty ones, and returns the 11 payload bytes. The next non-peek
recv() goes through process_rx_list(). There, each empty skb has chunk == 0
and rxm->full_len - skip == 0, so it is unlinked and freed, and then the
payload is copied. The last recv() still gets EAGAIN.

For 0data_0data_0data, the three empty skbs stay on ctx->rx_list.
tls_rx_rec_wait() returns -EAGAIN for the nonblocking peek, and the branch
above returns after checking only ret and errno.

tls_sw_sock_is_readable() reports readable while rx_list is non-empty:

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

Would a zero-timeout poll() on cfd that expects 0 after the EAGAIN peek
tell the fixed and unfixed kernels apart?

zero_len_splice already has that check, under the comment "The empty
records were consumed, not left queued". It only runs after a successful
splice, though, and tls_sw_splice_read() frees empty records instead of
requeueing them.

The later patch "selftests: tls: Cover the cap on consecutive empty
records" adds zero_len_run with recv and splice cases but no MSG_PEEK
case. So at the end of the series, it looks like reverting the peek half
of the kernel fix would not make any selftest fail.

> +
> +	ret = recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT | MSG_PEEK);
> +	EXPECT_EQ(ret, variant->peek_ret);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109175836.1246840-1-kuba%40kernel.org

[PATCH net 5/5] selftests: tls: Cover the cap on consecutive empty records

[Jakub Kicinski]

Check through both recv() and splice() that a run of 128 empty data
records is consumed and the data record behind it is delivered, and
that a run of 129 breaks the connection with EBADMSG before the data
record can be read. Two runs of 128 with a read returning EAGAIN in
between must not break the connection, the cap applies to a burst the
reader cannot keep up with, not to the connection as a whole.

kTLS cannot send an empty record. A TLS 1.3 record of type 0 whose
only byte is the data content type decrypts to one, since the receiver
strips the 0 as padding.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/tls.c | 124 ++++++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index ed7feeac1cc4..48409f06a975 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2889,6 +2889,130 @@ TEST_F(zero_len_splice, test)
 	close(p[1]);
 }
 
+FIXTURE(zero_len_run)
+{
+	int fd, cfd;
+	bool notls;
+};
+
+/* Before each of two reads, send n[i] empty records followed by a data
+ * record if data[i] is set. ret[i] is the result of the read.
+ */
+FIXTURE_VARIANT(zero_len_run)
+{
+	unsigned int n[2];
+	bool data[2];
+	int ret[2];
+};
+
+FIXTURE_VARIANT_ADD(zero_len_run, 128)
+{
+	.n = { 128, 0, },
+	.data = { true, false, },
+	.ret = { 4, -EAGAIN, },
+};
+
+/* More than 128 consecutive empty records break the connection */
+FIXTURE_VARIANT_ADD(zero_len_run, 129)
+{
+	.n = { 129, 0, },
+	.data = { true, false, },
+	.ret = { -EBADMSG, -EAGAIN, },
+};
+
+/* The count starts over on every read */
+FIXTURE_VARIANT_ADD(zero_len_run, 128_128)
+{
+	.n = { 128, 128, },
+	.data = { false, true, },
+	.ret = { -EAGAIN, 4, },
+};
+
+FIXTURE_SETUP(zero_len_run)
+{
+	struct tls_crypto_info_keys tls13;
+	int ret;
+
+	tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
+			     &tls13, 0);
+
+	ulp_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
+	if (self->notls)
+		return;
+
+	ret = setsockopt(self->fd, SOL_TLS, TLS_TX, &tls13, tls13.len);
+	ASSERT_EQ(ret, 0);
+	ret = setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls13, tls13.len);
+	ASSERT_EQ(ret, 0);
+}
+
+FIXTURE_TEARDOWN(zero_len_run)
+{
+	close(self->fd);
+	close(self->cfd);
+}
+
+static void zero_len_run_send(struct __test_metadata *_metadata, int fd,
+			      const FIXTURE_VARIANT(zero_len_run) *variant,
+			      int i)
+{
+	/* kTLS cannot send an empty record. Send the data content type as
+	 * the only byte of a record of type 0 instead, the receiver strips
+	 * the 0 as padding and sees an empty data record.
+	 */
+	char data_type = 0x17;
+	unsigned int j;
+
+	for (j = 0; j < variant->n[i]; j++)
+		ASSERT_EQ(tls_send_cmsg(fd, 0, &data_type, 1, 0), 1);
+	if (variant->data[i])
+		ASSERT_EQ(send(fd, "data", 4, 0), 4);
+}
+
+static void zero_len_run_expect(struct __test_metadata *_metadata,
+				ssize_t ret, int expected)
+{
+	EXPECT_EQ(ret, expected >= 0 ? expected : -1);
+	if (expected < 0)
+		EXPECT_EQ(errno, -expected);
+}
+
+TEST_F(zero_len_run, recv)
+{
+	char buf[16];
+	int i;
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	for (i = 0; i < 2; i++) {
+		zero_len_run_send(_metadata, self->fd, variant, i);
+		zero_len_run_expect(_metadata,
+				    recv(self->cfd, buf, sizeof(buf),
+					 MSG_DONTWAIT),
+				    variant->ret[i]);
+	}
+}
+
+TEST_F(zero_len_run, splice)
+{
+	int i, p[2];
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	ASSERT_GE(pipe(p), 0);
+	for (i = 0; i < 2; i++) {
+		zero_len_run_send(_metadata, self->fd, variant, i);
+		zero_len_run_expect(_metadata,
+				    splice(self->cfd, NULL, p[1], NULL, 16,
+					   SPLICE_F_NONBLOCK),
+				    variant->ret[i]);
+	}
+	close(p[0]);
+	close(p[1]);
+}
+
 FIXTURE(tls_err)
 {
 	int fd, cfd;
-- 
2.56.0

Re: [PATCH net 0/5] net: tls: break the connection after a burst of empty data records

[Chuck Lever]

In patch 4/5, zero_len_peek lost the poll() check that v6 had after
the EAGAIN case. Without the poll() check, the 0data_0data_0data
variant passes on a kernel that lacks 1/5 when decryption is
synchronous. The peek queues all three empty records on rx_list and
recv() still returns EAGAIN. With patch 1/5 applied, the peek consumes
the empty records, so poll() reports the socket as not readable.

Restoring

	/* A record left on rx_list keeps the socket readable */
	EXPECT_EQ(poll(&pfd, 1, 0), 0);

after the errno check, along with the pfd declaration, would cover
the MSG_PEEK change in patch 1/5. But I have not run this series. If
the check fails with patch 1/5 applied, I'd like to understand why.

The loop rewrite in patch 2/5 looks fine to me. For patches 1/5 and
5/5:

Reviewed-by: Chuck Lever <cel@kernel.org>


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)