[PATCH 0/2] nvme-tcp: consume TLS records without application data on a live queue

[PATCH 0/2] nvme-tcp: consume TLS records without application data on a live queue

[Martin Belanger via B4 Relay]

After the TLS handshake, a peer may send records that carry no
application data: a NewSessionTicket, a KeyUpdate or an alert.
tls_sw_read_sock() cannot pass such a record to nvme_tcp_recv_skb().
It returns -EINVAL and leaves the record at the head of the receive
queue. Since commit 288cfa187208 ("nvme-tcp: start error recovery when
read_sock fails"), nvme-tcp then starts error recovery, and the
controller reconnects.

TLS 1.3 allows a server to send a NewSessionTicket at any time after
the handshake (RFC 8446, section 4.6.1). Alerts can also arrive at any
time.

Patch 1 moves the TLS record read loop into a helper. No functional
change.

Patch 2 reads the record with recvmsg() when read_sock() returns
-EINVAL, and handles it by type. Handshake messages other than
KeyUpdate are discarded and alerts are logged. A fatal alert, a
KeyUpdate or another record type fails the queue.

This is the follow-up announced in the cover letter of "nvme-tcp:
handle TLS control records at connect and on receive errors":
https://patch.msgid.link/20261027-nvme-tcp-ctrlrec-fixes-v2-0-9656395ebcf2@gmail.com

Both patches are tagged for stable, because 288cfa187208 is being
backported to 6.12, 6.18 and 7.3.

Tool use
========

I developed this series with the help of an LLM coding assistant. It
drafted code, tests and changelogs, and helped with analysis of the
kTLS and sunrpc sources. I reviewed, edited and tested all of it, and
I am responsible for the content.

Testing
=======

Host and target in one VM (virtme-ng), on 7.4-rc2. A test-only change
to net/tls makes the target send records on a live I/O queue during
I/O. Each case writes data and reads it back with O_DIRECT.

  Records sent by the target   7.4-rc2     With this series
  ---------------------------  ----------  --------------------------
  NewSessionTicket, 64 bytes   reconnect   queue stays live
  2 x 300, 2 x 1000, 6 x 64    reconnect   queue stays live
  close_notify alert           reconnect   queue stays live
  warning alert (90)           reconnect   queue stays live, logged
  fatal alert (40)             reconnect   reconnect, logged
  KeyUpdate                    reconnect   reconnect, logged

Data was correct in all cases. Also built with W=1, with and without
CONFIG_NVME_TCP_TLS.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
Martin Belanger (2):
      nvme-tcp: move the TLS record read loop into a helper
      nvme-tcp: consume TLS records without application data

 drivers/nvme/host/tcp.c | 146 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 124 insertions(+), 22 deletions(-)
---
base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
change-id: 20261109-nvme-tcp-live-ctrlrec-4539b9390301

Best regards,
--  
Martin Belanger <martin.nvme.dev@gmail.com>

[PATCH 1/2] nvme-tcp: move the TLS record read loop into a helper

[Martin Belanger via B4 Relay]

From: Martin Belanger <martin.belanger@dell.com>

Move the read loop out of nvme_tcp_skip_tls_record() into a new
helper, nvme_tcp_drain_tls_record(). The next patch uses the helper
to read TLS records that arrive on a live queue.

No functional change.

Cc: stable@vger.kernel.org
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/tcp.c | 60 +++++++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 7ecc6c007f8d..bf71e6f6a6ef 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1623,31 +1623,16 @@ static int nvme_tcp_recv_tls_alert(struct nvme_tcp_queue *queue,
 }
 
 /*
- * Discard a TLS record that arrived in place of the ICResp. A peer may send
- * a NewSessionTicket as soon as the handshake completes.
+ * Read the rest of a TLS record whose first part was read into @msg.
  *
- * Returns 0 when the record was discarded and the ICResp can be read, or a
- * negative errno if the connection cannot continue.
+ * Returns the bytes read, or a negative errno.
  */
-static int nvme_tcp_skip_tls_record(struct nvme_tcp_queue *queue,
-				    struct msghdr *msg, u8 ctype, int len)
+static int nvme_tcp_drain_tls_record(struct nvme_tcp_queue *queue,
+				     struct msghdr *msg, int flags)
 {
 	u8 buf[NVME_TCP_TLS_REC_CHUNK];
-	int ret;
+	int ret, drained = 0;
 
-	switch (ctype) {
-	case TLS_RECORD_TYPE_HANDSHAKE:
-		break;
-	case TLS_RECORD_TYPE_ALERT:
-		ret = nvme_tcp_recv_tls_alert(queue, msg, len);
-		return ret ? : -ENOTCONN;
-	default:
-		pr_err("queue %d: unhandled TLS record %u\n",
-		       nvme_tcp_queue_id(queue), ctype);
-		return -ENOTCONN;
-	}
-
-	/* the ICResp is in a later record, drain this one first */
 	while (!(msg->msg_flags & MSG_EOR)) {
 		struct kvec iov = {
 			.iov_base = buf,
@@ -1661,16 +1646,47 @@ static int nvme_tcp_skip_tls_record(struct nvme_tcp_queue *queue,
 		};
 
 		ret = kernel_recvmsg(queue->sock, &drain, &iov, 1, sizeof(buf),
-				     0);
+				     flags);
 		if (ret <= 0) {
 			pr_warn("queue %d: failed to drain TLS record: %d\n",
 				nvme_tcp_queue_id(queue), ret);
 			return ret ? : -ECONNRESET;
 		}
+		drained += ret;
 		msg->msg_flags = drain.msg_flags;
 	}
 
-	return 0;
+	return drained;
+}
+
+/*
+ * Discard a TLS record that arrived in place of the ICResp. A peer may send
+ * a NewSessionTicket as soon as the handshake completes.
+ *
+ * Returns 0 when the record was discarded and the ICResp can be read, or a
+ * negative errno if the connection cannot continue.
+ */
+static int nvme_tcp_skip_tls_record(struct nvme_tcp_queue *queue,
+				    struct msghdr *msg, u8 ctype, int len)
+{
+	int ret;
+
+	switch (ctype) {
+	case TLS_RECORD_TYPE_HANDSHAKE:
+		break;
+	case TLS_RECORD_TYPE_ALERT:
+		ret = nvme_tcp_recv_tls_alert(queue, msg, len);
+		return ret ? : -ENOTCONN;
+	default:
+		pr_err("queue %d: unhandled TLS record %u\n",
+		       nvme_tcp_queue_id(queue), ctype);
+		return -ENOTCONN;
+	}
+
+	/* the ICResp is in a later record, drain this one first */
+	ret = nvme_tcp_drain_tls_record(queue, msg, 0);
+
+	return ret < 0 ? ret : 0;
 }
 
 /*

-- 
2.53.0

Re: [PATCH 1/2] nvme-tcp: move the TLS record read loop into a helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] nvme-tcp: unaligned cmsghdr access in
  nvme_tcp_init_connection() [1]
--

commit b550db6841afde16fa08cb67f259ffa9462cfbdb
Author: Martin Belanger <martin.belanger@dell.com>

nvme-tcp: move the TLS record read loop into a helper

This commit extracts the TLS record draining loop from
nvme_tcp_skip_tls_record() into a new nvme_tcp_drain_tls_record() helper.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 7ecc6c007f8d4..bf71e6f6a6ef6 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1623,31 +1623,16 @@ static int nvme_tcp_recv_tls_alert(struct nvme_tcp_queue *queue,
>  }
>  
>  /*
> - * Discard a TLS record that arrived in place of the ICResp. A peer may send
> - * a NewSessionTicket as soon as the handshake completes.
> + * Read the rest of a TLS record whose first part was read into @msg.
>   *
> - * Returns 0 when the record was discarded and the ICResp can be read, or a
> - * negative errno if the connection cannot continue.
> + * Returns the bytes read, or a negative errno.
>   */
> -static int nvme_tcp_skip_tls_record(struct nvme_tcp_queue *queue,
> -				    struct msghdr *msg, u8 ctype, int len)
> +static int nvme_tcp_drain_tls_record(struct nvme_tcp_queue *queue,
> +				     struct msghdr *msg, int flags)
>  {
>  	u8 buf[NVME_TCP_TLS_REC_CHUNK];
> -	int ret;
> +	int ret, drained = 0;
>  
> -	switch (ctype) {
> -	case TLS_RECORD_TYPE_HANDSHAKE:
> -		break;
> -	case TLS_RECORD_TYPE_ALERT:
> -		ret = nvme_tcp_recv_tls_alert(queue, msg, len);
> -		return ret ? : -ENOTCONN;
> -	default:
> -		pr_err("queue %d: unhandled TLS record %u\n",
> -		       nvme_tcp_queue_id(queue), ctype);
> -		return -ENOTCONN;
> -	}
> -
> -	/* the ICResp is in a later record, drain this one first */
>  	while (!(msg->msg_flags & MSG_EOR)) {
>  		struct kvec iov = {
>  			.iov_base = buf,
> @@ -1661,16 +1646,47 @@ static int nvme_tcp_skip_tls_record(struct nvme_tcp_queue *queue,
>  		};
>  
>  		ret = kernel_recvmsg(queue->sock, &drain, &iov, 1, sizeof(buf),
> -				     0);
> +				     flags);

[Severity: Medium]
This problem wasn't introduced by this patch, but can passing the unaligned
char cbuf[] array as msg_control [1] cause unaligned struct cmsghdr
accesses?

Both here in nvme_tcp_drain_tls_record() and in its caller
nvme_tcp_recv_icresp() (called from nvme_tcp_init_connection()), cbuf is
declared as a plain char array with 1-byte alignment:

drivers/nvme/host/tcp.c:nvme_tcp_recv_icresp() {
	char cbuf[CMSG_LEN(sizeof(char))];
	...
		if (nvme_tcp_queue_tls(queue)) {
			msg.msg_control = cbuf;
			msg.msg_controllen = sizeof(cbuf);
		}
		msg.msg_flags = MSG_WAITALL;
		ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
				     iov.iov_len, msg.msg_flags);
		if (ret <= 0 || !nvme_tcp_queue_tls(queue))
			return ret;

		ctype = tls_get_record_type(queue->sock->sk,
					    (struct cmsghdr *)cbuf);
}

During kernel_recvmsg(), put_cmsg() casts msg->msg_control to
struct cmsghdr * and writes fields such as cmsg_len directly, and
tls_get_record_type() likewise dereferences cbuf as a struct cmsghdr.  If the
compiler places cbuf at a stack offset not aligned to
__alignof__(struct cmsghdr), these accesses trigger UBSAN alignment warnings
or alignment faults on strict-alignment architectures.

[1] https://sashiko.dev/#/bug/linux-1e37cffb-41dc-487d-9c96-e0b9b82c4e63

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-nvme-tcp-live-ctrlrec-v1-0-47ba61670e65@gmail.com?part=1

Re: [PATCH 1/2] nvme-tcp: move the TLS record read loop into a helper

[Martin Belanger]

On 11/9/26 14:11, sashiko-bot@kernel.org wrote:
> [Severity: Medium]
> This problem wasn't introduced by this patch, but can passing the
unaligned
> char cbuf[] array as msg_control [1] cause unaligned struct cmsghdr
> accesses?

The report is correct. The char cbuf[CMSG_LEN(sizeof(char))] pattern
gives the buffer 1-byte alignment, and put_cmsg() and
tls_get_record_type() access it as a struct cmsghdr. The pattern exists
since commit 2837966ab2a8 ("nvme-tcp: control message handling for
recvmsg()"), and this series copies it into the two new receive paths.

v2 will add a first patch that declares the host buffers as a union
with struct cmsghdr, as sunrpc does in svcsock.c and xprtsock.c. The
new code in this series will use the same declaration. I will wait for
review of v1 before I post v2.

nvmet-tcp uses the same pattern in four places. I will fix it in a
separate patch.

Martin

[PATCH 2/2] nvme-tcp: consume TLS records without application data

[Martin Belanger via B4 Relay]

From: Martin Belanger <martin.belanger@dell.com>

A peer may send a TLS record without application data at any time
after the handshake: a NewSessionTicket, a KeyUpdate or an alert.
tls_sw_read_sock() cannot pass such a record to nvme_tcp_recv_skb().
It returns -EINVAL and leaves the record at the head of the receive
queue. nvme_tcp_try_recv() then starts error recovery, and the
controller reconnects.

TLS 1.3 allows a server to send a NewSessionTicket at any time after
the handshake (RFC 8446, section 4.6.1). Alerts can also arrive at any
time.

On -EINVAL, read the record with recvmsg(), which reports the record
type. Discard handshake messages other than KeyUpdate. Log alerts, and
fail the queue on a fatal alert. Fail the queue on a KeyUpdate, which
nvme-tcp cannot answer, and on any other record type. recvmsg() takes
the socket lock, so release it around the read.

svc_tcp_recvfrom() in sunrpc handles -EINVAL from read_sock() the same
way.

Fixes: be8e82caa685 ("nvme-tcp: enable TLS handshake upcall")
Cc: stable@vger.kernel.org
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/tcp.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index bf71e6f6a6ef..d460273a328a 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1444,6 +1444,8 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
 	return ret;
 }
 
+static int nvme_tcp_recv_tls_record(struct nvme_tcp_queue *queue);
+
 static int nvme_tcp_try_recv(struct nvme_tcp_queue *queue)
 {
 	struct socket *sock = queue->sock;
@@ -1456,6 +1458,18 @@ static int nvme_tcp_try_recv(struct nvme_tcp_queue *queue)
 	lock_sock(sk);
 	queue->nr_cqe = 0;
 	consumed = sock->ops->read_sock(sk, &rd_desc, nvme_tcp_recv_skb);
+	/*
+	 * kTLS cannot pass a TLS record without application data to
+	 * nvme_tcp_recv_skb(). It returns -EINVAL and leaves the record at
+	 * the head of the receive queue. recvmsg() takes the socket lock,
+	 * so read the record without it.
+	 */
+	if (unlikely(consumed == -EINVAL && nvme_tcp_queue_tls(queue) &&
+		     queue->rd_enabled)) {
+		release_sock(sk);
+		consumed = nvme_tcp_recv_tls_record(queue);
+		lock_sock(sk);
+	}
 	/*
 	 * nvme_tcp_recv_skb() handles its own errors, so a queue that is
 	 * still enabled failed inside read_sock(). Handle it before
@@ -1659,6 +1673,78 @@ static int nvme_tcp_drain_tls_record(struct nvme_tcp_queue *queue,
 	return drained;
 }
 
+/*
+ * Read the TLS record that read_sock() could not pass to
+ * nvme_tcp_recv_skb(). The caller must not hold the socket lock.
+ *
+ * A KeyUpdate needs new keys that nothing here can supply. Other handshake
+ * messages, such as a NewSessionTicket, are discarded.
+ *
+ * Returns the bytes consumed, or a negative errno if the connection cannot
+ * continue.
+ */
+static int nvme_tcp_recv_tls_record(struct nvme_tcp_queue *queue)
+{
+	u8 buf[NVME_TCP_TLS_REC_CHUNK];
+	char cbuf[CMSG_LEN(sizeof(char))] = {};
+	struct kvec iov = {
+		.iov_base = buf,
+		.iov_len = sizeof(buf),
+	};
+	struct msghdr msg = {
+		.msg_control = cbuf,
+		.msg_controllen = sizeof(cbuf),
+	};
+	int len, ret;
+	u8 ctype;
+
+	len = kernel_recvmsg(queue->sock, &msg, &iov, 1, sizeof(buf),
+			     MSG_DONTWAIT);
+	if (len <= 0) {
+		dev_err(queue->ctrl->ctrl.device,
+			"queue %d: failed to read TLS record: %d\n",
+			nvme_tcp_queue_id(queue), len);
+		goto fail;
+	}
+
+	ctype = tls_get_record_type(queue->sock->sk, (struct cmsghdr *)cbuf);
+	switch (ctype) {
+	case TLS_RECORD_TYPE_HANDSHAKE:
+		if (buf[0] == TLS_HANDSHAKE_KEYUPDATE) {
+			dev_err(queue->ctrl->ctrl.device,
+				"queue %d: peer sent a TLS KeyUpdate, which is not supported\n",
+				nvme_tcp_queue_id(queue));
+			return -EKEYEXPIRED;
+		}
+		break;
+	case TLS_RECORD_TYPE_ALERT:
+		ret = nvme_tcp_recv_tls_alert(queue, &msg, len);
+		if (ret)
+			return ret;
+		break;
+	default:
+		dev_err(queue->ctrl->ctrl.device,
+			"queue %d: unexpected TLS record %u\n",
+			nvme_tcp_queue_id(queue), ctype);
+		return -EPROTO;
+	}
+
+	ret = nvme_tcp_drain_tls_record(queue, &msg, MSG_DONTWAIT);
+	if (ret < 0) {
+		len = ret;
+		goto fail;
+	}
+
+	return len + ret;
+
+fail:
+	/* read_sock() left a whole record, so none of it can be missing */
+	if (len == -EAGAIN)
+		return -EPROTO;
+
+	return len ? : -ECONNRESET;
+}
+
 /*
  * Discard a TLS record that arrived in place of the ICResp. A peer may send
  * a NewSessionTicket as soon as the handshake completes.

-- 
2.53.0

Re: [PATCH 2/2] nvme-tcp: consume TLS records without application data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvme-tcp: race in nvme_tcp_try_recv() when dropping socket
  lock for TLS
--

commit 4ab07d4aae9edb0e5cd1d1957a5ff53b1dd8bb2d
Author: Martin Belanger <martin.belanger@dell.com>

nvme-tcp: consume TLS records without application data

This commit handles -EINVAL returns from read_sock() on TLS-enabled queues
by dropping the socket lock and reading non-application-data TLS records via
kernel_recvmsg().

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index bf71e6f6a6ef6..d460273a328a1 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1444,6 +1444,8 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
>  	return ret;
>  }
>  
> +static int nvme_tcp_recv_tls_record(struct nvme_tcp_queue *queue);
> +
>  static int nvme_tcp_try_recv(struct nvme_tcp_queue *queue)
>  {
>  	struct socket *sock = queue->sock;
> @@ -1456,6 +1458,18 @@ static int nvme_tcp_try_recv(struct nvme_tcp_queue *queue)
>  	lock_sock(sk);
>  	queue->nr_cqe = 0;
>  	consumed = sock->ops->read_sock(sk, &rd_desc, nvme_tcp_recv_skb);
> +	/*
> +	 * kTLS cannot pass a TLS record without application data to
> +	 * nvme_tcp_recv_skb(). It returns -EINVAL and leaves the record at
> +	 * the head of the receive queue. recvmsg() takes the socket lock,
> +	 * so read the record without it.
> +	 */
> +	if (unlikely(consumed == -EINVAL && nvme_tcp_queue_tls(queue) &&
> +		     queue->rd_enabled)) {
> +		release_sock(sk);
> +		consumed = nvme_tcp_recv_tls_record(queue);
> +		lock_sock(sk);
> +	}

[Severity: Medium]
Can dropping the socket lock here let concurrent callers of
nvme_tcp_try_recv() race on the same TLS control record?

On a TLS-enabled poll queue, nvme_tcp_queue_request() always schedules
queue->io_work, so nvme_tcp_io_work() and nvme_tcp_poll() (or two polling
threads) can enter nvme_tcp_try_recv() concurrently without a queue-level
lock:

CPU0 (nvme_tcp_io_work)           CPU1 (nvme_tcp_poll)
nvme_tcp_try_recv()               nvme_tcp_try_recv()
  lock_sock(sk)
  read_sock() -> -EINVAL
  release_sock(sk)
                                    lock_sock(sk)
                                    read_sock() -> -EINVAL
                                    release_sock(sk)
  nvme_tcp_recv_tls_record()
    kernel_recvmsg()
                                    nvme_tcp_recv_tls_record()
                                      kernel_recvmsg()
                                      <-- -EAGAIN or reads data record

Once CPU0 consumes the control record, CPU1's kernel_recvmsg() either gets
-EAGAIN (which nvme_tcp_recv_tls_record() turns into -EPROTO) or consumes up
to 256 bytes of a following TLS_RECORD_TYPE_DATA record and fails with
-EPROTO on the unexpected record type.  In both cases nvme_tcp_try_recv()
disables the queue and triggers nvme_tcp_error_recovery().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-nvme-tcp-live-ctrlrec-v1-0-47ba61670e65@gmail.com?part=2

Re: [PATCH 2/2] nvme-tcp: consume TLS records without application data

[Martin Belanger]

On 11/9/26 14:21, sashiko-bot@kernel.org wrote:
> [Severity: Medium]
> Can dropping the socket lock here let concurrent callers of
> nvme_tcp_try_recv() race on the same TLS control record?

The report is correct. On a poll queue, nvme_tcp_io_work() and
nvme_tcp_poll() both call nvme_tcp_try_recv(). This patch releases the
socket lock before recvmsg(), so both callers can see the same record.
The second caller then fails the queue, and the controller reconnects.

Without this patch, every such record leads to a reconnect. So the race
does not make the result worse than today, but it is a bug in this
patch.

v2 will add a queue flag. The caller that gets -EINVAL from read_sock()
sets it with test_and_set_bit() before it releases the socket lock, and
clears it after it takes the lock again. A caller that finds the flag
set returns 0 and leaves the record to the first one. The flag is set
and tested only under the socket lock, so its owner knows the record at
the head of the queue is the one read_sock() reported.

v2 will also include the cmsg buffer fix from the report on patch 1,
and a test with poll queues.

Martin