[PATVH V2 0/3] nvmet: passthru fixes

[PATVH V2 0/3] nvmet: passthru fixes

[Chaitanya Kulkarni]

Hi,

This is a small patch-series that fixes a bug in the passthru
submission path and makes the direct call to blk_mq_free_request().

First patch is prep patch to have labels for each action in the
nvnet_execute_passthru_cmd(), second patch fixes the actual oops,
third patch call blk_mq_free_request() directly.

Regards,
Chaitanya

* Changes from V1:-
-------------------

1. Don't use fail_XXX and be consistent with out_XXX_action as
   most of the target code labels.
2. Add a out_put_ns label as a prep patch to have proper error
   path handling for each action in passthru cmd setup.
3. Add a out_put_req label for blk_put_request() call and move this
   call to first in the error handling code path. Along with the
   reported but this also fixes potential Oops if nvme_find_get_ns()
   fails in the original code.
4. Add reviewers tag for the blk_mq_free_request() patch.

Chaitanya Kulkarni (3):
  nvmet: add ns tear down label for pt-cmd handling
  nvmet: fix oops in pt cmd execution
  nvmet: call blk_mq_free_request() directly

 drivers/nvme/target/passthru.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

-- 
2.22.1


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

[PATVH V2 1/3] nvmet: add ns tear down label for pt-cmd handling

[Chaitanya Kulkarni]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1795 bytes --]

In the current implementation before submitting the passthru cmd we
may come across error e.g. getting ns from passthru controller,
allocating a request from passthru controller, etc. For all the failure
cases it only uses single goto label fail_out.

In the target code, we follow the pattern to have a separate label for
each error out the case when setting up multiple things before the actual
action.

This patch follows the same pattern and renames generic fail_out label
to out_put_ns and updates the error out cases in the
nvmet_passthru_execute_cmd() where it is needed.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/passthru.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 89d91dc999a6..e7dbca12785d 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -230,7 +230,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 		if (unlikely(!ns)) {
 			pr_err("failed to get passthru ns nsid:%u\n", nsid);
 			status = NVME_SC_INVALID_NS | NVME_SC_DNR;
-			goto fail_out;
+			goto out;
 		}
 
 		q = ns->queue;
@@ -240,14 +240,14 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 	if (IS_ERR(rq)) {
 		rq = NULL;
 		status = NVME_SC_INTERNAL;
-		goto fail_out;
+		goto out_put_ns;
 	}
 
 	if (req->sg_cnt) {
 		ret = nvmet_passthru_map_sg(req, rq);
 		if (unlikely(ret)) {
 			status = NVME_SC_INTERNAL;
-			goto fail_out;
+			goto out_put_ns;
 		}
 	}
 
@@ -274,9 +274,10 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 
 	return;
 
-fail_out:
+out_put_ns:
 	if (ns)
 		nvme_put_ns(ns);
+out:
 	nvmet_req_complete(req, status);
 	blk_put_request(rq);
 }
-- 
2.22.1



