[PATCH] nvmet: Fix a use-after-free

[Sagi Grimberg]

>>> @@ -745,9 +747,9 @@ static void __nvmet_req_complete(struct nvmet_req 
>>> *req, u16 status)
>>>       trace_nvmet_req_complete(req);
>>> -    if (req->ns)
>>> -        nvmet_put_namespace(req->ns);
>>>       req->ops->queue_response(req);
>>> +    if (ns)
>>> +        nvmet_put_namespace(ns);
>>
>> Why did the put change position?
>> I'm not exactly clear what was used-after-free here..
> 
> Hi Sagi,
> 
> Is my understanding correct that the NVMe target namespace owns the 
> block device `req` is associated with and hence that the namespace 
> reference count must only be dropped after dereferencing the `req` 
> pointer has finished?
> 
> This is what I found in the NVMe target code:
> * nvmet_put_namespace() decreases ns->ref.
> * Dropping the last ns->ref causes nvmet_destroy_namespace() to be
>    called. That function completes ns->disable_done.
> * nvmet_ns_disable() waits on that completion and calls
>    nvmet_ns_dev_disable().
> * For a block device, nvmet_ns_dev_disable() calls blkdev_put().
> * The last blkdev_put() call calls disk_release().
> * disk_release() calls blk_put_queue().
> * The last blk_put_queue() call calls blk_release_queue().
> * blk_release_queue() frees struct request_queue.
> * blk_mq_complete_request_remote() dereferences req->q.

The analysis is correct, specifically for loop transport, but it is not
the same request_queue...

blk_mq_complete_request_remote references the initiator block device
created from the namespace, but nvmet_ns_dev_disable puts the backend
blockdev, which is a separate blockdev...

Maybe I'm misunderstanding your point?