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

[Guixin Liu]

在 2026/10/27 00:09, Nilay Shroff 写道:
> On 10/26/26 4:25 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.
>>
>> 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 | 43 ++++++++++++++++++++++++++++-------------
>>   1 file changed, 30 insertions(+), 13 deletions(-)
>>
>
> [...]
>
>> @@ -2627,6 +2632,7 @@ 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);
>> +    int i;
>>         if (list_empty_careful(&ctrl->list))
>>           goto free_ctrl;
>> @@ -2637,6 +2643,8 @@ static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
>>         nvmf_free_options(nctrl->opts);
>>   free_ctrl:
>> +    for (i = 0; i < ctrl->ctrl.queue_count; i++)
>> +        mutex_destroy(&ctrl->queues[i].queue_lock);
>
> What if in case ctrl->ctrl.queue_count changes during the lifetime
> of connection? I think in case num of queues changes during reconnect
> then ctrl.queue_count would not represent the initial value of the
> num of queues. 
Good catch, but this is not a bug, the queue_count will only be turn down,
so will not destroy an uninitialized mutex, and undestroyed mutex doesn't
cause any memory leak.

But, any way, I will send a v2 to fix this, use nctrl->opts->nr_io_queues +
nctrl->opts->nr_write_queues + nctrl->opts->nr_poll_queues + 1 to find the
initial queue_count to destroy all initialized mutex.

Best Regards,
Guixin Liu
>
> Thanks,
> --Nilay