Re: [PATCH v2] nvme-tcp: fix NULL queue->ctrl dereference on reconnect

[Hannes Reinecke <hare@suse.de>]

On 10/28/26 3:43 AM, Christian Aurich Zanettini Martins wrote:
> Since commit 61b0ed3be81b ("nvme-tcp: parallelize I/O queue allocation
> and startup"), a reconnect that gets more I/O queues than before grows
> the tag set before the new queues are allocated. queue->ctrl is only
> set by nvme_tcp_alloc_queue(), so I/O submitted in between reaches
> nvme_tcp_queue_rq() on a queue that was never allocated and
> dereferences a NULL queue->ctrl:
> 
>    BUG: kernel NULL pointer dereference, address: 00000000000002ac
>    RIP: 0010:nvme_tcp_queue_rq+0x33/0x3b0
> 
> Add nvme_tcp_init_queue() for the queue fields that do not depend on
> the connection and call it for every queue when allocating the
> controller. Such requests are then requeued like on any other queue
> that is not live. send_mutex and io_work move there as well, and the
> helper's mutexes are all destroyed when the controller is freed, like
> queue_lock already is.
> 
> Fixes: 61b0ed3be81b ("nvme-tcp: parallelize I/O queue allocation and startup")
> Suggested-by: Keith Busch <kbusch@kernel.org>
> Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
> ---
> v2:
> - Rebase on Guixin's v4 "nvme-tcp: Validate the queue state under the
>    queue lock in the address attribute" and move its queue_lock init
>    into the helper
> - Also move send_mutex and io_work into the helper, and destroy
>    send_mutex and pf_cache_lock next to queue_lock in
>    nvme_tcp_free_ctrl() (Keith)
> - Left in nvme_tcp_alloc_queue(): req_list and send_list, as resetting
>    them drops entries left over from the previous connection, and the
>    lockdep keys, which are per socket instance since commit 19bdb70c77d3
>    ("nvme-tcp: lockdep: use dynamic lockdep keys per socket instance")
> 
> Based on nvme-7.4 (9afc1b71d963) with "nvme-tcp: fix request PDU size
> when header digest is enabled" and Guixin's v4 on top. The last hunk
> assumes the conflict between those two in nvme_tcp_alloc_ctrl() is
> resolved by keeping both loop bodies and both comments. I can resend on
> top of nvme-7.4 once both are there.
> 
> Reproduced in QEMU with nvmet-tcp over loopback and
> CONFIG_NVME_MULTIPATH=n: connect with attr_qid_max=2, keep direct reads
> running on the CPUs that map to the new hw queues, then raise
> attr_qid_max to 4. Before the patch the host oopsed as above within two
> tries in each of three boots; with v2, 8 of 8 tries went live, with and
> without lockdep. 30 resets under digest I/O, with a reader looping on
> the address attribute, also pass and are clean with lockdep.
> 
>   drivers/nvme/host/tcp.c | 58 ++++++++++++++++++++---------------------
>   1 file changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 4ada1ff120e2..77bc530ae0a6 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1559,7 +1559,6 @@ static void nvme_tcp_free_queue(struct nvme_ctrl *nctrl, int qid)
>   	memalloc_noio_restore(noio_flag);
>   
>   	kfree(queue->pdu);
> -	mutex_destroy(&queue->send_mutex);
>   
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   	lockdep_unregister_key(&queue->nvme_tcp_sk_key);
> @@ -1987,6 +1986,26 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
>   	return ret;
>   }
>   
> +/*
> + * The I/O tag set is allocated, and grown on reconnect, before the queues are
> + * connected, so blk-mq can initialize requests on a queue, and dispatch to it,
> + * before nvme_tcp_alloc_queue() has run for it. Set up the fields that do not
> + * depend on the connection once, when allocating the controller, and keep them
> + * until the controller is freed.
> + */
> +static void nvme_tcp_init_queue(struct nvme_tcp_ctrl *ctrl, int qid)
> +{
> +	struct nvme_tcp_queue *queue = &ctrl->queues[qid];
> +
> +	queue->ctrl = ctrl;
> +	mutex_init(&queue->queue_lock);
> +	mutex_init(&queue->send_mutex);
> +	mutex_init(&queue->pf_cache_lock);
> +	INIT_WORK(&queue->io_work, nvme_tcp_io_work);
> +	queue->hdr_digest = ctrl->ctrl.opts->hdr_digest;
> +	queue->data_digest = ctrl->ctrl.opts->data_digest;
> +}
> +
>   static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>   				key_serial_t pskid)
>   {
> @@ -1995,11 +2014,8 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>   	int ret, rcv_pdu_size;
>   	struct file *sock_file;
>   
> -	queue->ctrl = ctrl;
>   	init_llist_head(&queue->req_list);
>   	INIT_LIST_HEAD(&queue->send_list);
> -	mutex_init(&queue->send_mutex);
> -	INIT_WORK(&queue->io_work, nvme_tcp_io_work);
>   
>   	if (qid > 0)
>   		queue->cmnd_capsule_len = nctrl->ioccsz * 16;
> @@ -2013,14 +2029,12 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>   	if (ret) {
>   		dev_err(nctrl->device,
>   			"failed to create socket: %d\n", ret);
> -		goto err_destroy_mutex;
> +		return ret;
>   	}
>   
>   	sock_file = sock_alloc_file(queue->sock, O_CLOEXEC, NULL);
> -	if (IS_ERR(sock_file)) {
> -		ret = PTR_ERR(sock_file);
> -		goto err_destroy_mutex;
> -	}
> +	if (IS_ERR(sock_file))
> +		return PTR_ERR(sock_file);
>   
>   	sk_net_refcnt_upgrade(queue->sock->sk);
>   
> @@ -2088,8 +2102,6 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>   		}
>   	}
>   
> -	queue->data_digest = nctrl->opts->data_digest;
> -
>   	rcv_pdu_size = sizeof(struct nvme_tcp_rsp_pdu) +
>   			nvme_tcp_hdgst_len(queue);
>   	queue->pdu = kmalloc(rcv_pdu_size, GFP_KERNEL);
> @@ -2136,8 +2148,6 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>   	lockdep_unregister_key(&queue->nvme_tcp_sk_key);
>   	lockdep_unregister_key(&queue->nvme_tcp_slock_key);
>   #endif
> -err_destroy_mutex:
> -	mutex_destroy(&queue->send_mutex);
>   	return ret;
>   }
>   
> @@ -2838,8 +2848,11 @@ static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
>   	nvmf_free_options(nctrl->opts);
>   free_ctrl:
>   	put_net(ctrl->net);
> -	for (i = 0; i < nr_queues; i++)
> +	for (i = 0; i < nr_queues; i++) {
>   		mutex_destroy(&ctrl->queues[i].queue_lock);
> +		mutex_destroy(&ctrl->queues[i].send_mutex);
> +		mutex_destroy(&ctrl->queues[i].pf_cache_lock);
> +	}

Might be an idea to use a new function 'nvme_tcp_exit_queue()'
here for symmetry with 'nvme_tcp_init_queue()'.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich