All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error
@ 2021-03-29 18:01 elad.grupi
  2021-03-30  4:12 ` Hou Pu
  0 siblings, 1 reply; 5+ messages in thread
From: elad.grupi @ 2021-03-29 18:01 UTC (permalink / raw)
  To: sagi, linux-nvme; +Cc: Elad Grupi

From: Elad Grupi <elad.grupi@dell.com>

In case there is an io that contains inline data and it goes to
parsing error flow, command response will free command and iov
before clearing the data on the socket buffer.
This will delay the command response until receive flow is completed.

Fixes: 872d26a391da ("nvmet-tcp: add NVMe over TCP target driver")
Signed-off-by: Elad Grupi <elad.grupi@dell.com>
---
 drivers/nvme/target/tcp.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

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;
+	}
 
 	llist_add(&cmd->lentry, &queue->resp_list);
 	queue_work_on(queue_cpu(queue), nvmet_tcp_wq, &cmd->queue->io_work);
 }
 
+static void nvmet_tcp_execute_request(struct nvmet_tcp_cmd *cmd)
+{
+	if (unlikely(cmd->flags & NVMET_TCP_F_INIT_FAILED))
+		nvmet_tcp_queue_response(&cmd->req);
+	else
+		cmd->req.execute(&cmd->req);
+}
+
 static int nvmet_try_send_data_pdu(struct nvmet_tcp_cmd *cmd)
 {
 	u8 hdgst = nvmet_tcp_hdgst_len(cmd->queue);
@@ -1103,10 +1126,8 @@ static int nvmet_tcp_try_recv_data(struct nvmet_tcp_queue *queue)
 		return 0;
 	}
 
-	if (!(cmd->flags & NVMET_TCP_F_INIT_FAILED) &&
-	    cmd->rbytes_done == cmd->req.transfer_len) {
-		cmd->req.execute(&cmd->req);
-	}
+	if (cmd->rbytes_done == cmd->req.transfer_len)
+		nvmet_tcp_execute_request(cmd);
 
 	nvmet_prepare_receive_pdu(queue);
 	return 0;
@@ -1143,9 +1164,9 @@ static int nvmet_tcp_try_recv_ddgst(struct nvmet_tcp_queue *queue)
 		goto out;
 	}
 
-	if (!(cmd->flags & NVMET_TCP_F_INIT_FAILED) &&
-	    cmd->rbytes_done == cmd->req.transfer_len)
-		cmd->req.execute(&cmd->req);
+	if (cmd->rbytes_done == cmd->req.transfer_len)
+		nvmet_tcp_execute_request(cmd);
+
 	ret = 0;
 out:
 	nvmet_prepare_receive_pdu(queue);
-- 
2.18.2


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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error
  2021-03-29 18:01 [PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error elad.grupi
@ 2021-03-30  4:12 ` Hou Pu
  2021-03-30  5:48   ` Hou Pu
  0 siblings, 1 reply; 5+ messages in thread
From: Hou Pu @ 2021-03-30  4:12 UTC (permalink / raw)
  To: elad.grupi; +Cc: linux-nvme, sagi, houpu.main

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.

Thanks,
Hou


> +	}
 
>  	llist_add(&cmd->lentry, &queue->resp_list);
>  	queue_work_on(queue_cpu(queue), nvmet_tcp_wq, &cmd->queue->io_work);
>  }

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error
  2021-03-30  4:12 ` Hou Pu
@ 2021-03-30  5:48   ` Hou Pu
  2021-03-30 17:25     ` Grupi, Elad
  0 siblings, 1 reply; 5+ messages in thread
From: Hou Pu @ 2021-03-30  5:48 UTC (permalink / raw)
  To: houpu.main; +Cc: elad.grupi, linux-nvme, sagi

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

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* RE: [PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error
  2021-03-30  5:48   ` Hou Pu
@ 2021-03-30 17:25     ` Grupi, Elad
  0 siblings, 0 replies; 5+ messages in thread
From: Grupi, Elad @ 2021-03-30 17:25 UTC (permalink / raw)
  To: Hou Pu; +Cc: linux-nvme, sagi

Right. There is a race in patch v3.

Thanks

-----Original Message-----
From: Hou Pu <houpu.main@gmail.com> 
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:
> > 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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error
@ 2021-03-28  9:50 elad.grupi
  0 siblings, 0 replies; 5+ messages in thread
From: elad.grupi @ 2021-03-28  9:50 UTC (permalink / raw)
  To: sagi, linux-nvme; +Cc: Elad Grupi

From: Elad Grupi <elad.grupi@dell.com>

In case there is an io that contains inline data and it goes to
parsing error flow, command response will free command and iov
before clearing the data on the socket buffer.
This will delay the command response until receive flow is completed.

Fixes: 872d26a391da ("nvmet-tcp: add NVMe over TCP target driver")
Signed-off-by: Elad Grupi <elad.grupi@dell.com>
---
 drivers/nvme/target/tcp.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

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;
+	}
 
 	llist_add(&cmd->lentry, &queue->resp_list);
 	queue_work_on(queue_cpu(queue), nvmet_tcp_wq, &cmd->queue->io_work);
 }
 
+static void nvmet_tcp_execute_request(struct nvmet_tcp_cmd *cmd)
+{
+	if (unlikely(cmd->flags & NVMET_TCP_F_INIT_FAILED))
+		nvmet_tcp_queue_response(&cmd->req);
+	else
+		cmd->req.execute(&cmd->req);
+}
+
 static int nvmet_try_send_data_pdu(struct nvmet_tcp_cmd *cmd)
 {
 	u8 hdgst = nvmet_tcp_hdgst_len(cmd->queue);
@@ -1103,10 +1126,8 @@ static int nvmet_tcp_try_recv_data(struct nvmet_tcp_queue *queue)
 		return 0;
 	}
 
-	if (!(cmd->flags & NVMET_TCP_F_INIT_FAILED) &&
-	    cmd->rbytes_done == cmd->req.transfer_len) {
-		cmd->req.execute(&cmd->req);
-	}
+	if (cmd->rbytes_done == cmd->req.transfer_len)
+		nvmet_tcp_execute_request(cmd);
 
 	nvmet_prepare_receive_pdu(queue);
 	return 0;
@@ -1143,9 +1164,9 @@ static int nvmet_tcp_try_recv_ddgst(struct nvmet_tcp_queue *queue)
 		goto out;
 	}
 
-	if (!(cmd->flags & NVMET_TCP_F_INIT_FAILED) &&
-	    cmd->rbytes_done == cmd->req.transfer_len)
-		cmd->req.execute(&cmd->req);
+	if (cmd->rbytes_done == cmd->req.transfer_len)
+		nvmet_tcp_execute_request(cmd);
+
 	ret = 0;
 out:
 	nvmet_prepare_receive_pdu(queue);
-- 
2.18.2


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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-30 17:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-29 18:01 [PATCH v3] nvmet-tcp: fix a segmentation fault during io parsing error elad.grupi
2021-03-30  4:12 ` Hou Pu
2021-03-30  5:48   ` Hou Pu
2021-03-30 17:25     ` Grupi, Elad
  -- strict thread matches above, loose matches on Subject: below --
2021-03-28  9:50 elad.grupi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.