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

[Keith Busch <kbusch@kernel.org>]

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