blktests nvme 041,042 leak memory

[Keith Busch]

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);
--

Second suggestion, let the error handling release the last reference.
The trick here is to not call the "free_ctrl" callback on "new"
controllers, and don't take a ctrl reference until the end.

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f5d150c62955d..72c9693e1df61 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4601,7 +4601,8 @@ static void nvme_free_ctrl(struct device *dev)
 		mutex_unlock(&nvme_subsystems_lock);
 	}
 
-	ctrl->ops->free_ctrl(ctrl);
+	if (nvme_ctrl_state(ctrl) != NVME_CTRL_NEW)
+		ctrl->ops->free_ctrl(ctrl);
 
 	if (subsys)
 		nvme_put_subsystem(subsys);
@@ -4675,7 +4676,6 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	if (ret)
 		goto out_release_instance;
 
-	nvme_get_ctrl(ctrl);
 	cdev_init(&ctrl->cdev, &nvme_dev_fops);
 	ctrl->cdev.owner = ops->module;
 	ret = cdev_device_add(&ctrl->cdev, ctrl->device);
@@ -4696,13 +4696,16 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	if (ret)
 		goto out_free_cdev;
 
+	nvme_get_ctrl(ctrl);
 	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);
-out_free_name:
 	nvme_put_ctrl(ctrl);
+	return ret;
+
+out_free_name:
 	kfree_const(ctrl->device->kobj.name);
 out_release_instance:
 	ida_free(&nvme_instance_ida, ctrl->instance);
--