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

[sashiko-bot@kernel.org]

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