nvme: split device add from initialization

Combining both creates an ambiguous cleanup scenario for the caller if
an error is returned: does the device reference need to be dropped or
did the error occur before the device was initialized? If an error
occurs after the device is added, then the existing cleanup routines
will leak memory.

Furthermore, the nvme core is taking it upon itself to free the device's
kobj name under certain conditions rather than go through the core
device API. We shouldn't be peaking into these implementation details.

Split the device initialization from the addition to make it easier to
know the error handling actions, fix the existing memory leaks, and stop
the device layering violations.

Link: https://lore.kernel.org/linux-nvme/c4050a37-ecc9-462c-9772-65e25166f439@grimberg.me/
Tested-by: Yi Zhang <yi.zhang@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index c43ada920c3b279111e8c9183329843b39b8de0a..42ba7fbc749f35b3184a1d85d10de7dc90619520 100644 (file)
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -1531,6 +1531,10 @@ static int apple_nvme_probe(struct platform_device *pdev)
        if (IS_ERR(anv))
                return PTR_ERR(anv);
 
+       ret = nvme_add_ctrl(&anv->ctrl);
+       if (ret)
+               goto out_put_ctrl;
+
        anv->ctrl.admin_q = blk_mq_alloc_queue(&anv->admin_tagset, NULL, NULL);
        if (IS_ERR(anv->ctrl.admin_q)) {
                ret = -ENOMEM;
@@ -1545,6 +1549,7 @@ static int apple_nvme_probe(struct platform_device *pdev)
 
 out_uninit_ctrl:
        nvme_uninit_ctrl(&anv->ctrl);
+out_put_ctrl:
        nvme_put_ctrl(&anv->ctrl);
        return ret;
 }

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 89ebfa89613ee6a0ff49f37743ca2d8a1786436d..80d8460bb92f23d184ba8bf113cb73b76f045650 100644 (file)
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4667,6 +4667,9 @@ static void nvme_free_ctrl(struct device *dev)
  * Initialize a NVMe controller structures.  This needs to be called during
  * earliest initialization so that we have the initialized structured around
  * during probing.
+ *
+ * On success, the caller must use the nvme_put_ctrl() to release this when
+ * needed, which also invokes the ops->free_ctrl() callback.
  */
 int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
                const struct nvme_ctrl_ops *ops, unsigned long quirks)
@@ -4715,6 +4718,12 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
                goto out;
        ctrl->instance = ret;
 
+       ret = nvme_auth_init_ctrl(ctrl);
+       if (ret)
+               goto out_release_instance;
+
+       nvme_mpath_init_ctrl(ctrl);
+
        device_initialize(&ctrl->ctrl_device);
        ctrl->device = &ctrl->ctrl_device;
        ctrl->device->devt = MKDEV(MAJOR(nvme_ctrl_base_chr_devt),
@@ -4727,16 +4736,36 @@ 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);
+
+       return ret;
+
+out_release_instance:
+       ida_free(&nvme_instance_ida, ctrl->instance);
+out:
+       if (ctrl->discard_page)
+               __free_page(ctrl->discard_page);
+       cleanup_srcu_struct(&ctrl->srcu);
+       return ret;
+}
+EXPORT_SYMBOL_GPL(nvme_init_ctrl);
+
+/*
+ * On success, returns with an elevated controller reference and caller must
+ * use nvme_uninit_ctrl() to properly free resources associated with the ctrl.
+ */
+int nvme_add_ctrl(struct nvme_ctrl *ctrl)
+{
+       int ret;
+
        ret = dev_set_name(ctrl->device, "nvme%d", ctrl->instance);
        if (ret)
-               goto out_release_instance;
+               return ret;
 
-       nvme_get_ctrl(ctrl);
        cdev_init(&ctrl->cdev, &nvme_dev_fops);
-       ctrl->cdev.owner = ops->module;
+       ctrl->cdev.owner = ctrl->ops->module;
        ret = cdev_device_add(&ctrl->cdev, ctrl->device);
        if (ret)
-               goto out_free_name;
+               return ret;
 
        /*
         * Initialize latency tolerance controls.  The sysfs files won't
@@ -4747,28 +4776,11 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
                min(default_ps_max_latency_us, (unsigned long)S32_MAX));
 
        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;
+       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);
-       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);
-       cleanup_srcu_struct(&ctrl->srcu);
-       return ret;
 }
-EXPORT_SYMBOL_GPL(nvme_init_ctrl);
+EXPORT_SYMBOL_GPL(nvme_add_ctrl);
 
 /* let I/O to all namespaces fail in preparation for surprise removal */
 void nvme_mark_namespaces_dead(struct nvme_ctrl *ctrl)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index c52446013388f5dd62b5e78d2552d91a053a7b4f..d5a383766b34d72b325c4a5d1a6aceb0b1c35f60 100644 (file)
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3563,6 +3563,10 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
        if (IS_ERR(ctrl))
                return ERR_CAST(ctrl);
 
+       ret = nvme_add_ctrl(&ctrl->ctrl);
+       if (ret)
+               goto out_put_ctrl;
+
        ret = nvme_alloc_admin_tag_set(&ctrl->ctrl, &ctrl->admin_tag_set,
                        &nvme_fc_admin_mq_ops,
                        struct_size_t(struct nvme_fcp_op_w_sgl, priv,
@@ -3607,6 +3611,7 @@ fail_ctrl:
        /* initiate nvme ctrl ref counting teardown */
        nvme_uninit_ctrl(&ctrl->ctrl);
 
+out_put_ctrl:
        /* Remove core ctrl ref. */
        nvme_put_ctrl(&ctrl->ctrl);
 

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index f3a41133ac3f9745ee98fcc109710ee8a2988cc2..969349068086655aa77295e5dddf644ac4f5e92f 100644 (file)
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -792,6 +792,7 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown);
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl);
 int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
                const struct nvme_ctrl_ops *ops, unsigned long quirks);
+int nvme_add_ctrl(struct nvme_ctrl *ctrl);
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
 void nvme_start_ctrl(struct nvme_ctrl *ctrl);
 void nvme_stop_ctrl(struct nvme_ctrl *ctrl);

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 102a9fb0c65fff7dcbbba97f01d93f54d547b063..c92125b0238d480432cd728cf66044fd7bb81fcc 100644 (file)
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3015,6 +3015,10 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        if (IS_ERR(dev))
                return PTR_ERR(dev);
 
+       result = nvme_add_ctrl(&dev->ctrl);
+       if (result)
+               goto out_put_ctrl;
+
        result = nvme_dev_map(dev);
        if (result)
                goto out_uninit_ctrl;
@@ -3101,6 +3105,7 @@ out_dev_unmap:
        nvme_dev_unmap(dev);
 out_uninit_ctrl:
        nvme_uninit_ctrl(&dev->ctrl);
+out_put_ctrl:
        nvme_put_ctrl(&dev->ctrl);
        return result;
 }

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 94d4f3dbac6b64e5384cd15eace5598e27b34bc7..5c44c7c5c688c2bd31aab60355f5d37616549b9e 100644 (file)
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2323,6 +2323,10 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
        if (IS_ERR(ctrl))
                return ERR_CAST(ctrl);
 
+       ret = nvme_add_ctrl(&ctrl->ctrl);
+       if (ret)
+               goto out_put_ctrl;
+
        changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING);
        WARN_ON_ONCE(!changed);
 
@@ -2341,6 +2345,7 @@ 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;

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 5ee3bbc67f411e7148df79b00ac989d8665a77f8..3be67c98c906e6eda180c5d5f888f1a381ad9215 100644 (file)
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2779,6 +2779,10 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
        if (IS_ERR(ctrl))
                return ERR_CAST(ctrl);
 
+       ret = nvme_add_ctrl(&ctrl->ctrl);
+       if (ret)
+               goto out_put_ctrl;
+
        if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
                WARN_ON_ONCE(1);
                ret = -EINTR;
@@ -2800,6 +2804,7 @@ 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;

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index e589915ddef85cf5f67fcba50deed724b37616d3..e32790d8fc260c8a2146bc925a974aa4cea90e04 100644 (file)
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -555,6 +555,10 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
                goto out;
        }
 
+       ret = nvme_add_ctrl(&ctrl->ctrl);
+       if (ret)
+               goto out_put_ctrl;
+
        if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
                WARN_ON_ONCE(1);
 
@@ -611,6 +615,7 @@ out_free_queues:
        kfree(ctrl->queues);
 out_uninit_ctrl:
        nvme_uninit_ctrl(&ctrl->ctrl);
+out_put_ctrl:
        nvme_put_ctrl(&ctrl->ctrl);
 out:
        if (ret > 0)