Right. There is a race in patch v3.

Thanks

-----Original Message-----
From: Hou Pu <redacted> 
Sent: Tuesday, 30 March 2021 8:49
To: houpu.main@gmail.com
Cc: Grupi, Elad; linux-nvme@lists.infradead.org; sagi@grimberg.me
Subject: Re: [PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error


[EXTERNAL EMAIL] 

On Tue, 30 Mar 2021 12:12:19 +0800, Hou Pu wrote:

On Date: Mon, 29 Mar 2021 21:01:25 +0300, Elad Grupi wrote:

quoted

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c 
index 70cc507d1565..41102fc09595 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c

@@ -525,11 +525,34 @@ static void nvmet_tcp_queue_response(struct nvmet_req *req)
 	struct nvmet_tcp_cmd *cmd =
 		container_of(req, struct nvmet_tcp_cmd, req);
 	struct nvmet_tcp_queue	*queue = cmd->queue;
+	struct nvme_sgl_desc *sgl;
+	u32 len;
+
+	if (unlikely(cmd == queue->cmd)) {
+		sgl = &cmd->req.cmd->common.dptr.sgl;
+		len = le32_to_cpu(sgl->length);
+
+		/*
+		 * Wait for inline data before processing the response.
+		 * Avoid using helpers, this might happen before
+		 * nvmet_req_init is completed.
+		 */
+		if (len && cmd->rcv_state == NVMET_TCP_RECV_PDU)
+			return;

Is it queue->rcv_state ?
I tried this patch, the identify command could get here. And nvme connect could hang.
We need to figure out a way to tell if it needs abort queue the 
request. Or maybe we could use the v2 version.

Adding nvme_is_write() would solve the problem.
Also as we skip queue queue->io_work, we should return
0 instead -EAGAIN like below to consume the inline data in nvmet_tcp_try_recv_one(). Or the io_work might not have a chance to run.

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c index a10a3bd59..f3d117771 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c

@@ -537,7 +537,8 @@ static void nvmet_tcp_queue_response(struct nvmet_req *req)
                 * Avoid using helpers, this might happen before
                 * nvmet_req_init is completed.
                 */
-               if (len && cmd->rcv_state == NVMET_TCP_RECV_PDU)
+               if (len && queue->rcv_state == NVMET_TCP_RECV_PDU &&
+                   nvme_is_write(cmd->req.cmd))
                        return;
        }

@@ -984,7 +985,7 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
                        le32_to_cpu(req->cmd->common.dptr.sgl.length));

                nvmet_tcp_handle_req_failure(queue, queue->cmd, req);
-               return -EAGAIN;
+               return 0;
        }

        ret = nvmet_tcp_map_data(queue->cmd);

Thanks,
Hou
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme