Re: [PATCH] block: fix error handling for device_add_disk

[Christoph Hellwig <hch@lst.de>]

On Fri, Dec 17, 2021 at 07:37:43PM +0900, Tetsuo Handa wrote:
> Well, I don't think that we can remove this blk_free_ext_minor() call, for
> this call is releasing disk->first_minor rather than MINOR(bdev->bd_dev).
> 
> Since bdev_add(disk->part0, MKDEV(disk->major, disk->first_minor)) is not
> called when reaching the out_free_ext_minor label,
> 
> 	if (MAJOR(bdev->bd_dev) == BLOCK_EXT_MAJOR)
> 		blk_free_ext_minor(MINOR(bdev->bd_dev));
> 
> in bdev_free_inode() will not be called because MAJOR(bdev->bd_dev) == 0
> because bdev->bd_dev == 0.
> 
> I think we can apply this patch as-is...

With the patch as-is we'll still leak disk->ev if device_add fails.
Something like the patch below should solve that by moving the disk->ev
allocation later and always cleaning it up through disk->release:


diff --git a/block/genhd.c b/block/genhd.c
index 3c139a1b6f049..3e4bbfa3e1c24 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -442,10 +442,6 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
 		disk->first_minor = ret;
 	}
 
-	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);
 
@@ -456,7 +452,12 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
 		ddev->devt = MKDEV(disk->major, disk->first_minor);
 	ret = device_add(ddev);
 	if (ret)
-		goto out_disk_release_events;
+		goto out_free_ext_minor;
+
+	ret = disk_alloc_events(disk);
+	if (ret)
+		goto out_device_del;
+
 	if (!sysfs_deprecated) {
 		ret = sysfs_create_link(block_depr, &ddev->kobj,
 					kobject_name(&ddev->kobj));
@@ -538,8 +539,7 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
 		sysfs_remove_link(block_depr, dev_name(ddev));
 out_device_del:
 	device_del(ddev);
-out_disk_release_events:
-	disk_release_events(disk);
+	return ret;
 out_free_ext_minor:
 	if (disk->major == BLOCK_EXT_MAJOR)
 		blk_free_ext_minor(disk->first_minor);