blktests nvme 041,042 leak memory
Hannes Reinecke
hare at suse.de
Mon Jun 3 01:37:33 PDT 2024
On 5/31/24 16:37, Maurizio Lombardi wrote:
> V Fri, May 31, 2024 at 01:55:03PM +0200, Maurizio Lombardi napsal(a):
>> Maybe a better idea:
>>
>> move device_initialize() to the very beginning of nvme_init_ctrl()
>> initialize ctrl->instance to a negative value, so we can check in the
>> .release() method if we have to call ida_free() or not.
>>
>
> This is how it would look like, I did some base testing with TCP
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 371e14f0a203..f809a4fc23f9 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -886,7 +886,7 @@ int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
> }
> EXPORT_SYMBOL_GPL(nvme_auth_wait);
>
> -static void nvme_ctrl_auth_work(struct work_struct *work)
> +void nvme_ctrl_auth_work(struct work_struct *work)
> {
> struct nvme_ctrl *ctrl =
> container_of(work, struct nvme_ctrl, dhchap_auth_work);
> @@ -934,14 +934,13 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
> "qid %d: authentication failed\n", q);
> }
> }
> +EXPORT_SYMBOL_GPL(nvme_ctrl_auth_work);
>
> int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
> {
> struct nvme_dhchap_queue_context *chap;
> int i, ret;
>
> - mutex_init(&ctrl->dhchap_auth_mutex);
> - INIT_WORK(&ctrl->dhchap_auth_work, nvme_ctrl_auth_work);
> if (!ctrl->opts)
> return 0;
> ret = nvme_auth_generate_key(ctrl->opts->dhchap_secret,
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 954f850f113a..e775c4df9af5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4572,14 +4572,16 @@ static void nvme_free_ctrl(struct device *dev)
> container_of(dev, struct nvme_ctrl, ctrl_device);
> struct nvme_subsystem *subsys = ctrl->subsys;
>
> - if (!subsys || ctrl->instance != subsys->instance)
> + if (ctrl->instance >= 0 &&
> + (!subsys || ctrl->instance != subsys->instance))
> ida_free(&nvme_instance_ida, ctrl->instance);
> key_put(ctrl->tls_key);
> nvme_free_cels(ctrl);
> nvme_mpath_uninit(ctrl);
> nvme_auth_stop(ctrl);
> nvme_auth_free(ctrl);
> - __free_page(ctrl->discard_page);
> + if (ctrl->discard_page)
> + __free_page(ctrl->discard_page);
> free_opal_dev(ctrl->opal_dev);
>
> if (subsys) {
> @@ -4610,6 +4612,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
> clear_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags);
> spin_lock_init(&ctrl->lock);
> mutex_init(&ctrl->scan_lock);
> + mutex_init(&ctrl->dhchap_auth_mutex);
> INIT_LIST_HEAD(&ctrl->namespaces);
> xa_init(&ctrl->cels);
> init_rwsem(&ctrl->namespaces_rwsem);
> @@ -4621,6 +4624,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
> INIT_WORK(&ctrl->async_event_work, nvme_async_event_work);
> INIT_WORK(&ctrl->fw_act_work, nvme_fw_act_work);
> INIT_WORK(&ctrl->delete_work, nvme_delete_ctrl_work);
> + INIT_WORK(&ctrl->dhchap_auth_work, nvme_ctrl_auth_work);
> +
> init_waitqueue_head(&ctrl->state_wq);
>
> INIT_DELAYED_WORK(&ctrl->ka_work, nvme_keep_alive_work);
> @@ -4631,16 +4636,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>
> BUILD_BUG_ON(NVME_DSM_MAX_RANGES * sizeof(struct nvme_dsm_range) >
> PAGE_SIZE);
> - ctrl->discard_page = alloc_page(GFP_KERNEL);
> - if (!ctrl->discard_page) {
> - ret = -ENOMEM;
> - goto out;
> - }
>
> - ret = ida_alloc(&nvme_instance_ida, GFP_KERNEL);
> - if (ret < 0)
> - goto out;
> - ctrl->instance = ret;
> + ctrl->instance = -1;
>
> device_initialize(&ctrl->ctrl_device);
> ctrl->device = &ctrl->ctrl_device;
> @@ -4654,16 +4651,28 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
> ctrl->device->groups = nvme_dev_attr_groups;
> ctrl->device->release = nvme_free_ctrl;
> dev_set_drvdata(ctrl->device, ctrl);
> +
> + ret = ida_alloc(&nvme_instance_ida, GFP_KERNEL);
> + if (ret < 0)
> + goto out;
> +
> + ctrl->instance = ret;
> ret = dev_set_name(ctrl->device, "nvme%d", ctrl->instance);
> if (ret)
> - goto out_release_instance;
> + goto out;
> +
> + ctrl->discard_page = alloc_page(GFP_KERNEL);
> + if (!ctrl->discard_page) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> nvme_get_ctrl(ctrl);
> cdev_init(&ctrl->cdev, &nvme_dev_fops);
> ctrl->cdev.owner = ops->module;
> ret = cdev_device_add(&ctrl->cdev, ctrl->device);
> if (ret)
> - goto out_free_name;
> + goto out_put_ctrl;
>
> /*
> * Initialize latency tolerance controls. The sysfs files won't
> @@ -4684,14 +4693,9 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
> nvme_fault_inject_fini(&ctrl->fault_inject);
> dev_pm_qos_hide_latency_tolerance(ctrl->device);
> cdev_device_del(&ctrl->cdev, ctrl->device);
> -out_free_name:
> +out_put_ctrl:
> nvme_put_ctrl(ctrl);
> - kfree_const(ctrl->device->kobj.name);
> -out_release_instance:
> - ida_free(&nvme_instance_ida, ctrl->instance);
> out:
> - if (ctrl->discard_page)
> - __free_page(ctrl->discard_page);
> return ret;
> }
> EXPORT_SYMBOL_GPL(nvme_init_ctrl);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index cacc56f4bbf4..b0e378e6ca63 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -1124,6 +1124,7 @@ static inline bool nvme_ctrl_sgl_supported(struct nvme_ctrl *ctrl)
> #ifdef CONFIG_NVME_HOST_AUTH
> int __init nvme_init_auth(void);
> void __exit nvme_exit_auth(void);
> +void nvme_ctrl_auth_work(struct work_struct *work);
> int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl);
> void nvme_auth_stop(struct nvme_ctrl *ctrl);
> int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid);
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 51a62b0c645a..bc749bebe134 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -2302,7 +2302,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
> ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_rdma_ctrl_ops,
> 0 /* no quirks, we're perfect! */);
> if (ret)
> - goto out_kfree_queues;
> + goto out_put_ctrl;
>
> changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING);
> WARN_ON_ONCE(!changed);
> @@ -2322,12 +2322,11 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>
> out_uninit_ctrl:
> nvme_uninit_ctrl(&ctrl->ctrl);
> +out_put_ctrl:
> nvme_put_ctrl(&ctrl->ctrl);
> if (ret > 0)
> ret = -EIO;
> return ERR_PTR(ret);
> -out_kfree_queues:
> - kfree(ctrl->queues);
> out_free_ctrl:
> kfree(ctrl);
> return ERR_PTR(ret);
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 8b5e4327fe83..9787a4fdbb00 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2759,7 +2759,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
>
> ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_tcp_ctrl_ops, 0);
> if (ret)
> - goto out_kfree_queues;
> + goto out_put_ctrl;
>
> if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
> WARN_ON_ONCE(1);
> @@ -2782,12 +2782,11 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
>
> out_uninit_ctrl:
> nvme_uninit_ctrl(&ctrl->ctrl);
> +out_put_ctrl:
> nvme_put_ctrl(&ctrl->ctrl);
> if (ret > 0)
> ret = -EIO;
> return ERR_PTR(ret);
> -out_kfree_queues:
> - kfree(ctrl->queues);
> out_free_ctrl:
> kfree(ctrl);
> return ERR_PTR(ret);
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index e589915ddef8..6d1359915b6c 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -550,10 +550,8 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
>
> ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_loop_ctrl_ops,
> 0 /* no quirks, we're perfect! */);
> - if (ret) {
> - kfree(ctrl);
> + if (ret)
> goto out;
> - }
>
> if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
> WARN_ON_ONCE(1);
> @@ -611,8 +609,8 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
> kfree(ctrl->queues);
> out_uninit_ctrl:
> nvme_uninit_ctrl(&ctrl->ctrl);
> - nvme_put_ctrl(&ctrl->ctrl);
> out:
> + nvme_put_ctrl(&ctrl->ctrl);
> if (ret > 0)
> ret = -EIO;
> return ERR_PTR(ret);
> --
> 2.43.0
>
Hmm. Don't really like it.
We took great pains of moving then entire DH-CHAP code into its own
file, and this patch reverts it and is causing the DH-CHAP code to be
littered all over the place.
Cheers,
Hannes
More information about the Linux-nvme
mailing list