[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);
 }

 /*