Re: blktests nvme 041,042 leak memory

[Sagi Grimberg <sagi@grimberg.me>]

On 29/05/2024 19:25, Keith Busch wrote:
> On Wed, May 29, 2024 at 03:51:09PM +0300, Sagi Grimberg wrote:
>> But this is ugly...
> I agree. Could we not work around the device model instead? Once we call
> device_add successfully, we really need to pair that with a device_del.
>
> I see two ways we might be able to do this.
>
> First suggestion: move the device_add to the end so we don't have to
> worry about cleaning up if subsequence actions fail. And auth_init_ctrl
> doesn't appear to have any dependency on the ctrl->device anyway.
>
> ---
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f5d150c62955d..d86db6a193fcb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4675,12 +4675,16 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>   	if (ret)
>   		goto out_release_instance;
>   
> +	ret = nvme_auth_init_ctrl(ctrl);
> +	if (ret)
> +		goto out_free_name;
> +
>   	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 auth_free;
>   
>   	/*
>   	 * Initialize latency tolerance controls.  The sysfs files won't
> @@ -4692,15 +4696,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>   
>   	nvme_fault_inject_init(&ctrl->fault_inject, dev_name(ctrl->device));
>   	nvme_mpath_init_ctrl(ctrl);
> -	ret = nvme_auth_init_ctrl(ctrl);
> -	if (ret)
> -		goto out_free_cdev;
>   
>   	return 0;
> -out_free_cdev:
> -	nvme_fault_inject_fini(&ctrl->fault_inject);
> -	dev_pm_qos_hide_latency_tolerance(ctrl->device);
> -	cdev_device_del(&ctrl->cdev, ctrl->device);
> +auth_free:
> +	nvme_auth_free(ctrl);
>   out_free_name:
>   	nvme_put_ctrl(ctrl);
>   	kfree_const(ctrl->device->kobj.name);
> --

Lets just do that... and add a comment that we don't want to add
function calls that can fail post cdev_device_add() because we have a
cleanup problem (with a short explanation), so this is less likely to 
come back...