Re: blktests nvme 041,042 leak memory

[Hannes Reinecke <hare@suse.de>]

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