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

[Logan Gunthorpe]

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.

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.

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().

>  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?

Logan