Re: [PATCH v2 3/3] 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: incomplete receive drain and NVME_TCP_Q_TLS_RECORD
  race in nvme_tcp_try_recv()
--

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

nvme-tcp: consume TLS records without application data

This commit handles -EINVAL returns from sock->ops->read_sock() on TLS
queues by reading non-application-data TLS records via kernel_recvmsg(),
discarding post-handshake records such as NewSessionTicket while failing
the queue on KeyUpdate or fatal alerts.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index c3bf345c4e921..cc6a46f118b25 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
[ ... ]
> @@ -1462,10 +1465,31 @@ 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)) {
> +		/*
> +		 * On a poll queue, nvme_tcp_poll() and nvme_tcp_io_work() can
> +		 * both get here for the same record. Only one of them reads it.
> +		 */
> +		if (test_and_set_bit(NVME_TCP_Q_TLS_RECORD, &queue->flags)) {
> +			consumed = 0;
> +		} else {
> +			release_sock(sk);
> +			consumed = nvme_tcp_recv_tls_record(queue);
> +			lock_sock(sk);
> +			clear_bit(NVME_TCP_Q_TLS_RECORD, &queue->flags);
> +		}
> +	}

[Severity: Medium]
Can nvme_tcp_try_recv() leave subsequent TLS records or NVMe PDUs stranded
on a poll queue after consuming a control record?

When tls_sw_read_sock() hits a non-data record (record 1),
tls_rx_rec_done() calls tls_strp_check_rcv(), which parses the next queued
record (record 2, such as an R2T PDU, completion, or a second
NewSessionTicket) and sets strp->msg_ready.  Before returning -EINVAL,
tls_rx_reader_release() sets strp->msg_announced and calls
nvme_tcp_data_ready(sk).

If nvme_tcp_poll() and nvme_tcp_io_work() race on a TLS poll queue:

CPU0 (nvme_tcp_poll)              CPU1 (nvme_tcp_io_work)
set_bit(NVME_TCP_Q_POLLING, ...)
nvme_tcp_try_recv()
  lock_sock(sk)
  read_sock() -> -EINVAL
    /* announces record 2;
     * NVME_TCP_Q_POLLING skips
     * queueing io_work */
  test_and_set_bit(TLS_RECORD)
  release_sock(sk)
                                  nvme_tcp_try_recv()
                                    lock_sock(sk)
                                    read_sock() -> -EINVAL
                                    test_and_set_bit(TLS_RECORD)
                                    consumed = 0
                                    release_sock(sk)
                                  /* result == 0 -> pending == false,
                                   * io_work exits */
  nvme_tcp_recv_tls_record()
    /* consumes only record 1 */
  lock_sock(sk)
  clear_bit(TLS_RECORD)
  release_sock(sk)
clear_bit(NVME_TCP_Q_POLLING, ...)
/* returns nr_cqe == 0 without
 * retrying read_sock() */

Even without the race, nvme_tcp_poll() calls nvme_tcp_try_recv() once with
NVME_TCP_Q_POLLING set, so consuming record 1 returns nr_cqe == 0 without
re-running read_sock() for record 2.  Likewise, kernel_recvmsg() inside
nvme_tcp_recv_tls_record() drops the socket lock after consuming record 1
while NVME_TCP_Q_TLS_RECORD is still set; if record 2 is a second
NewSessionTicket, CPU1 can enter read_sock() in that window, decrypt
record 2 onto rx_list, see NVME_TCP_Q_TLS_RECORD still set, and return 0.

Because strp->msg_ready and strp->msg_announced are already set, later TCP
data will not trigger nvme_tcp_data_ready(), stalling the queue until I/O
times out.  Looping back to read_sock() after nvme_tcp_recv_tls_record()
succeeds would drain any remaining records under the same receive call.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-nvme-tcp-live-ctrlrec-v2-0-6295202b84de@gmail.com?part=3