[PATCH 0/2] nvmet: couple of passthru fixes

[PATCH 0/2] nvmet: couple of passthru fixes

[Chaitanya Kulkarni]

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

[PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Chaitanya Kulkarni]

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

[PATCH 2/2] 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>
---
 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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Keith Busch]

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

Re: [PATCH 2/2] nvmet: call blk_mq_free_request() directly

[Keith Busch]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Logan Gunthorpe]

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

Re: [PATCH 2/2] nvmet: call blk_mq_free_request() directly

[Logan Gunthorpe]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Keith Busch]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Sagi Grimberg]

> 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

Re: [PATCH 2/2] nvmet: call blk_mq_free_request() directly

[Sagi Grimberg]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Chaitanya Kulkarni]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Chaitanya Kulkarni]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Chaitanya Kulkarni]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Sagi Grimberg]

>> 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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Chaitanya Kulkarni]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Logan Gunthorpe]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Sagi Grimberg]

>> 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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Logan Gunthorpe]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Chaitanya Kulkarni]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Chaitanya Kulkarni]

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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Sagi Grimberg]

>> 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

Re: [PATCH 1/2] nvmet: fix oops in nvmet_execute_passthru_cmd()

[Chaitanya Kulkarni]

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