[PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error

[Grupi, Elad]

Right. There is a race in patch v3.

Thanks

-----Original Message-----
From: Hou Pu <houpu.main at gmail.com> 
Sent: Tuesday, 30 March 2021 8:49
To: houpu.main at gmail.com
Cc: Grupi, Elad; linux-nvme at lists.infradead.org; sagi at 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:
> > 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