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

[sashiko-bot@kernel.org]

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