block: add error handling for device_add_disk / add_disk

Properly unwind on errors in device_add_disk.  This is the initial work
as drivers are not converted yet, which will follow in separate patches.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
[hch: major rebase.  All bugs are probably mine]
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Link: https://lore.kernel.org/r/20210818144542.19305-10-hch@lst.de
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/block/genhd.c b/block/genhd.c
index a54b4849242c41dfd1a032aab676406acee0685c..a925f773145f046be0aecef496d845f4b1169de7 100644 (file)
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -417,11 +417,8 @@ static void disk_scan_partitions(struct gendisk *disk)
  *
  * This function registers the partitioning information in @disk
  * with the kernel.
  *
  * This function registers the partitioning information in @disk
  * with the kernel.

- *
- * FIXME: error handling

  */
  */

-
-void device_add_disk(struct device *parent, struct gendisk *disk,
+int device_add_disk(struct device *parent, struct gendisk *disk,

                     const struct attribute_group **groups)
 
 {
                     const struct attribute_group **groups)
 
 {


@@ -444,7 +441,8 @@ void device_add_disk(struct device *parent, struct gendisk *disk,
         * and all partitions from the extended dev_t space.
         */
        if (disk->major) {
         * and all partitions from the extended dev_t space.
         */
        if (disk->major) {

-               WARN_ON(!disk->minors);
+               if (WARN_ON(!disk->minors))
+                       return -EINVAL;

 
                if (disk->minors > DISK_MAX_PARTS) {
                        pr_err("block: can't allocate more than %d partitions\n",
 
                if (disk->minors > DISK_MAX_PARTS) {
                        pr_err("block: can't allocate more than %d partitions\n",


@@ -452,19 +450,20 @@ void device_add_disk(struct device *parent, struct gendisk *disk,
                        disk->minors = DISK_MAX_PARTS;
                }
        } else {
                        disk->minors = DISK_MAX_PARTS;
                }
        } else {

-               WARN_ON(disk->minors);
+               if (WARN_ON(disk->minors))
+                       return -EINVAL;

 
                ret = blk_alloc_ext_minor();
 
                ret = blk_alloc_ext_minor();

-               if (ret < 0) {
-                       WARN_ON(1);
-                       return;
-               }
+               if (ret < 0)
+                       return ret;

                disk->major = BLOCK_EXT_MAJOR;
                disk->first_minor = MINOR(ret);
                disk->flags |= GENHD_FL_EXT_DEVT;
        }
 
                disk->major = BLOCK_EXT_MAJOR;
                disk->first_minor = MINOR(ret);
                disk->flags |= GENHD_FL_EXT_DEVT;
        }
 

-       disk_alloc_events(disk);
+       ret = disk_alloc_events(disk);
+       if (ret)
+               goto out_free_ext_minor;

 
        /* delay uevents, until we scanned partition table */
        dev_set_uevent_suppress(ddev, 1);
 
        /* delay uevents, until we scanned partition table */
        dev_set_uevent_suppress(ddev, 1);


@@ -474,15 +473,14 @@ void device_add_disk(struct device *parent, struct gendisk *disk,
        dev_set_name(ddev, "%s", disk->disk_name);
        if (!(disk->flags & GENHD_FL_HIDDEN))
                ddev->devt = MKDEV(disk->major, disk->first_minor);
        dev_set_name(ddev, "%s", disk->disk_name);
        if (!(disk->flags & GENHD_FL_HIDDEN))
                ddev->devt = MKDEV(disk->major, disk->first_minor);

-       if (device_add(ddev))
-               return;
+       ret = device_add(ddev);
+       if (ret)
+               goto out_disk_release_events;

        if (!sysfs_deprecated) {
                ret = sysfs_create_link(block_depr, &ddev->kobj,
                                        kobject_name(&ddev->kobj));
        if (!sysfs_deprecated) {
                ret = sysfs_create_link(block_depr, &ddev->kobj,
                                        kobject_name(&ddev->kobj));

-               if (ret) {
-                       device_del(ddev);
-                       return;
-               }
+               if (ret)
+                       goto out_device_del;

        }
 
        /*
        }
 
        /*


@@ -492,23 +490,25 @@ void device_add_disk(struct device *parent, struct gendisk *disk,
         */
        pm_runtime_set_memalloc_noio(ddev, true);
 
         */
        pm_runtime_set_memalloc_noio(ddev, true);
 

-       blk_integrity_add(disk);
+       ret = blk_integrity_add(disk);
+       if (ret)
+               goto out_del_block_link;

 
        disk->part0->bd_holder_dir =
                kobject_create_and_add("holders", &ddev->kobj);
 
        disk->part0->bd_holder_dir =
                kobject_create_and_add("holders", &ddev->kobj);

+       if (!disk->part0->bd_holder_dir)
+               goto out_del_integrity;

        disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj);
        disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj);

+       if (!disk->slave_dir)
+               goto out_put_holder_dir;

 
 

-       /*
-        * XXX: this is a mess, can't wait for real error handling in add_disk.
-        * Make sure ->slave_dir is NULL if we failed some of the registration
-        * so that the cleanup in bd_unlink_disk_holder works properly.
-        */
-       if (bd_register_pending_holders(disk) < 0) {
-               kobject_put(disk->slave_dir);
-               disk->slave_dir = NULL;
-       }
+       ret = bd_register_pending_holders(disk);
+       if (ret < 0)
+               goto out_put_slave_dir;

 
 

-       blk_register_queue(disk);
+       ret = blk_register_queue(disk);
+       if (ret)
+               goto out_put_slave_dir;

 
        if (disk->flags & GENHD_FL_HIDDEN) {
                /*
 
        if (disk->flags & GENHD_FL_HIDDEN) {
                /*


@@ -520,13 +520,13 @@ void device_add_disk(struct device *parent, struct gendisk *disk,
        } else {
                ret = bdi_register(disk->bdi, "%u:%u",
                                   disk->major, disk->first_minor);
        } else {
                ret = bdi_register(disk->bdi, "%u:%u",
                                   disk->major, disk->first_minor);

-               WARN_ON(ret);
+               if (ret)
+                       goto out_unregister_queue;

                bdi_set_owner(disk->bdi, ddev);
                bdi_set_owner(disk->bdi, ddev);

-               if (disk->bdi->dev) {
-                       ret = sysfs_create_link(&ddev->kobj,
-                                               &disk->bdi->dev->kobj, "bdi");
-                       WARN_ON(ret);
-               }
+               ret = sysfs_create_link(&ddev->kobj,
+                                       &disk->bdi->dev->kobj, "bdi");
+               if (ret)
+                       goto out_unregister_bdi;

 
                bdev_add(disk->part0, ddev->devt);
                disk_scan_partitions(disk);
 
                bdev_add(disk->part0, ddev->devt);
                disk_scan_partitions(disk);


@@ -541,6 +541,30 @@ void device_add_disk(struct device *parent, struct gendisk *disk,
 
        disk_update_readahead(disk);
        disk_add_events(disk);
 
        disk_update_readahead(disk);
        disk_add_events(disk);

+       return 0;
+
+out_unregister_bdi:
+       if (!(disk->flags & GENHD_FL_HIDDEN))
+               bdi_unregister(disk->bdi);
+out_unregister_queue:
+       blk_unregister_queue(disk);
+out_put_slave_dir:
+       kobject_put(disk->slave_dir);
+out_put_holder_dir:
+       kobject_put(disk->part0->bd_holder_dir);
+out_del_integrity:
+       blk_integrity_del(disk);
+out_del_block_link:
+       if (!sysfs_deprecated)
+               sysfs_remove_link(block_depr, dev_name(ddev));
+out_device_del:
+       device_del(ddev);
+out_disk_release_events:
+       disk_release_events(disk);
+out_free_ext_minor:
+       if (disk->major == BLOCK_EXT_MAJOR)
+               blk_free_ext_minor(disk->first_minor);
+       return WARN_ON_ONCE(ret); /* keep until all callers handle errors */

 }
 EXPORT_SYMBOL(device_add_disk);
 
 }
 EXPORT_SYMBOL(device_add_disk);
 

diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 55acefdd8a204dbef60d04b61112344262c53acd..c68d83c87f83fc9c0e3e946c5ccbd895e4d97724 100644 (file)
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -214,11 +214,11 @@ static inline dev_t disk_devt(struct gendisk *disk)
 void disk_uevent(struct gendisk *disk, enum kobject_action action);
 
 /* block/genhd.c */
 void disk_uevent(struct gendisk *disk, enum kobject_action action);
 
 /* block/genhd.c */

-extern void device_add_disk(struct device *parent, struct gendisk *disk,
-                           const struct attribute_group **groups);
-static inline void add_disk(struct gendisk *disk)
+int device_add_disk(struct device *parent, struct gendisk *disk,
+               const struct attribute_group **groups);
+static inline int add_disk(struct gendisk *disk)

 {
 {

-       device_add_disk(NULL, disk, NULL);
+       return device_add_disk(NULL, disk, NULL);

 }
 extern void del_gendisk(struct gendisk *gp);
 
 }
 extern void del_gendisk(struct gendisk *gp);