[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

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

Re: [PATVH V2 1/3] nvmet: add ns tear down label for pt-cmd handling

[Logan Gunthorpe]

On 2020-08-06 8:20 p.m., Chaitanya Kulkarni wrote:
> In the current implementation before submitting the passthru cmd we
> may come across error e.g. getting ns from passthru controller,
> allocating a request from passthru controller, etc. For all the failure
> cases it only uses single goto label fail_out.
> 
> In the target code, we follow the pattern to have a separate label for
> each error out the case when setting up multiple things before the actual
> action.
> 
> This patch follows the same pattern and renames generic fail_out label
> to out_put_ns and updates the error out cases in the
> nvmet_passthru_execute_cmd() where it is needed.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/nvme/target/passthru.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
> index 89d91dc999a6..e7dbca12785d 100644
> --- a/drivers/nvme/target/passthru.c
> +++ b/drivers/nvme/target/passthru.c
> @@ -230,7 +230,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
>  		if (unlikely(!ns)) {
>  			pr_err("failed to get passthru ns nsid:%u\n", nsid);
>  			status = NVME_SC_INVALID_NS | NVME_SC_DNR;
> -			goto fail_out;
> +			goto out;

Not sure it's necessary to have this extra label seeing "if (ns)" takes
care of things. But, I guess, it doesn't really hurt.

For both this patch and the next one:

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Logan

>  		}
>  
>  		q = ns->queue;
> @@ -240,14 +240,14 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
>  	if (IS_ERR(rq)) {
>  		rq = NULL;
>  		status = NVME_SC_INTERNAL;
> -		goto fail_out;
> +		goto out_put_ns;
>  	}
>  
>  	if (req->sg_cnt) {
>  		ret = nvmet_passthru_map_sg(req, rq);
>  		if (unlikely(ret)) {
>  			status = NVME_SC_INTERNAL;
> -			goto fail_out;
> +			goto out_put_ns;
>  		}
>  	}
>  
> @@ -274,9 +274,10 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
>  
>  	return;
>  
> -fail_out:
> +out_put_ns:
>  	if (ns)
>  		nvme_put_ns(ns);
> +out:
>  	nvmet_req_complete(req, status);
>  	blk_put_request(rq);
>  }
> 

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

Re: [PATVH V2 1/3] nvmet: add ns tear down label for pt-cmd handling

[Sagi Grimberg]

> In the current implementation before submitting the passthru cmd we
> may come across error e.g. getting ns from passthru controller,
> allocating a request from passthru controller, etc. For all the failure
> cases it only uses single goto label fail_out.
> 
> In the target code, we follow the pattern to have a separate label for
> each error out the case when setting up multiple things before the actual
> action.
> 
> This patch follows the same pattern and renames generic fail_out label
> to out_put_ns and updates the error out cases in the
> nvmet_passthru_execute_cmd() where it is needed.

This patch seems fail when applying:

$ git am -s mail/chaitanya-pt-fixes/1
error: cannot convert from y to UTF-8
fatal: could not parse patch

Google search suggest that maybe the file is corrupted, but not sure.

Did you do something special with your environment?

Patches 2+3 do not have this issue.

Strange...

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

Re: [PATVH V2 1/3] nvmet: add ns tear down label for pt-cmd handling

[Chaitanya Kulkarni]

On 8/7/20 10:37 AM, Sagi Grimberg wrote:
> This patch seems fail when applying:
> 
> $ git am -s mail/chaitanya-pt-fixes/1
> error: cannot convert from y to UTF-8
> fatal: could not parse patch
> 
> Google search suggest that maybe the file is corrupted, but not sure.
> 
> Did you do something special with your environment?
> 
> Patches 2+3 do not have this issue.
> 
> Strange...

No, let me resend.

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

Re: [PATVH V2 1/3] nvmet: add ns tear down label for pt-cmd handling

[Christoph Hellwig]

Looks good modulo the patch file corruption:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATVH V2 2/3] nvmet: fix oops in pt cmd execution

[Chaitanya Kulkarni]

In the existing NVMeOF Passthru core command handling on failure of
nvme_alloc_request() it errors out with rq value set to NULL. In the
error handling path it calls blk_put_request() without checking if
rq is set to NULL or not which produces following Oops:-

