nvme: fix crash and memory leak during invalid cdev teardown

In the NVMe multipath code, if nvme_add_ns_head_cdev() fails during
nvme_mpath_set_live(), the error is ignored. However, during teardown,
nvme_remove_head() unconditionally calls nvme_cdev_del(). This teardown
asymmetry leads to a kernel panic if the character device was never
successfully initialized.

BUG: kernel NULL pointer dereference, address: 00000000000000d0
 device_del+0x39/0x3c0
 cdev_device_del+0x15/0x50
 nvme_cdev_del+0xe/0x20 [nvme_core]
 nvme_mpath_shutdown_disk+0x38/0x60 [nvme_core]
 nvme_ns_remove+0x177/0x1f0 [nvme_core]
 nvme_remove_namespaces+0xdc/0x130 [nvme_core]
 nvme_do_delete_ctrl+0x71/0xd0 [nvme_core]

Additionally, a memory leak exists in the nvme_cdev_add() failure path.
Previously, dev_set_name() was called before ida_alloc(). If ida_alloc()
subsequently failed, device_initialize() was never called, meaning
put_device() could not be used to clean up the kobject, leaking the
memory allocated by dev_set_name().

*  Introduces the NVME_NSHEAD_CDEV_LIVE and NVME_NS_CDEV_LIVE bits to track
   the successful creation of the character devices. Teardown routines now
   check these bits before attempting deletion.
*  Refactor nvme_cdev_add() to accept the formatted device name as a
   parameter, moving dev_set_name() after the IDA allocation and
   immediately before device_initialize(). This ensures any internally
   allocated strings are safely cleaned up by put_device() upon failure.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d37fc70fe48aa46e4ba839cb07a344f2bcdf74ec..c6930e43cfea2361b0b544fb019e3e96a11e7f2f 100644 (file)
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3894,7 +3894,8 @@ void nvme_cdev_del(struct cdev *cdev, struct device *cdev_device)
        put_device(cdev_device);
 }
 
