Re: [PATCH 48/98] nvmet-tcp: rewind the iterator after non-data records

[Bart Van Assche <bvanassche@acm.org>]

On 10/28/26 09:41, Jens Axboe wrote:
>  	case TLS_RECORD_TYPE_ALERT:
> +		iov_iter_revert(&msg->msg_iter, len);
>  		tls_alert_recv(queue->sock->sk, msg, &level, &description);
...
>  		if (queue->tls_pskid) {
>  			ret = nvmet_tcp_tls_record_ok(cmd->queue,
> -					&cmd->recv_msg, cmd->recv_cbuf);
> +					&cmd->recv_msg, cmd->recv_cbuf, len);

Rewinding msg_iter fixes the iterator advance, but TLS_RECORD_TYPE_ALERT
is still broken in two ways here:

1. In nvmet_tcp_try_recv_data(), cmd->recv_msg.msg_iter is an ITER_BVEC
   initialized with iov_iter_bvec(), whereas tls_alert_recv() in
   net/handshake/alert.c unconditionally reads union member
   msg->msg_iter.kvec->iov_base. For an ITER_BVEC, iov->iov_base aliases
   bvec->bv_page (struct page *), so tls_alert_recv() reads the first
   two bytes of struct page instead of the received alert payload and
   ignores fatal alerts whenever byte 0 != TLS_ALERT_LEVEL_FATAL.
2. If queue->left == 1 when a 2-byte TLS alert arrives (or a peer sends
   a 1-byte malformed alert record), sock_recvmsg() returns len == 1,
   and tls_alert_recv() reads data[0] and data[1] without checking that
   2 bytes were received.

To fix this inside nvmet-tcp without cross-tree changes to
net/handshake/alert.c, nvmet_tcp_tls_record_ok() can extract the alert
bytes from a local ITER_SOURCE copy of the rewound msg->msg_iter via
copy_from_iter_full(), pull the second byte from queue->sock when
len == 1 and !(msg->msg_flags & (MSG_EOR | MSG_PEEK)), reject < 2-byte
alerts with -EPROTO, and pass a local 2-byte ITER_KVEC msghdr to
tls_alert_recv() so its tracepoint still works.

Note: patch 51/98 will need a trivial context adjustment around the
nvmet_tcp_tls_alert_recv() call in nvmet_tcp_tls_record_ok() (and
propagating MSG_EOR from rem_msg lets 51/98's nvmet_tcp_tls_drop_rest()
check work unchanged).

Here is a replacement patch (checked with checkpatch.pl and
syntax-checked with Clang; not runtime-tested):

---
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index c38ea3501ea5..e6eadfc35d01 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1173,8 +1173,54 @@ static inline bool nvmet_tcp_pdu_valid(u8 type)
 	return false;
 }
 
+static int nvmet_tcp_tls_alert_recv(struct nvmet_tcp_queue *queue,
+		struct msghdr *msg, int len, u8 *level, u8 *description)
+{
+	struct iov_iter iter = msg->msg_iter;
+	size_t n = min_t(size_t, len, 2);
+	u8 alert[2];
+	struct kvec kvec = {
+		.iov_base = alert,
+		.iov_len = sizeof(alert),
+	};
+	struct msghdr alert_msg = {};
+
+	iter.data_source = ITER_SOURCE;
+	if (!copy_from_iter_full(alert, n, &iter))
+		return -EPROTO;
+
+	if (n < sizeof(alert)) {
+		char cbuf[CMSG_LEN(sizeof(char))] = {};
+		struct kvec rem = {
+			.iov_base = alert + n,
+			.iov_len = sizeof(alert) - n,
+		};
+		struct msghdr rem_msg = {
+			.msg_control = cbuf,
+			.msg_controllen = sizeof(cbuf),
+			.msg_flags = MSG_DONTWAIT,
+		};
+		int ret;
+
+		if (msg->msg_flags & (MSG_EOR | MSG_PEEK))
+			return -EPROTO;
+		ret = kernel_recvmsg(queue->sock, &rem_msg, &rem, 1,
+				     rem.iov_len, rem_msg.msg_flags);
+		if (ret != rem.iov_len ||
+		    tls_get_record_type(queue->sock->sk,
+					(struct cmsghdr *)cbuf) !=
+		    TLS_RECORD_TYPE_ALERT)
+			return -EPROTO;
+		msg->msg_flags |= rem_msg.msg_flags & MSG_EOR;
+	}
+
+	iov_iter_kvec(&alert_msg.msg_iter, ITER_DEST, &kvec, 1, sizeof(alert));
+	tls_alert_recv(queue->sock->sk, &alert_msg, level, description);
+	return 0;
+}
+
 static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
-		struct msghdr *msg, char *cbuf)
+		struct msghdr *msg, char *cbuf, int len)
 {
 	struct cmsghdr *cmsg = (struct cmsghdr *)cbuf;
 	u8 ctype, level, description;
@@ -1187,7 +1233,11 @@ static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
 	case TLS_RECORD_TYPE_DATA:
 		break;
 	case TLS_RECORD_TYPE_ALERT:
-		tls_alert_recv(queue->sock->sk, msg, &level, &description);
+		iov_iter_revert(&msg->msg_iter, len);
+		ret = nvmet_tcp_tls_alert_recv(queue, msg, len, &level,
+					       &description);
+		if (ret)
+			break;
 		if (level == TLS_ALERT_LEVEL_FATAL) {
 			pr_err("queue %d: TLS Alert desc %u\n",
 			       queue->idx, description);
@@ -1200,6 +1250,7 @@ static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
 		break;
 	default:
 		/* discard this record type */
+		iov_iter_revert(&msg->msg_iter, len);
 		pr_err("queue %d: TLS record %d unhandled\n",
 		       queue->idx, ctype);
 		ret = -EAGAIN;
@@ -1228,7 +1279,7 @@ recv:
 	if (unlikely(len < 0))
 		return len;
 	if (queue->tls_pskid) {
-		ret = nvmet_tcp_tls_record_ok(queue, &msg, cbuf);
+		ret = nvmet_tcp_tls_record_ok(queue, &msg, cbuf, len);
 		if (ret < 0)
 			return ret;
 	}
@@ -1290,7 +1341,7 @@ static int nvmet_tcp_try_recv_data(struct nvmet_tcp_queue *queue)
 			return len;
 		if (queue->tls_pskid) {
 			ret = nvmet_tcp_tls_record_ok(cmd->queue,
-					&cmd->recv_msg, cmd->recv_cbuf);
+					&cmd->recv_msg, cmd->recv_cbuf, len);
 			if (ret < 0)
 				return ret;
 		}
@@ -1331,7 +1382,7 @@ static int nvmet_tcp_try_recv_ddgst(struct nvmet_tcp_queue *queue)
 	if (unlikely(len < 0))
 		return len;
 	if (queue->tls_pskid) {
-		ret = nvmet_tcp_tls_record_ok(queue, &msg, cbuf);
+		ret = nvmet_tcp_tls_record_ok(queue, &msg, cbuf, len);
 		if (ret < 0)
 			return ret;
 	}
@@ -1803,7 +1854,7 @@ static int nvmet_tcp_try_peek_pdu(struct nvmet_tcp_queue *queue)
 		return len;
 	}
 
-	ret = nvmet_tcp_tls_record_ok(queue, &msg, cbuf);
+	ret = nvmet_tcp_tls_record_ok(queue, &msg, cbuf, len);
 	if (ret < 0)
 		return ret;