[ 1457.346861] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 1457.347838] #PF: supervisor read access in kernel mode
[ 1457.348464] #PF: error_code(0x0000) - not-present page
[ 1457.349085] PGD 0 P4D 0
[ 1457.349402] Oops: 0000 [#1] SMP NOPTI
[ 1457.349851] CPU: 18 PID: 10782 Comm: kworker/18:2 Tainted: G           OE     5.8.0-rc4nvme-5.9+ #35
[ 1457.350951] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-59-gc9ba5276e3214
[ 1457.352347] Workqueue: events nvme_loop_execute_work [nvme_loop]
[ 1457.353062] RIP: 0010:blk_mq_free_request+0xe/0x110
[ 1457.353651] Code: 3f ff ff ff 83 f8 01 75 0d 4c 89 e7 e8 1b db ff ff e9 2d ff ff ff 0f 0b eb ef 66 8
[ 1457.355975] RSP: 0018:ffffc900035b7de0 EFLAGS: 00010282
[ 1457.356636] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000002
[ 1457.357526] RDX: ffffffffa060bd05 RSI: 0000000000000000 RDI: 0000000000000000
[ 1457.358416] RBP: 0000000000000037 R08: 0000000000000000 R09: 0000000000000000
[ 1457.359317] R10: 0000000000000000 R11: 000000000000006d R12: 0000000000000000
[ 1457.360424] R13: ffff8887ffa68600 R14: 0000000000000000 R15: ffff8888150564c8
[ 1457.361322] FS:  0000000000000000(0000) GS:ffff888814600000(0000) knlGS:0000000000000000
[ 1457.362337] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1457.363058] CR2: 0000000000000000 CR3: 000000081c0ac000 CR4: 00000000003406e0
[ 1457.363973] Call Trace:
[ 1457.364296]  nvmet_passthru_execute_cmd+0x150/0x2c0 [nvmet]
[ 1457.364990]  process_one_work+0x24e/0x5a0
[ 1457.365493]  ? __schedule+0x353/0x840
[ 1457.365957]  worker_thread+0x3c/0x380
[ 1457.366426]  ? process_one_work+0x5a0/0x5a0
[ 1457.366948]  kthread+0x135/0x150
[ 1457.367362]  ? kthread_create_on_node+0x60/0x60
[ 1457.367934]  ret_from_fork+0x22/0x30
[ 1457.368388] Modules linked in: nvme_loop(OE) nvmet(OE) nvme_fabrics(OE) null_blk nvme(OE) nvme_corer
[ 1457.368414]  ata_piix crc32c_intel virtio_pci libata virtio_ring serio_raw t10_pi virtio floppy dm_]
[ 1457.380849] CR2: 0000000000000000
[ 1457.381288] ---[ end trace c6cab61bfd1f68fd ]---
[ 1457.381861] RIP: 0010:blk_mq_free_request+0xe/0x110
[ 1457.382469] Code: 3f ff ff ff 83 f8 01 75 0d 4c 89 e7 e8 1b db ff ff e9 2d ff ff ff 0f 0b eb ef 66 8
[ 1457.384749] RSP: 0018:ffffc900035b7de0 EFLAGS: 00010282
[ 1457.385393] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000002
[ 1457.386264] RDX: ffffffffa060bd05 RSI: 0000000000000000 RDI: 0000000000000000
[ 1457.387142] RBP: 0000000000000037 R08: 0000000000000000 R09: 0000000000000000
[ 1457.388029] R10: 0000000000000000 R11: 000000000000006d R12: 0000000000000000
[ 1457.388914] R13: ffff8887ffa68600 R14: 0000000000000000 R15: ffff8888150564c8
[ 1457.389798] FS:  0000000000000000(0000) GS:ffff888814600000(0000) knlGS:0000000000000000
[ 1457.390796] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1457.391508] CR2: 0000000000000000 CR3: 000000081c0ac000 CR4: 00000000003406e0
[ 1457.392525] Kernel panic - not syncing: Fatal exception
[ 1457.394138] Kernel Offset: disabled
[ 1457.394677] ---[ end Kernel panic - not syncing: Fatal exception ]---

We fix this Oops by adding a new goto label out_put_req and reordering
the blk_put_request call to avoid calling blk_put_request() with rq
value is set to NULL. Here we also update the rest of the code
accordingly.

