[PATCH v2] nvme-tcp: Validate the queue state under the queue lock in the address attribute

[Nilay Shroff]

On 10/27/26 1:07 PM, Guixin Liu wrote:
> Reading the "address" sysfs attribute calls kernel_getsockname() on the
> admin queue socket, which can race with the queue teardown during
> connection error recovery: a reader can observe NVME_TCP_Q_LIVE set,
> then block on the queue lock while nvme_tcp_free_queue() releases the
> socket and clears queue->sock without holding the lock.  Once the
> reader acquires the lock it dereferences the NULL socket pointer.
> 
> Commit 76d54bf20cdc ("nvme-tcp: don't access released socket during
> error recovery") had the queue liveness check inside the lock.
> Commit 782373ba2766 ("nvme: tcp: avoid race between queue_lock lock
> and destroy") moved it back out because the lock itself is destroyed
> in nvme_tcp_free_queue(), so a reader arriving late trips
> mutex_destroy().
> 
> Check the queue liveness inside the queue lock, and clear queue->sock
> under the same lock.  The teardown clears NVME_TCP_Q_LIVE under the
> lock well before the socket is released, so a reader either sees the
> bit cleared or a socket that cannot be released while it holds the
> lock.  To also close the lock lifetime race, the queue locks are now
> owned by the controller: they are initialized with the queue array and
> only destroyed when the controller is freed, which cannot happen while
> a sysfs attribute read is still in progress.  The opts-derived queue
> count is used for both initialization and destruction, as reconnects
> may shrink ctrl->queue_count.
> 
> Fixes: 782373ba2766 ("nvme: tcp: avoid race between queue_lock lock and destroy")
> Cc:stable at vger.kernel.org
> Signed-off-by: Guixin Liu<kanie at linux.alibaba.com>
> ---
>   drivers/nvme/host/tcp.c | 46 +++++++++++++++++++++++++++++------------
>   1 file changed, 33 insertions(+), 13 deletions(-)
> 
[...]
> @@ -2627,6 +2632,10 @@ static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
>   static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
>   {
>   	struct nvme_tcp_ctrl *ctrl = to_tcp_ctrl(nctrl);
> +	/* the same count used to size and initialize the queue array */
> +	unsigned int nr_queues = nctrl->opts->nr_io_queues +
> +		nctrl->opts->nr_write_queues + nctrl->opts->nr_poll_queues + 1;

This looks good. However, since the same expression is used to calculate
nr_queues during both controller allocation and freeing, would it make
sense to define a macro for it and reuse it in both places?

Thanks,
--Nilay