Re: [PATCH 2/2] nvmet: fix a race condition between release_queue and io_work

Sagi Grimberg
2021-11-15T10:09:26Z

>> How about something like this:
>> --
>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>> index 6eb0b3153477..6425375faf5b 100644
>> --- a/drivers/nvme/target/tcp.c
>> +++ b/drivers/nvme/target/tcp.c
>> @@ -1436,7 +1436,9 @@ static void nvmet_tcp_release_queue_work(struct
>> work_struct *w)
>> mutex_unlock(&nvmet_tcp_queue_mutex);
>>
>> nvmet_tcp_restore_socket_callbacks(queue);
>> - flush_work(&queue->io_work);
>> + /* stop accepting incoming data */
>> + kernel_sock_shutdown(queue->sock, SHUT_RD);
>> + cancel_work_sync(&queue->io_work);
>>
>> nvmet_tcp_uninit_data_in_cmds(queue);
>> nvmet_sq_destroy(&queue->nvme_sq);
>> --
>>
>
> This one doesn't work, remember that nvmet_sq_destroy() will start io_work
> again and, as I wrote in my previous email, recvmsg() might still return some
> data after the socket has been shut down.

I see, the reason why we hit this is because we uninit_data_in_cmds as
we need to clear the the sq references so nvmet_sq_destroy() can
complete, and then when nvmet_sq_destroy schedules io_work we hit this.

I think what we need is to make sure we don't recv from the socket.
How about this patch:
--
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 6eb0b3153477..65210dec3f1a 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1436,6 +1436,8 @@ static void nvmet_tcp_release_queue_work(struct
work_struct *w)
mutex_unlock(&nvmet_tcp_queue_mutex);

nvmet_tcp_restore_socket_callbacks(queue);
+ /* stop accepting incoming data */
+ queue->rcv_state = NVMET_TCP_RECV_ERR;
flush_work(&queue->io_work);

nvmet_tcp_uninit_data_in_cmds(queue);
--