Re: [PATCH] nvme-tcp: wait for in-flight TLS handshake callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nvme-tcp: stale tls_enabled and tls_pskid in nvme_tcp_start_tls()
--

commit d9acb1a02c55a0e337ba9cf1eb5bf3a75d22cb58
Author: Avi Weiss <thnkslprpt@gmail.com>

nvme-tcp: wait for in-flight TLS handshake callback

When nvme_tcp_start_tls() times out or is interrupted and
tls_handshake_cancel() loses the race against handshake completion, this
commit waits for queue->tls_complete before returning the timeout or signal
error so the queue is not freed while nvme_tcp_tls_done() is still running.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 7eec983c0eade..26a626abef7b2 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1816,7 +1816,8 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
>  		dev_err(nctrl->device,
>  			"queue %d: TLS handshake failed, error %d\n",
>  			qid, ret);
> -		tls_handshake_cancel(queue->sock->sk);
> +		if (!tls_handshake_cancel(queue->sock->sk))
> +			wait_for_completion(&queue->tls_complete);
>  	} else {
>  		if (queue->tls_err) {
>  			dev_err(nctrl->device,

[Severity: Medium]
What happens to queue->tls_enabled and ctrl->ctrl.tls_pskid if
wait_for_completion_interruptible_timeout() in nvme_tcp_start_tls() returns 0
or -ERESTARTSYS concurrently with handshake_complete() invoking
nvme_tcp_tls_done() with status == 0?

When tls_handshake_cancel() returns false and nvme_tcp_start_tls() waits for
queue->tls_complete, a successful nvme_tcp_tls_done() callback modifies the
queue and controller state before signaling completion:

drivers/nvme/host/tcp.c:nvme_tcp_tls_done() {
	...
	tls_key = nvme_tls_key_lookup(pskid);
	if (IS_ERR(tls_key)) {
		...
	} else {
		queue->tls_enabled = true;
		if (qid == 0)
			ctrl->ctrl.tls_pskid = key_serial(tls_key);
		key_put(tls_key);
		queue->tls_err = 0;
	}

out_complete:
	complete(&queue->tls_complete);
}

Because nvme_tcp_start_tls() still returns the negative timeout or signal
error after wait_for_completion(&queue->tls_complete), nvme_tcp_alloc_queue()
jumps to err_init_connect before setting NVME_TCP_Q_ALLOCATED on
queue->flags:

drivers/nvme/host/tcp.c:nvme_tcp_alloc_queue() {
	...
	if (nvme_tcp_tls_configured(nctrl) && pskid) {
		ret = nvme_tcp_start_tls(nctrl, queue, pskid);
		if (ret)
			goto err_init_connect;
	}
	...
	set_bit(NVME_TCP_Q_ALLOCATED, &queue->flags);
}

Neither err_init_connect nor a subsequent nvme_tcp_alloc_queue() call resets
queue->tls_enabled to false, and nvme_tcp_stop_queue_nowait() returns early
when NVME_TCP_Q_ALLOCATED is not set:

drivers/nvme/host/tcp.c:nvme_tcp_stop_queue_nowait() {
	...
	if (!test_bit(NVME_TCP_Q_ALLOCATED, &queue->flags))
		return;
	...
	/* Stopping the queue will disable TLS */
	queue->tls_enabled = false;
}

If that queue is later connected without TLS (for example, qid == 0 during
the initial unencrypted DH-HMAC-CHAP step of a secure concatenation reset or
reconnect where pskid == 0), nvme_tcp_init_connection() will see
nvme_tcp_queue_tls(queue) == true on an unencrypted TCP socket, call
tls_get_record_type(), and fail with -ENOTCONN. In addition, when
nvme_tcp_alloc_admin_queue() fails during the initial
nvme_tcp_configure_admin_queue() call in nvme_tcp_setup_ctrl(), both
functions return early without calling nvme_tcp_teardown_admin_queue(),
leaving ctrl->tls_pskid populated on a disconnected controller.

When tls_handshake_cancel() returns false and wait_for_completion() finishes,
should nvme_tcp_start_tls() either clean up queue->tls_enabled and
ctrl->ctrl.tls_pskid before returning the error, or use queue->tls_err if not
interrupted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022071934.3818224-1-thnkslprpt@gmail.com?part=1