-int nvme_cdev_add(struct cdev *cdev, struct device *cdev_device,
+int nvme_cdev_add(const char *name, struct cdev *cdev,
+               struct device *cdev_device,
                const struct file_operations *fops, struct module *owner)
 {
        int minor, ret;
@@ -3902,6 +3903,12 @@ int nvme_cdev_add(struct cdev *cdev, struct device *cdev_device,
        minor = ida_alloc(&nvme_ns_chr_minor_ida, GFP_KERNEL);
        if (minor < 0)
                return minor;
+
+       ret = dev_set_name(cdev_device, name);
+       if (ret) {
+               ida_free(&nvme_ns_chr_minor_ida, minor);
+               return ret;
+       }
        cdev_device->devt = MKDEV(MAJOR(nvme_ns_chr_devt), minor);
        cdev_device->class = &nvme_ns_chr_class;
        cdev_device->release = nvme_cdev_rel;
@@ -3939,15 +3946,21 @@ static const struct file_operations nvme_ns_chr_fops = {
 static int nvme_add_ns_cdev(struct nvme_ns *ns)
 {
        int ret;
+       char name[32];
 
        ns->cdev_device.parent = ns->ctrl->device;
-       ret = dev_set_name(&ns->cdev_device, "ng%dn%d",
-                          ns->ctrl->instance, ns->head->instance);
-       if (ret)
-               return ret;
+       snprintf(name, sizeof(name), "ng%dn%d", ns->ctrl->instance,
+                ns->head->instance);
 
-       return nvme_cdev_add(&ns->cdev, &ns->cdev_device, &nvme_ns_chr_fops,
-                            ns->ctrl->ops->module);
+       ret = nvme_cdev_add(name, &ns->cdev, &ns->cdev_device,
+                           &nvme_ns_chr_fops, ns->ctrl->ops->module);
+       if (ret) {
+               dev_err(ns->ctrl->device, "Unable to create the %s device\n",
+                       name);
+       } else {
+               set_bit(NVME_NS_CDEV_LIVE, &ns->flags);
+       }
+       return ret;
 }
 
 static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
@@ -4323,8 +4336,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
        /* guarantee not available in head->list */
        synchronize_srcu(&ns->head->srcu);
 
-       if (!nvme_ns_head_multipath(ns->head))
-               nvme_cdev_del(&ns->cdev, &ns->cdev_device);
+       if (!nvme_ns_head_multipath(ns->head)) {
+               if (test_and_clear_bit(NVME_NS_CDEV_LIVE, &ns->flags))
+                       nvme_cdev_del(&ns->cdev, &ns->cdev_device);
+       }
 
        nvme_mpath_remove_sysfs_link(ns);
 

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index e033ede953cce1071f7244baa9efa4357a4235aa..9cd49e2f760d0a7620074c410c0552f541b43507 100644 (file)
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -642,14 +642,20 @@ static const struct file_operations nvme_ns_head_chr_fops = {
 static int nvme_add_ns_head_cdev(struct nvme_ns_head *head)
 {
        int ret;
+       char name[32];
 
        head->cdev_device.parent = &head->subsys->dev;
-       ret = dev_set_name(&head->cdev_device, "ng%dn%d",
-                          head->subsys->instance, head->instance);
-       if (ret)
-               return ret;
-       ret = nvme_cdev_add(&head->cdev, &head->cdev_device,
+       snprintf(name, sizeof(name), "ng%dn%d", head->subsys->instance,
+                head->instance);
+
+       ret = nvme_cdev_add(name, &head->cdev, &head->cdev_device,
                            &nvme_ns_head_chr_fops, THIS_MODULE);
+       if (ret) {
+               dev_err(disk_to_dev(head->disk),
+                       "Unable to create the %s device\n", name);
+       } else {
+               set_bit(NVME_NSHEAD_CDEV_LIVE, &head->flags);
+       }
        return ret;
 }
 
@@ -694,7 +700,8 @@ static void nvme_remove_head(struct nvme_ns_head *head)
                 */
                kblockd_schedule_work(&head->requeue_work);
 
-               nvme_cdev_del(&head->cdev, &head->cdev_device);
+               if (test_and_clear_bit(NVME_NSHEAD_CDEV_LIVE, &head->flags))
+                       nvme_cdev_del(&head->cdev, &head->cdev_device);
                synchronize_srcu(&head->srcu);
                del_gendisk(head->disk);
        }

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index b367c67dcb37f225d55d9192b94fe528d358952f..824651cc898dbee2815109224a3dfbbee47fe997 100644 (file)
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -573,6 +573,7 @@ struct nvme_ns_head {
        atomic_long_t           io_fail_no_available_path_count;
 #define NVME_NSHEAD_DISK_LIVE          0
 #define NVME_NSHEAD_QUEUE_IF_NO_PATH   1
+#define NVME_NSHEAD_CDEV_LIVE          2
        struct nvme_ns __rcu    *current_path[];
 #endif
 };
@@ -611,6 +612,7 @@ struct nvme_ns {
 #define NVME_NS_FORCE_RO               3
 #define NVME_NS_READY                  4
 #define NVME_NS_SYSFS_ATTR_LINK        5
+#define NVME_NS_CDEV_LIVE              6
 
        struct cdev             cdev;
        struct device           cdev_device;
@@ -995,7 +997,8 @@ int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp, u8 csi,
                void *log, size_t size, u64 offset);
 bool nvme_tryget_ns_head(struct nvme_ns_head *head);
 void nvme_put_ns_head(struct nvme_ns_head *head);
-int nvme_cdev_add(struct cdev *cdev, struct device *cdev_device,
+int nvme_cdev_add(const char *name, struct cdev *cdev,
+               struct device *cdev_device,
                const struct file_operations *fops, struct module *owner);
 void nvme_cdev_del(struct cdev *cdev, struct device *cdev_device);
 int nvme_ioctl(struct block_device *bdev, blk_mode_t mode,