[PATCH 11/14] nvmet: fix nvme module ref count Oops

[Chaitanya Kulkarni]

On 8/10/20 15:39, Logan Gunthorpe wrote:
> 
> On 2020-08-10 12:54 p.m., Chaitanya Kulkarni wrote:
>> In the passthru controller enable path current code doesn't take the
>> reference to the passthru ctrl module. Which produces following Oops :-
> Please mention the steps required to reproduce the oops.
> 
> [snip]
> 
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 05aa568a60af..ba019c0b514b 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -4608,6 +4608,11 @@ struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path)
>>   
>>   	ctrl = f->private_data;
>>   	nvme_get_ctrl(ctrl);
>> +	if (!try_module_get(ctrl->ops->module)) {
>> +		pr_err("try_module_get failed\n");
>> +		nvme_put_ctrl(ctrl);
>> +		ctrl = ERR_PTR(-EINVAL);
>> +	}
> This looks racy. If the module goes away after the nvme_get_ctrl(), then
> it will still crash.
> 
Hmm, let me see what I can do.
> But there might be a similar problem here with the core code. If a user
> opens a char device, they get a pointer to the ctrl in private_data
> without a reference. If the ctrl then goes away and they try to do an
> ioctl, it will crash. Similarly, if the underlying driver module goes
> away it will probably crash in the same way as this oops.
> 
I think you are right, still need read code and make sure.
> Looks to me like we need a reference taken to the ctrl and the module in
> nvme_dev_open() (which are put in the file release). Then we know the
> module can't go away inside nvme_ctrl_get_by_path().
> 
Let me see for next version if that works.
>>   out_close:
>>   	filp_close(f, NULL);
>> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
>> index 32f4951a1df7..5925e93c4682 100644
>> --- a/drivers/nvme/target/passthru.c
>> +++ b/drivers/nvme/target/passthru.c
>> @@ -488,6 +488,7 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
>>   
>>   static void nvme_pt_put_ctrl(struct nvme_ctrl *ctrl)
>>   {
>> +	module_put(ctrl->ops->module);
>>   	nvme_put_ctrl(ctrl);
>>   }
> Given that this must be done for every caller of nvme_ctrl_get_by_path()
> shouldn't this be in the core code?
> 
Yes it should be.
> Logan
>