* [PATCH 0/2] nvmet: couple of passthru fixes @ 2020-08-06 7:31 Chaitanya Kulkarni 2020-08-06 7:31 ` [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() Chaitanya Kulkarni 2020-08-06 7:31 ` [PATCH 2/2] nvmet: call blk_mq_free_request() directly Chaitanya Kulkarni 0 siblings, 2 replies; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 7:31 UTC (permalink / raw) To: linux-nvme; +Cc: sagi, Chaitanya Kulkarni, hch Hi, This is a small patch-series that fixes a bug in the passthru submission path and makes the the direct call to blk_mq_free_request(). Regarding second patch I didn't understand the exact reason to call blk_mq_free_request() indirectly via blk_put_request(), if there is one feel free to ignore. Regards, Chaitanya Chaitanya Kulkarni (2): nvmet: fix oops in nvmet_execute_passthru_cmd() nvmet: call blk_mq_free_request() directly drivers/nvme/target/passthru.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) -- 2.22.1 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 7:31 [PATCH 0/2] nvmet: couple of passthru fixes Chaitanya Kulkarni @ 2020-08-06 7:31 ` Chaitanya Kulkarni 2020-08-06 15:42 ` Keith Busch 2020-08-06 7:31 ` [PATCH 2/2] nvmet: call blk_mq_free_request() directly Chaitanya Kulkarni 1 sibling, 1 reply; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 7:31 UTC (permalink / raw) To: linux-nvme; +Cc: sagi, Chaitanya Kulkarni, hch This patch adds a check in nvmet_execute_passthru_cmd() to prevent the 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 ]--- Fixes: 06b7164dfdc0 ("nvmet: add passthru code to process commands") Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> --- drivers/nvme/target/passthru.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c index 89d91dc999a6..b56292c9a76c 100644 --- a/drivers/nvme/target/passthru.c +++ b/drivers/nvme/target/passthru.c @@ -278,7 +278,8 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req) if (ns) nvme_put_ns(ns); nvmet_req_complete(req, status); - blk_put_request(rq); + if (rq) + blk_put_request(rq); } /* -- 2.22.1 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 7:31 ` [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() Chaitanya Kulkarni @ 2020-08-06 15:42 ` Keith Busch 2020-08-06 16:12 ` Logan Gunthorpe 2020-08-06 19:29 ` Chaitanya Kulkarni 0 siblings, 2 replies; 22+ messages in thread From: Keith Busch @ 2020-08-06 15:42 UTC (permalink / raw) To: Chaitanya Kulkarni; +Cc: sagi, linux-nvme, hch On Thu, Aug 06, 2020 at 12:31:50AM -0700, Chaitanya Kulkarni wrote: > This patch adds a check in nvmet_execute_passthru_cmd() to prevent the > following Oops :- Suggested changelog: A passthrough request may be NULL if an invalid namespace is used, or if the allocation failed. Check for a valid request before releasing it to fix a NULL pointer dereference. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 15:42 ` Keith Busch @ 2020-08-06 16:12 ` Logan Gunthorpe 2020-08-06 16:26 ` Keith Busch ` (2 more replies) 2020-08-06 19:29 ` Chaitanya Kulkarni 1 sibling, 3 replies; 22+ messages in thread From: Logan Gunthorpe @ 2020-08-06 16:12 UTC (permalink / raw) To: Keith Busch, Chaitanya Kulkarni; +Cc: sagi, linux-nvme, hch Hi Chaitanya, Thanks for the fixes but can you please CC me on bug fixes to this code? On 2020-08-06 9:42 a.m., Keith Busch wrote: > On Thu, Aug 06, 2020 at 12:31:50AM -0700, Chaitanya Kulkarni wrote: >> This patch adds a check in nvmet_execute_passthru_cmd() to prevent the >> following Oops :- It would be nice to have in the changelog what was done to trigger this oops. > Suggested changelog: > > A passthrough request may be NULL if an invalid namespace is used, or > if the allocation failed. Check for a valid request before releasing > it to fix a NULL pointer dereference. I don't think this is quite accurate either. It looks to me like it's just a bug in the error path logic. If the namespace is invalid, it doesn't even try to allocate the request so it should really just order the cleanup correctly and skip it, if it's not allocated. I think a fix like this would be cleaner: diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c index a260c09b5ab2..96bc9aa11ddb 100644 --- a/drivers/nvme/target/passthru.c +++ b/drivers/nvme/target/passthru.c @@ -239,7 +239,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 fail_out; } @@ -248,7 +247,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 fail_out; + goto fail_put_req; } } @@ -275,11 +274,12 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req) return; +fail_put_req: + blk_put_request(rq); fail_out: if (ns) nvme_put_ns(ns); 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 16:12 ` Logan Gunthorpe @ 2020-08-06 16:26 ` Keith Busch 2020-08-06 19:40 ` Chaitanya Kulkarni 2020-08-06 19:18 ` Sagi Grimberg 2020-08-06 19:31 ` Chaitanya Kulkarni 2 siblings, 1 reply; 22+ messages in thread From: Keith Busch @ 2020-08-06 16:26 UTC (permalink / raw) To: Logan Gunthorpe; +Cc: linux-nvme, sagi, Chaitanya Kulkarni, hch On Thu, Aug 06, 2020 at 10:12:24AM -0600, Logan Gunthorpe wrote: > On 2020-08-06 9:42 a.m., Keith Busch wrote: > > On Thu, Aug 06, 2020 at 12:31:50AM -0700, Chaitanya Kulkarni wrote: > >> This patch adds a check in nvmet_execute_passthru_cmd() to prevent the > >> following Oops :- > > It would be nice to have in the changelog what was done to trigger this > oops. > > > Suggested changelog: > > > > A passthrough request may be NULL if an invalid namespace is used, or > > if the allocation failed. Check for a valid request before releasing > > it to fix a NULL pointer dereference. > > I don't think this is quite accurate either. It looks to me like it's > just a bug in the error path logic. If the namespace is invalid, it > doesn't even try to allocate the request so it should really just order > the cleanup correctly and skip it, if it's not allocated. > > I think a fix like this would be cleaner: Agree, this looks like a more appropriate way to unwind on failure. > diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c > index a260c09b5ab2..96bc9aa11ddb 100644 > --- a/drivers/nvme/target/passthru.c > +++ b/drivers/nvme/target/passthru.c > @@ -239,7 +239,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 fail_out; > } > @@ -248,7 +247,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 fail_out; > + goto fail_put_req; > } > } > > @@ -275,11 +274,12 @@ static void nvmet_passthru_execute_cmd(struct > nvmet_req *req) > > return; > > +fail_put_req: > + blk_put_request(rq); > fail_out: > if (ns) > nvme_put_ns(ns); > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 16:26 ` Keith Busch @ 2020-08-06 19:40 ` Chaitanya Kulkarni 2020-08-06 19:45 ` Logan Gunthorpe 0 siblings, 1 reply; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 19:40 UTC (permalink / raw) To: Keith Busch, Logan Gunthorpe; +Cc: sagi, linux-nvme, hch Logan, >> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c >> index a260c09b5ab2..96bc9aa11ddb 100644 >> --- a/drivers/nvme/target/passthru.c >> +++ b/drivers/nvme/target/passthru.c >> @@ -239,7 +239,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 fail_out; >> } >> @@ -248,7 +247,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 fail_out; >> + goto fail_put_req; >> } >> } >> >> @@ -275,11 +274,12 @@ static void nvmet_passthru_execute_cmd(struct >> nvmet_req *req) >> >> return; >> >> +fail_put_req: >> + blk_put_request(rq); >> fail_out: >> if (ns) >> nvme_put_ns(ns); >> nvmet_req_complete(req, status); >> - blk_put_request(rq); >> } >> >> /* > This adds an extra label. Also I don't understand why we should change the order of nvmet_req_complete() and blk_put_request() in nvmet_passthru_execute_cmd() and make is inconsistent with in nvmet_passthru_req_done() ? _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 19:40 ` Chaitanya Kulkarni @ 2020-08-06 19:45 ` Logan Gunthorpe 2020-08-06 19:54 ` Sagi Grimberg 0 siblings, 1 reply; 22+ messages in thread From: Logan Gunthorpe @ 2020-08-06 19:45 UTC (permalink / raw) To: Chaitanya Kulkarni, Keith Busch; +Cc: sagi, linux-nvme, hch On 2020-08-06 1:40 p.m., Chaitanya Kulkarni wrote: > Logan, > >>> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c >>> index a260c09b5ab2..96bc9aa11ddb 100644 >>> --- a/drivers/nvme/target/passthru.c >>> +++ b/drivers/nvme/target/passthru.c >>> @@ -239,7 +239,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 fail_out; >>> } >>> @@ -248,7 +247,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 fail_out; >>> + goto fail_put_req; >>> } >>> } >>> >>> @@ -275,11 +274,12 @@ static void nvmet_passthru_execute_cmd(struct >>> nvmet_req *req) >>> >>> return; >>> >>> +fail_put_req: >>> + blk_put_request(rq); >>> fail_out: >>> if (ns) >>> nvme_put_ns(ns); >>> nvmet_req_complete(req, status); >>> - blk_put_request(rq); >>> } >>> >>> /* >> > > This adds an extra label. Also I don't understand why we should change > the order of nvmet_req_complete() and blk_put_request() in > nvmet_passthru_execute_cmd() and make is inconsistent with in > nvmet_passthru_req_done() ? I don't know why an extra label is an issue and I don't think the inconsistency is important. This follows the common pattern where things are cleaned up in the reverse order of their creation and labels are used to control which cleanups happen based on how far it got: rc = setup1(); if (rc) return -1; rc = setup2(); if (rc) goto out_teardown1; rc = setup3(); if (rc) goto teardown2; return; out_teradown2: teardown2(); out_teardown1: teardown1(); return -1; _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 19:45 ` Logan Gunthorpe @ 2020-08-06 19:54 ` Sagi Grimberg 2020-08-06 19:55 ` Logan Gunthorpe 2020-08-06 19:55 ` Chaitanya Kulkarni 0 siblings, 2 replies; 22+ messages in thread From: Sagi Grimberg @ 2020-08-06 19:54 UTC (permalink / raw) To: Logan Gunthorpe, Chaitanya Kulkarni, Keith Busch; +Cc: hch, linux-nvme >> This adds an extra label. Also I don't understand why we should change >> the order of nvmet_req_complete() and blk_put_request() in >> nvmet_passthru_execute_cmd() and make is inconsistent with in >> nvmet_passthru_req_done() ? > > I don't know why an extra label is an issue and I don't think the > inconsistency is important. This follows the common pattern where things > are cleaned up in the reverse order of their creation and labels are > used to control which cleanups happen based on how far it got: > > rc = setup1(); > if (rc) > return -1; > > rc = setup2(); > if (rc) > goto out_teardown1; > > > rc = setup3(); > if (rc) > goto teardown2; > > return; > > out_teradown2: > teardown2(); > out_teardown1: > teardown1(); > return -1; > Can we also add a label to remove the if (ns) condition? _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 19:54 ` Sagi Grimberg @ 2020-08-06 19:55 ` Logan Gunthorpe 2020-08-06 19:57 ` Chaitanya Kulkarni 2020-08-06 19:55 ` Chaitanya Kulkarni 1 sibling, 1 reply; 22+ messages in thread From: Logan Gunthorpe @ 2020-08-06 19:55 UTC (permalink / raw) To: Sagi Grimberg, Chaitanya Kulkarni, Keith Busch; +Cc: hch, linux-nvme On 2020-08-06 1:54 p.m., Sagi Grimberg wrote: > >>> This adds an extra label. Also I don't understand why we should change >>> the order of nvmet_req_complete() and blk_put_request() in >>> nvmet_passthru_execute_cmd() and make is inconsistent with in >>> nvmet_passthru_req_done() ? >> >> I don't know why an extra label is an issue and I don't think the >> inconsistency is important. This follows the common pattern where things >> are cleaned up in the reverse order of their creation and labels are >> used to control which cleanups happen based on how far it got: >> >> rc = setup1(); >> if (rc) >> return -1; >> >> rc = setup2(); >> if (rc) >> goto out_teardown1; >> >> >> rc = setup3(); >> if (rc) >> goto teardown2; >> >> return; >> >> out_teradown2: >> teardown2(); >> out_teardown1: >> teardown1(); >> return -1; >> > > Can we also add a label to remove the if (ns) condition? No, because the ns is only conditionally allocated if the queue-id is non-zero. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 19:55 ` Logan Gunthorpe @ 2020-08-06 19:57 ` Chaitanya Kulkarni 2020-08-06 20:03 ` Sagi Grimberg 0 siblings, 1 reply; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 19:57 UTC (permalink / raw) To: Logan Gunthorpe, Sagi Grimberg, Keith Busch; +Cc: hch, linux-nvme On 8/6/20 12:55 PM, Logan Gunthorpe wrote: > No, because the ns is only conditionally allocated if the queue-id is > non-zero. > but ns == NULL? _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 19:57 ` Chaitanya Kulkarni @ 2020-08-06 20:03 ` Sagi Grimberg 2020-08-06 20:06 ` Chaitanya Kulkarni 0 siblings, 1 reply; 22+ messages in thread From: Sagi Grimberg @ 2020-08-06 20:03 UTC (permalink / raw) To: Chaitanya Kulkarni, Logan Gunthorpe, Keith Busch; +Cc: hch, linux-nvme >> No, because the ns is only conditionally allocated if the queue-id is >> non-zero. >> > but ns == NULL? Logan is right, you have the admin queue condition... _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 20:03 ` Sagi Grimberg @ 2020-08-06 20:06 ` Chaitanya Kulkarni 0 siblings, 0 replies; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 20:06 UTC (permalink / raw) To: Sagi Grimberg, Logan Gunthorpe, Keith Busch; +Cc: hch, linux-nvme On 8/6/20 1:03 PM, Sagi Grimberg wrote: >>> No, because the ns is only conditionally allocated if the queue-id is >>> non-zero. >>> >> but ns == NULL? > Logan is right, you have the admin queue condition... > Let me send V2 it is easier to discuss on the code than text. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 19:54 ` Sagi Grimberg 2020-08-06 19:55 ` Logan Gunthorpe @ 2020-08-06 19:55 ` Chaitanya Kulkarni 1 sibling, 0 replies; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 19:55 UTC (permalink / raw) To: Sagi Grimberg, Logan Gunthorpe, Keith Busch; +Cc: hch, linux-nvme On 8/6/20 12:54 PM, Sagi Grimberg wrote: > Can we also add a label to remove the if (ns) condition? > Sure, as it is we are adding label not we might as well do it. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 16:12 ` Logan Gunthorpe 2020-08-06 16:26 ` Keith Busch @ 2020-08-06 19:18 ` Sagi Grimberg 2020-08-06 19:31 ` Chaitanya Kulkarni 2 siblings, 0 replies; 22+ messages in thread From: Sagi Grimberg @ 2020-08-06 19:18 UTC (permalink / raw) To: Logan Gunthorpe, Keith Busch, Chaitanya Kulkarni; +Cc: hch, linux-nvme > I don't think this is quite accurate either. It looks to me like it's > just a bug in the error path logic. If the namespace is invalid, it > doesn't even try to allocate the request so it should really just order > the cleanup correctly and skip it, if it's not allocated. > > I think a fix like this would be cleaner: Can you please send a proper patch for this Logan/Chaitanya? _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 16:12 ` Logan Gunthorpe 2020-08-06 16:26 ` Keith Busch 2020-08-06 19:18 ` Sagi Grimberg @ 2020-08-06 19:31 ` Chaitanya Kulkarni 2020-08-06 19:41 ` Sagi Grimberg 2 siblings, 1 reply; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 19:31 UTC (permalink / raw) To: Logan Gunthorpe, Keith Busch; +Cc: sagi, linux-nvme, hch On 8/6/20 9:12 AM, Logan Gunthorpe wrote: > Hi Chaitanya, > > Thanks for the fixes but can you please CC me on bug fixes to this code? > Sure. > On 2020-08-06 9:42 a.m., Keith Busch wrote: >> On Thu, Aug 06, 2020 at 12:31:50AM -0700, Chaitanya Kulkarni wrote: >>> This patch adds a check in nvmet_execute_passthru_cmd() to prevent the >>> following Oops :- > > It would be nice to have in the changelog what was done to trigger this > oops. > >> Suggested changelog: >> >> A passthrough request may be NULL if an invalid namespace is used, or >> if the allocation failed. Check for a valid request before releasing >> it to fix a NULL pointer dereference. > > I don't think this is quite accurate either. It looks to me like it's > just a bug in the error path logic. If the namespace is invalid, it > doesn't even try to allocate the request so it should really just order > the cleanup correctly and skip it, if it's not allocated. > > I think a fix like this would be cleaner: > Christoph, Sagi how guys would like to have this fix? I personally like to avoid introducing new labels unless they are extremely necessary. I'll wait for Christoph and Sagi's opinion then send V2. > > diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c > index a260c09b5ab2..96bc9aa11ddb 100644 > --- a/drivers/nvme/target/passthru.c > +++ b/drivers/nvme/target/passthru.c > @@ -239,7 +239,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 fail_out; > } > @@ -248,7 +247,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 fail_out; > + goto fail_put_req; > } > } > > @@ -275,11 +274,12 @@ static void nvmet_passthru_execute_cmd(struct > nvmet_req *req) > > return; > > +fail_put_req: > + blk_put_request(rq); > fail_out: > if (ns) > nvme_put_ns(ns); > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 19:31 ` Chaitanya Kulkarni @ 2020-08-06 19:41 ` Sagi Grimberg 2020-08-06 19:44 ` Chaitanya Kulkarni 0 siblings, 1 reply; 22+ messages in thread From: Sagi Grimberg @ 2020-08-06 19:41 UTC (permalink / raw) To: Chaitanya Kulkarni, Logan Gunthorpe, Keith Busch; +Cc: hch, linux-nvme >> I don't think this is quite accurate either. It looks to me like it's >> just a bug in the error path logic. If the namespace is invalid, it >> doesn't even try to allocate the request so it should really just order >> the cleanup correctly and skip it, if it's not allocated. >> >> I think a fix like this would be cleaner: >> > > Christoph, Sagi how guys would like to have this fix? > > I personally like to avoid introducing new labels unless they are > extremely necessary. I think that having an explicit and reverse-ordered target error labels are always better. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 19:41 ` Sagi Grimberg @ 2020-08-06 19:44 ` Chaitanya Kulkarni 0 siblings, 0 replies; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 19:44 UTC (permalink / raw) To: Sagi Grimberg, Logan Gunthorpe, Keith Busch; +Cc: hch, linux-nvme On 8/6/20 12:41 PM, Sagi Grimberg wrote: >> Christoph, Sagi how guys would like to have this fix? >> >> I personally like to avoid introducing new labels unless they are >> extremely necessary. > I think that having an explicit and reverse-ordered target error labels > are always better. > Okay will send v2 shortly, although it dose create an inconsistency between a call to nvmet_req_complete() and blk_put_request() in nvmet_execute_passthru_cmd() and nvmet_passthru_req_done() which I didn't understand why we should allow it. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() 2020-08-06 15:42 ` Keith Busch 2020-08-06 16:12 ` Logan Gunthorpe @ 2020-08-06 19:29 ` Chaitanya Kulkarni 1 sibling, 0 replies; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 19:29 UTC (permalink / raw) To: Keith Busch; +Cc: sagi, linux-nvme, hch On 8/6/20 8:43 AM, Keith Busch wrote: > On Thu, Aug 06, 2020 at 12:31:50AM -0700, Chaitanya Kulkarni wrote: >> This patch adds a check in nvmet_execute_passthru_cmd() to prevent the >> following Oops :- > Suggested changelog: > > A passthrough request may be NULL if an invalid namespace is used, or > if the allocation failed. Check for a valid request before releasing > it to fix a NULL pointer dereference. > Okay, will send V2. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] nvmet: call blk_mq_free_request() directly 2020-08-06 7:31 [PATCH 0/2] nvmet: couple of passthru fixes Chaitanya Kulkarni 2020-08-06 7:31 ` [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() Chaitanya Kulkarni @ 2020-08-06 7:31 ` Chaitanya Kulkarni 2020-08-06 15:45 ` Keith Busch ` (2 more replies) 1 sibling, 3 replies; 22+ messages in thread From: Chaitanya Kulkarni @ 2020-08-06 7:31 UTC (permalink / raw) To: linux-nvme; +Cc: sagi, Chaitanya Kulkarni, hch 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> --- 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 b56292c9a76c..050e68b9f570 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) @@ -279,7 +279,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req) nvme_put_ns(ns); nvmet_req_complete(req, status); if (rq) - blk_put_request(rq); + blk_mq_free_request(rq); } /* -- 2.22.1 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] nvmet: call blk_mq_free_request() directly 2020-08-06 7:31 ` [PATCH 2/2] nvmet: call blk_mq_free_request() directly Chaitanya Kulkarni @ 2020-08-06 15:45 ` Keith Busch 2020-08-06 16:14 ` Logan Gunthorpe 2020-08-06 19:19 ` Sagi Grimberg 2 siblings, 0 replies; 22+ messages in thread From: Keith Busch @ 2020-08-06 15:45 UTC (permalink / raw) To: Chaitanya Kulkarni; +Cc: sagi, linux-nvme, hch On Thu, Aug 06, 2020 at 12:31:51AM -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(). This is fine. blk_put_request() is a left-over function before blk-mq. Reviewed-by: Keith Busch <kbusch@kernel.org> _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] nvmet: call blk_mq_free_request() directly 2020-08-06 7:31 ` [PATCH 2/2] nvmet: call blk_mq_free_request() directly Chaitanya Kulkarni 2020-08-06 15:45 ` Keith Busch @ 2020-08-06 16:14 ` Logan Gunthorpe 2020-08-06 19:19 ` Sagi Grimberg 2 siblings, 0 replies; 22+ messages in thread From: Logan Gunthorpe @ 2020-08-06 16:14 UTC (permalink / raw) To: Chaitanya Kulkarni, linux-nvme; +Cc: sagi, hch On 2020-08-06 1:31 a.m., 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(). > > Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> Looks good to me. It seems blk_mq_free_request() is currently always used with nvme_alloc_request(). Reviewed-by: Logan Gunthorpe <logang@deltatee.com> Thanks, Logan _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] nvmet: call blk_mq_free_request() directly 2020-08-06 7:31 ` [PATCH 2/2] nvmet: call blk_mq_free_request() directly Chaitanya Kulkarni 2020-08-06 15:45 ` Keith Busch 2020-08-06 16:14 ` Logan Gunthorpe @ 2020-08-06 19:19 ` Sagi Grimberg 2 siblings, 0 replies; 22+ messages in thread From: Sagi Grimberg @ 2020-08-06 19:19 UTC (permalink / raw) To: Chaitanya Kulkarni, linux-nvme; +Cc: hch This one looks good, I can queue this one up. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-08-06 20:06 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-06 7:31 [PATCH 0/2] nvmet: couple of passthru fixes Chaitanya Kulkarni 2020-08-06 7:31 ` [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd() Chaitanya Kulkarni 2020-08-06 15:42 ` Keith Busch 2020-08-06 16:12 ` Logan Gunthorpe 2020-08-06 16:26 ` Keith Busch 2020-08-06 19:40 ` Chaitanya Kulkarni 2020-08-06 19:45 ` Logan Gunthorpe 2020-08-06 19:54 ` Sagi Grimberg 2020-08-06 19:55 ` Logan Gunthorpe 2020-08-06 19:57 ` Chaitanya Kulkarni 2020-08-06 20:03 ` Sagi Grimberg 2020-08-06 20:06 ` Chaitanya Kulkarni 2020-08-06 19:55 ` Chaitanya Kulkarni 2020-08-06 19:18 ` Sagi Grimberg 2020-08-06 19:31 ` Chaitanya Kulkarni 2020-08-06 19:41 ` Sagi Grimberg 2020-08-06 19:44 ` Chaitanya Kulkarni 2020-08-06 19:29 ` Chaitanya Kulkarni 2020-08-06 7:31 ` [PATCH 2/2] nvmet: call blk_mq_free_request() directly Chaitanya Kulkarni 2020-08-06 15:45 ` Keith Busch 2020-08-06 16:14 ` Logan Gunthorpe 2020-08-06 19:19 ` Sagi Grimberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).