Fixes: 06b7164dfdc0 ("nvmet: add passthru code to process commands")
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/passthru.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index e7dbca12785d..aecabf423df2 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -238,7 +238,6 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 
 	rq = nvme_alloc_request(q, req->cmd, BLK_MQ_REQ_NOWAIT, NVME_QID_ANY);
 	if (IS_ERR(rq)) {
-		rq = NULL;
 		status = NVME_SC_INTERNAL;
 		goto out_put_ns;
 	}
@@ -247,7 +246,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 		ret = nvmet_passthru_map_sg(req, rq);
 		if (unlikely(ret)) {
 			status = NVME_SC_INTERNAL;
-			goto out_put_ns;
+			goto out_put_req;
 		}
 	}
 
@@ -274,12 +273,13 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 
 	return;
 
+out_put_req:
+	blk_put_request(rq);
 out_put_ns:
 	if (ns)
 		nvme_put_ns(ns);
 out:
 	nvmet_req_complete(req, status);
-	blk_put_request(rq);
 }
 
 /*
-- 
2.22.1


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

Re: [PATVH V2 2/3] nvmet: fix oops in pt cmd execution

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATVH V2 3/3] nvmet: call blk_mq_free_request() directly

[Chaitanya Kulkarni]

Instead of calling blk_put_request() which calls blk_mq_free_request(),
call blk_mq_free_request() directly for NVMeOF passthru. This is to
mainly avoid an extra function call in the completion path
nvmet_passthru_req_done().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/target/passthru.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index aecabf423df2..68701754b036 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -165,7 +165,7 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
 
 	req->cqe->result = nvme_req(rq)->result;
 	nvmet_req_complete(req, status);
-	blk_put_request(rq);
+	blk_mq_free_request(rq);
 }
 
 static void nvmet_passthru_req_done(struct request *rq,
@@ -175,7 +175,7 @@ static void nvmet_passthru_req_done(struct request *rq,
 
 	req->cqe->result = nvme_req(rq)->result;
 	nvmet_req_complete(req, nvme_req(rq)->status);
-	blk_put_request(rq);
+	blk_mq_free_request(rq);
 }
 
 static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
@@ -274,7 +274,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 	return;
 
 out_put_req:
-	blk_put_request(rq);
+	blk_mq_free_request(rq);
 out_put_ns:
 	if (ns)
 		nvme_put_ns(ns);
-- 
2.22.1


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

Re: [PATVH V2 3/3] nvmet: call blk_mq_free_request() directly

[Christoph Hellwig]

On Thu, Aug 06, 2020 at 07:20:32PM -0700, Chaitanya Kulkarni wrote:
> Instead of calling blk_put_request() which calls blk_mq_free_request(),
> call blk_mq_free_request() directly for NVMeOF passthru. This is to
> mainly avoid an extra function call in the completion path
> nvmet_passthru_req_done().

Not strictly a fix, but I think we can just queue this up for 5.9
anyway as it is for brand new code:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATVH V2 0/3] nvmet: passthru fixes

[Chaitanya Kulkarni]

Hi,

This is a small patch-series that fixes a bug in the passthru
submission path and makes the direct call to blk_mq_free_request().

First patch is prep patch to have labels for each action in the
nvnet_execute_passthru_cmd(), second patch fixes the actual oops,
third patch call blk_mq_free_request() directly.

Regards,
Chaitanya

* Changes from V1:-
-------------------

1. Don't use fail_XXX and be consistent with out_XXX_action as
   most of the target code labels.
2. Add a out_put_ns label as a prep patch to have proper error
   path handling for each action in passthru cmd setup.
3. Add a out_put_req label for blk_put_request() call and move this
   call to first in the error handling code path. Along with the
   reported but this also fixes potential Oops if nvme_find_get_ns()
   fails in the original code.
4. Add reviewers tag for the blk_mq_free_request() patch.

Chaitanya Kulkarni (3):
  nvmet: add ns tear down label for pt-cmd handling
  nvmet: fix oops in pt cmd execution
  nvmet: call blk_mq_free_request() directly

 drivers/nvme/target/passthru.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

-- 
2.22.1


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