Hi Sagi,

On Thu, Nov 04, 2021 at 02:59:53PM +0200, Sagi Grimberg wrote:

quoted

Right, after the call to cancel_work_sync we will know that io_work
is not running. Note that it can run as a result of a backend completion
but that is ok and we do want to let it run and return completion to the
host, but the socket should already be shut down for recv, so we cannot
get any other byte from the network.

I did some tests and I found out that kernel_recvmsg() sometimes returns
data even if the socket has been already shut down (maybe it's data it received
before the call to kernel_sock_shutdown() and waiting in some internal buffer?).

So when nvmet_sq_destroy() triggered io_work, recvmsg() still returned data
and the kernel crashed again despite the socket was closed.

Therefore, I think that after we shut down the socket we
should let io_work run and requeue itself until it finishes its job
and no more data is returned by recvmsg(),
one way to achieve this is to repeatedly call flush_work() until it returns
false.

Right now I am testing the patch below and it works perfectly.

Note that when the socket is closed recvmsg() might return 0,
nvmet_tcp_try_recv_data() should return -EAGAIN
in that case otherwise we end up in an infinite loop (io_work
will countinously requeue itself).

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 2f03a94725ae..7b441071c6b9 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c

@@ -1139,8 +1139,10 @@ static int nvmet_tcp_try_recv_data(struct nvmet_tcp_queue *queue)
  	while (msg_data_left(&cmd->recv_msg)) {
  		ret = sock_recvmsg(cmd->queue->sock, &cmd->recv_msg,
  			cmd->recv_msg.msg_flags);
-		if (ret <= 0)
+		if (ret < 0)
  			return ret;
+		else if (ret == 0)
+			return -EAGAIN;
  
  		cmd->pdu_recv += ret;
  		cmd->rbytes_done += ret;

@@ -1446,8 +1450,10 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
  	list_del_init(&queue->queue_list);
  	mutex_unlock(&nvmet_tcp_queue_mutex);
  
+	kernel_sock_shutdown(queue->sock, SHUT_RD);
+
  	nvmet_tcp_restore_socket_callbacks(queue);