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