Re: [PATCH 2/2] nvme-tcp: skip TLS control records while waiting for the ICResp

[Hannes Reinecke <hare@suse.de>]

On 10/26/26 7:09 PM, Martin Belanger via B4 Relay wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> A peer may send a NewSessionTicket as soon as the handshake completes,
> ahead of the ICResp. nvme_tcp_init_connection() fails the connection on
> any record that is not application data, so such a target cannot be
> reached at all. An OpenSSL-based target does this by default.
> 
> Discard such a record and read again, up to a fixed number of records.
> Report an alert, and fail the connection for any other record type.
> 
> The record type is now checked before the length. A control record
> shorter than the ICResp was reported as -ECONNRESET, which hid the real
> cause.
> 
> Fixes: 2837966ab2a8 ("nvme-tcp: control message handling for recvmsg()")
> Assisted-by: LLM
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/tcp.c | 157 +++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 135 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index a03476f0f69d..2d361a9e8710 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -55,6 +55,12 @@ MODULE_PARM_DESC(tls_handshake_timeout,
>   		 "nvme TLS handshake timeout in seconds (default 10)");
>   #endif
>   
> +/* read a TLS control record in chunks of this size */
> +#define NVME_TCP_TLS_REC_CHUNK 256
> +
> +/* TLS records read while waiting for the ICResp, the ICResp included */
> +#define NVME_TCP_ICRESP_MAX_RECORDS 8
> +
>   static int nvme_tcp_cpu_queues[NR_CPUS];
>   static DEFINE_SPINLOCK(nvme_tcp_cpu_queues_lock);
>   
> @@ -1548,12 +1554,138 @@ static void nvme_tcp_free_queue(struct nvme_ctrl *nctrl, int qid)
>   	mutex_destroy(&queue->pf_cache_lock);
>   }
>   
> +static int nvme_tcp_recv_tls_alert(struct nvme_tcp_queue *queue,
> +				   struct msghdr *msg, int len)
> +{
> +	struct sock *sk = queue->sock->sk;
> +	u8 level, description;
> +
> +	if (len < 2) {
> +		dev_err(queue->ctrl->ctrl.device,
> +			"queue %d: truncated TLS alert\n",
> +			nvme_tcp_queue_id(queue));
> +		return -ENOTCONN;
> +	}
> +
> +	/* tls_alert_recv() reads through the iterator */
> +	iov_iter_revert(&msg->msg_iter, len);
> +	tls_alert_recv(sk, msg, &level, &description);
> +
> +	if (level == TLS_ALERT_LEVEL_FATAL) {
> +		dev_err(queue->ctrl->ctrl.device,
> +			"queue %d: fatal TLS alert %u\n",
> +			nvme_tcp_queue_id(queue), description);
> +		return -ENOTCONN;
> +	}
> +
> +	/* a peer shutting down normally is not worth a warning */
> +	if (description == TLS_ALERT_DESC_CLOSE_NOTIFY)
> +		dev_dbg(queue->ctrl->ctrl.device,
> +			"queue %d: TLS close_notify\n",
> +			nvme_tcp_queue_id(queue));
> +	else
> +		dev_warn(queue->ctrl->ctrl.device,
> +			 "queue %d: TLS alert %u\n",
> +			 nvme_tcp_queue_id(queue), description);
> +
> +	return 0;
> +}
> +
> +/*
> + * 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)
> +{
> +	u8 buf[NVME_TCP_TLS_REC_CHUNK];
> +	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 */
> +	while (!(msg->msg_flags & MSG_EOR)) {
> +		struct kvec iov = {
> +			.iov_base = buf,
> +			.iov_len = sizeof(buf),
> +		};
> +		struct msghdr drain = {};
> +
> +		ret = kernel_recvmsg(queue->sock, &drain, &iov, 1, sizeof(buf),
> +				     0);
> +		if (ret <= 0) {
> +			pr_warn("queue %d: failed to drain TLS record: %d\n",
> +				nvme_tcp_queue_id(queue), ret);
> +			return ret ? : -ECONNRESET;
> +		}
> +		msg->msg_flags = drain.msg_flags;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Receive the ICResp. Over TLS, skip the records that a peer may send
> + * ahead of it.
> + *
> + * Returns the length received, or a negative errno.
> + */
> +static int nvme_tcp_recv_icresp(struct nvme_tcp_queue *queue,
> +				struct nvme_tcp_icresp_pdu *icresp)
> +{
> +	char cbuf[CMSG_LEN(sizeof(char))];
> +	struct msghdr msg;
> +	struct kvec iov;
> +	u8 ctype;
> +	int i, ret;
> +
> +	for (i = 0; i < NVME_TCP_ICRESP_MAX_RECORDS; i++) {
> +		memset(&msg, 0, sizeof(msg));
> +		memset(cbuf, 0, sizeof(cbuf));
> +		iov.iov_base = icresp;
> +		iov.iov_len = sizeof(*icresp);
> +		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);
> +		if (ctype == TLS_RECORD_TYPE_DATA)
> +			return ret;
> +
> +		ret = nvme_tcp_skip_tls_record(queue, &msg, ctype, ret);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	pr_err("queue %d: too many TLS records before the ICResp\n",
> +	       nvme_tcp_queue_id(queue));
> +	return -EPROTO;
> +}
> +
>   static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
>   {
>   	struct nvme_tcp_icreq_pdu *icreq;
>   	struct nvme_tcp_icresp_pdu *icresp;
> -	char cbuf[CMSG_LEN(sizeof(char))] = {};
> -	u8 ctype;
>   	struct msghdr msg = {};
>   	struct kvec iov;
>   	bool ctrl_hdgst, ctrl_ddgst;
> @@ -1591,16 +1723,7 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
>   		goto free_icresp;
>   	}
>   
> -	memset(&msg, 0, sizeof(msg));
> -	iov.iov_base = icresp;
> -	iov.iov_len = sizeof(*icresp);
> -	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);
> +	ret = nvme_tcp_recv_icresp(queue, icresp);
>   	if (ret >= 0 && ret < sizeof(*icresp))
>   		ret = -ECONNRESET;
>   	if (ret < 0) {
> @@ -1608,16 +1731,6 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
>   			nvme_tcp_queue_id(queue), ret);
>   		goto free_icresp;
>   	}
> -	ret = -ENOTCONN;
> -	if (nvme_tcp_queue_tls(queue)) {
> -		ctype = tls_get_record_type(queue->sock->sk,
> -					    (struct cmsghdr *)cbuf);
> -		if (ctype != TLS_RECORD_TYPE_DATA) {
> -			pr_err("queue %d: unhandled TLS record %d\n",
> -			       nvme_tcp_queue_id(queue), ctype);
> -			goto free_icresp;
> -		}
> -	}
>   	ret = -EINVAL;
>   	if (icresp->hdr.type != nvme_tcp_icresp) {
>   		pr_err("queue %d: bad type returned %d\n",
> 

Thanks for doing this. Turns out that the 'new' TLS RFC 9846 actually 
requires us to drop NewSessionTicket message as we don't do session
resumption.
Note to self: we could check to (ab-)use the NewSessionTicket mechanism
for I/O queues when creating the connection; might be interesting.
Anyway.

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich