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