[RFC v1 0/6] block: add error handling for *add_disk*()

[RFC v1 0/6] block: add error handling for *add_disk*()

[Luis Chamberlain]

While working on some blktrace races I noticed that we don't do
error handling on *add_disk*() and friends. This is my initial
work on that.

This is based on linux-next tag next-20200428, you can also get this
on my branch 20200428-block-fixes [0].

Let me know what you think.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20200428-blktrace-fixes

Luis Chamberlain (6):
  block: refcount the request_queue early in __device_add_disk()
  block: move disk announce work from register_disk() to a helper
  block: move disk invalidation from del_gendisk() into a helper
  block: move disk unregistration work from del_gendisk() to a helper
  block: add initial error handling for *add_disk()* and friends
  loop: add error handling support for add_disk()

 block/blk-integrity.c |  13 +-
 block/blk-sysfs.c     |   7 +-
 block/blk.h           |   5 +-
 block/genhd.c         | 366 +++++++++++++++++++++++++++---------------
 drivers/block/loop.c  |   7 +-
 include/linux/genhd.h |  16 +-
 6 files changed, 265 insertions(+), 149 deletions(-)

-- 
2.25.1

[RFC v1 1/6] block: refcount the request_queue early in __device_add_disk()

[Luis Chamberlain]

We refcount the request_queue right now towards the end of the
__device_add_disk(), however when we add error handling on this
function we'll want to refcount the request_queue first, to help
make less complicated changes on drivers on their error paths.

For instance, today a driver may call add_disk without error handling
but still handle other errors:

int foo_block_add(...)
{
	...
	queue = blk_mq_init_queue(...);
	...
	disk->queue = queue;
	disk = alloc_disk(...);
	if (!disk)
		goto out_free_queue;
	...
        add_disk(disk);
	...
        return 0;

out_free_queue:
        blk_cleanup_queue(queue);
	/* Note: we never call put_disk() as add_disk() never failed */
	...
}

We want drivers to cleanup with put_disk() on the error path if
add_disk() fails. However, calling blk_cleanup_queue() will already
put the queue, and so the last put_disk() on the error path will
be extra. This can be simplified later if once error handling is
added to __device_add_disk(), if refcounting the request_queue
fails right away on __device_add_disk() we just return early and
set disk->NULL for the driver. That would ensure driver error
paths chug on with their error paths, and all they'd need to
expand with is the missing put_disk().

The collateral evolution for adding error paths for add_disk() becomes
larger with the alternative of replacing the blk_cleanup_queue() with
a put_disk(). We'd still need to sprinkle then some blk_cleanup_queue()
calls on the driver paths up above prior to add_disk(). And how would
we know we reached a part of add_disk() which did refcount then?

A related commit is 5a0ec388ef0f ("pktcdvd: Fix pkt_setup_dev() error
path") which *had* to take the approach of removing the blk_cleanup_queue()
because otherwise the driver crashes.

Moving this to the top ensure our future error path can easily just
handle this itself. For instance, if it was not able to refcount the
request_queue it can disk->queue to NULL, that way allowing a
blk_cleanup_queue() call followed but a put_disk(). And if the
refcount was incremented, we'd still be able to keep the same error
path of blk_cleanup_queue() followed by put_disk().

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index a933cffbee2e..5f7faaf9cc83 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -803,6 +803,12 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 	dev_t devt;
 	int retval;
 
+	/*
+	 * Take an extra ref on queue which will be put on disk_release()
+	 * so that it sticks around as long as @disk is there.
+	 */
+	WARN_ON_ONCE(!blk_get_queue(disk->queue));
+
 	/*
 	 * The disk queue should now be all set with enough information about
 	 * the device for the elevator code to pick an adequate default
@@ -854,12 +860,6 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 	if (register_queue)
 		blk_register_queue(disk);
 
-	/*
-	 * Take an extra ref on queue which will be put on disk_release()
-	 * so that it sticks around as long as @disk is there.
-	 */
-	WARN_ON_ONCE(!blk_get_queue(disk->queue));
-
 	disk_add_events(disk);
 	blk_integrity_add(disk);
 }
-- 
2.25.1

[RFC v1 2/6] block: move disk announce work from register_disk() to a helper

[Luis Chamberlain]

This moves quite a bit of code which does one thing into a helper.
We currently do not check for errors but we may decide that might
be desirable later.

This also makes the code easier to read.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 5f7faaf9cc83..091208f5f27b 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -701,13 +701,29 @@ static int exact_lock(dev_t devt, void *data)
 	return 0;
 }
 
+static void disk_announce(struct gendisk *disk)
+{
+	struct device *ddev = disk_to_dev(disk);
+	struct disk_part_iter piter;
+	struct hd_struct *part;
+
+	/* announce disk after possible partitions are created */
+	dev_set_uevent_suppress(ddev, 0);
+	kobject_uevent(&ddev->kobj, KOBJ_ADD);
+
+	/* announce possible partitions */
+	disk_part_iter_init(&piter, disk, 0);
+	while ((part = disk_part_iter_next(&piter)))
+		kobject_uevent(&part_to_dev(part)->kobj, KOBJ_ADD);
+	disk_part_iter_exit(&piter);
+}
+
 static void register_disk(struct device *parent, struct gendisk *disk,
 			  const struct attribute_group **groups)
 {
 	struct device *ddev = disk_to_dev(disk);
 	struct block_device *bdev;
-	struct disk_part_iter piter;
-	struct hd_struct *part;
+
 	int err;
 
 	ddev->parent = parent;
@@ -766,15 +782,7 @@ static void register_disk(struct device *parent, struct gendisk *disk,
 	blkdev_put(bdev, FMODE_READ);
 
 exit:
-	/* announce disk after possible partitions are created */
-	dev_set_uevent_suppress(ddev, 0);
-	kobject_uevent(&ddev->kobj, KOBJ_ADD);
-
-	/* announce possible partitions */
-	disk_part_iter_init(&piter, disk, 0);
-	while ((part = disk_part_iter_next(&piter)))
-		kobject_uevent(&part_to_dev(part)->kobj, KOBJ_ADD);
-	disk_part_iter_exit(&piter);
+	disk_announce(disk);
 
 	if (disk->queue->backing_dev_info->dev) {
 		err = sysfs_create_link(&ddev->kobj,
-- 
2.25.1

[RFC v1 3/6] block: move disk invalidation from del_gendisk() into a helper

[Luis Chamberlain]

Move the disk / partition invalidation into a helper. This will make
reading del_gendisk easier to read, in preparation for adding support
to add error handling later on register_disk() and to later share more
code with del_gendisk.

This change has no functional changes.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c | 85 +++++++++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 40 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 091208f5f27b..b4d75a15fd31 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -718,6 +718,50 @@ static void disk_announce(struct gendisk *disk)
 	disk_part_iter_exit(&piter);
 }
 
+static void invalidate_partition(struct gendisk *disk, int partno)
+{
+	struct block_device *bdev;
+
+	bdev = bdget_disk(disk, partno);
+	if (!bdev)
+		return;
+
+	fsync_bdev(bdev);
+	__invalidate_device(bdev, true);
+
+	/*
+	 * Unhash the bdev inode for this device so that it gets evicted as soon
+	 * as last inode reference is dropped.
+	 */
+	remove_inode_hash(bdev->bd_inode);
+	bdput(bdev);
+}
+
+static void disk_invalidate(struct gendisk *disk)
+{
+	struct disk_part_iter piter;
+	struct hd_struct *part;
+
+	/*
+	 * Block lookups of the disk until all bdevs are unhashed and the
+	 * disk is marked as dead (GENHD_FL_UP cleared).
+	 */
+	down_write(&disk->lookup_sem);
+	/* invalidate stuff */
+	disk_part_iter_init(&piter, disk,
+			     DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE);
+	while ((part = disk_part_iter_next(&piter))) {
+		invalidate_partition(disk, part->partno);
+		delete_partition(disk, part);
+	}
+	disk_part_iter_exit(&piter);
+
+	invalidate_partition(disk, 0);
+	set_capacity(disk, 0);
+	disk->flags &= ~GENHD_FL_UP;
+	up_write(&disk->lookup_sem);
+}
+
 static void register_disk(struct device *parent, struct gendisk *disk,
 			  const struct attribute_group **groups)
 {
@@ -886,25 +930,6 @@ void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
 }
 EXPORT_SYMBOL(device_add_disk_no_queue_reg);
 
-static void invalidate_partition(struct gendisk *disk, int partno)
-{
-	struct block_device *bdev;
-
-	bdev = bdget_disk(disk, partno);
-	if (!bdev)
-		return;
-
-	fsync_bdev(bdev);
-	__invalidate_device(bdev, true);
-
-	/*
-	 * Unhash the bdev inode for this device so that it gets evicted as soon
-	 * as last inode reference is dropped.
-	 */
-	remove_inode_hash(bdev->bd_inode);
-	bdput(bdev);
-}
-
 /**
  * del_gendisk - remove the gendisk
  * @disk: the struct gendisk to remove
@@ -926,32 +951,12 @@ static void invalidate_partition(struct gendisk *disk, int partno)
  */
 void del_gendisk(struct gendisk *disk)
 {
-	struct disk_part_iter piter;
-	struct hd_struct *part;
-
 	might_sleep();
 
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
-	/*
-	 * Block lookups of the disk until all bdevs are unhashed and the
-	 * disk is marked as dead (GENHD_FL_UP cleared).
-	 */
-	down_write(&disk->lookup_sem);
-	/* invalidate stuff */
-	disk_part_iter_init(&piter, disk,
-			     DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE);
-	while ((part = disk_part_iter_next(&piter))) {
-		invalidate_partition(disk, part->partno);
-		delete_partition(disk, part);
-	}
-	disk_part_iter_exit(&piter);
-
-	invalidate_partition(disk, 0);
-	set_capacity(disk, 0);
-	disk->flags &= ~GENHD_FL_UP;
-	up_write(&disk->lookup_sem);
+	disk_invalidate(disk);
 
 	if (!(disk->flags & GENHD_FL_HIDDEN))
 		sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
-- 
2.25.1

[RFC v1 4/6] block: move disk unregistration work from del_gendisk() to a helper

[Luis Chamberlain]

There is quite a bit of code on del_gendisk() which relates to
unregistering the disk, using register_disk() as an counter.
Move all this code into a helper instead of re-writing our own,
which we'll need later to handle errors on add_disk().

I note that register_disk() links the bdi at the end, but since
del_gendisk() deals with this before queue de-registration we'll
take a hint that's the right order that this should be done, and
we shouldn't instead strictly unwind register_disk() exactly.

We'll instead keep whatever lessons have been learned from
del_gendisk().

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index b4d75a15fd31..ed2a0eaa4e7b 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -762,6 +762,28 @@ static void disk_invalidate(struct gendisk *disk)
 	up_write(&disk->lookup_sem);
 }
 
+static void unregister_disk(struct gendisk *disk)
+{
+	/*
+	 * Remove gendisk pointer from idr so that it cannot be looked up
+	 * while RCU period before freeing gendisk is running to prevent
+	 * use-after-free issues. Note that the device number stays
+	 * "in-use" until we really free the gendisk.
+	 */
+	blk_invalidate_devt(disk_devt(disk));
+
+	kobject_put(disk->part0.holder_dir);
+	kobject_put(disk->slave_dir);
+
+	part_stat_set_all(&disk->part0, 0);
+	disk->part0.stamp = 0;
+	if (!sysfs_deprecated)
+		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
+
+	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
+	device_del(disk_to_dev(disk));
+}
+
 static void register_disk(struct device *parent, struct gendisk *disk,
 			  const struct attribute_group **groups)
 {
@@ -972,25 +994,10 @@ void del_gendisk(struct gendisk *disk)
 		WARN_ON(1);
 	}
 
+	unregister_disk(disk);
+
 	if (!(disk->flags & GENHD_FL_HIDDEN))
 		blk_unregister_region(disk_devt(disk), disk->minors);
-	/*
-	 * Remove gendisk pointer from idr so that it cannot be looked up
-	 * while RCU period before freeing gendisk is running to prevent
-	 * use-after-free issues. Note that the device number stays
-	 * "in-use" until we really free the gendisk.
-	 */
-	blk_invalidate_devt(disk_devt(disk));
-
-	kobject_put(disk->part0.holder_dir);
-	kobject_put(disk->slave_dir);
-
-	part_stat_set_all(&disk->part0, 0);
-	disk->part0.stamp = 0;
-	if (!sysfs_deprecated)
-		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
-	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
-	device_del(disk_to_dev(disk));
 }
 EXPORT_SYMBOL(del_gendisk);
 
-- 
2.25.1

[RFC v1 5/6] block: add initial error handling for *add_disk()* and friends

[Luis Chamberlain]

This adds error handling to the *add_disk*() callers and the functions
it depends on. This is initial work as drivers are not converted. That
is separate work.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/blk-integrity.c |  13 ++-
 block/blk-sysfs.c     |   7 +-
 block/blk.h           |   5 +-
 block/genhd.c         | 210 +++++++++++++++++++++++++++++-------------
 include/linux/genhd.h |  16 ++--
 5 files changed, 171 insertions(+), 80 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index ff1070edbb40..c6ceb2a1bc66 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -426,13 +426,18 @@ void blk_integrity_unregister(struct gendisk *disk)
 }
 EXPORT_SYMBOL(blk_integrity_unregister);
 
-void blk_integrity_add(struct gendisk *disk)
+int blk_integrity_add(struct gendisk *disk)
 {
-	if (kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
-				 &disk_to_dev(disk)->kobj, "%s", "integrity"))
-		return;
+	int ret;
+
+	ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
+				   &disk_to_dev(disk)->kobj, "%s", "integrity");
+	if (ret)
+		return ret;
 
 	kobject_uevent(&disk->integrity_kobj, KOBJ_ADD);
+
+	return 0;
 }
 
 void blk_integrity_del(struct gendisk *disk)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index f758a7e06671..aee2503ec120 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -939,9 +939,10 @@ int blk_register_queue(struct gendisk *disk)
 	if (WARN_ON(!q))
 		return -ENXIO;
 
-	WARN_ONCE(blk_queue_registered(q),
-		  "%s is registering an already registered queue\n",
-		  kobject_name(&dev->kobj));
+	if (WARN_ONCE(blk_queue_registered(q),
+		      "%s is registering an already registered queue\n",
+		      kobject_name(&dev->kobj)))
+		return -ENXIO;
 
 	/*
 	 * SCSI probing may synchronously create and destroy a lot of
diff --git a/block/blk.h b/block/blk.h
index 46d867a7f5bc..7c239ce14e79 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -151,7 +151,7 @@ static inline bool integrity_req_gap_front_merge(struct request *req,
 				bip_next->bip_vec[0].bv_offset);
 }
 
-void blk_integrity_add(struct gendisk *);
+int blk_integrity_add(struct gendisk *);
 void blk_integrity_del(struct gendisk *);
 #else /* CONFIG_BLK_DEV_INTEGRITY */
 static inline bool integrity_req_gap_back_merge(struct request *req,
@@ -175,8 +175,9 @@ static inline bool bio_integrity_endio(struct bio *bio)
 static inline void bio_integrity_free(struct bio *bio)
 {
 }
-static inline void blk_integrity_add(struct gendisk *disk)
+static inline int blk_integrity_add(struct gendisk *disk)
 {
+	return 0;
 }
 static inline void blk_integrity_del(struct gendisk *disk)
 {
diff --git a/block/genhd.c b/block/genhd.c
index ed2a0eaa4e7b..f3b6ed2dd4d8 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -42,8 +42,8 @@ static const struct device_type disk_type;
 
 static void disk_check_events(struct disk_events *ev,
 			      unsigned int *clearing_ptr);
-static void disk_alloc_events(struct gendisk *disk);
-static void disk_add_events(struct gendisk *disk);
+static int disk_alloc_events(struct gendisk *disk);
+static int disk_add_events(struct gendisk *disk);
 static void disk_del_events(struct gendisk *disk);
 static void disk_release_events(struct gendisk *disk);
 
@@ -669,11 +669,11 @@ static char *bdevt_str(dev_t devt, char *buf)
  * range must be nonzero
  * The hash chain is sorted on range, so that subranges can override.
  */
-void blk_register_region(dev_t devt, unsigned long range, struct module *module,
-			 struct kobject *(*probe)(dev_t, int *, void *),
-			 int (*lock)(dev_t, void *), void *data)
+int blk_register_region(dev_t devt, unsigned long range, struct module *module,
+			struct kobject *(*probe)(dev_t, int *, void *),
+			int (*lock)(dev_t, void *), void *data)
 {
-	kobj_map(bdev_map, devt, range, module, probe, lock, data);
+	return kobj_map(bdev_map, devt, range, module, probe, lock, data);
 }
 
 EXPORT_SYMBOL(blk_register_region);
@@ -784,12 +784,12 @@ static void unregister_disk(struct gendisk *disk)
 	device_del(disk_to_dev(disk));
 }
 
-static void register_disk(struct device *parent, struct gendisk *disk,
-			  const struct attribute_group **groups)
+static int __must_check register_disk(struct device *parent,
+				      struct gendisk *disk,
+				      const struct attribute_group **groups)
 {
 	struct device *ddev = disk_to_dev(disk);
-	struct block_device *bdev;
-
+	struct block_device *bdev = NULL;
 	int err;
 
 	ddev->parent = parent;
@@ -803,15 +803,26 @@ static void register_disk(struct device *parent, struct gendisk *disk,
 		WARN_ON(ddev->groups);
 		ddev->groups = groups;
 	}
-	if (device_add(ddev))
-		return;
+
+	err = device_add(ddev);
+	if (err) {
+		/*
+		 * We don't put_device(ddev) until later as we need to wait
+		 * until all the device users are unregistered as well. An
+		 * example is that we still have the device associated with a
+		 * bdi with bdi_register_owner().
+		 *
+		 * The driver issues the last put_device(ddev), however it uses
+		 * put_disk() instead.
+		 */
+		return err;
+	}
+
 	if (!sysfs_deprecated) {
 		err = sysfs_create_link(block_depr, &ddev->kobj,
 					kobject_name(&ddev->kobj));
-		if (err) {
-			device_del(ddev);
-			return;
-		}
+		if (err)
+			goto exit_del_device;
 	}
 
 	/*
@@ -826,36 +837,54 @@ static void register_disk(struct device *parent, struct gendisk *disk,
 
 	if (disk->flags & GENHD_FL_HIDDEN) {
 		dev_set_uevent_suppress(ddev, 0);
-		return;
+		return 0;
 	}
 
 	/* No minors to use for partitions */
 	if (!disk_part_scan_enabled(disk))
-		goto exit;
+		goto exit_success;
 
 	/* No such device (e.g., media were just removed) */
-	if (!get_capacity(disk))
-		goto exit;
+	if (!get_capacity(disk)) {
+		err = -ENODEV;
+		goto exit_sysfs_deprecated;
+	}
 
 	bdev = bdget_disk(disk, 0);
-	if (!bdev)
-		goto exit;
+	if (!bdev) {
+		err = -ENODEV;
+		goto exit_sysfs_deprecated;
+	}
 
 	bdev->bd_invalidated = 1;
 	err = blkdev_get(bdev, FMODE_READ, NULL);
 	if (err < 0)
-		goto exit;
+		goto exit_bdput;
 	blkdev_put(bdev, FMODE_READ);
 
-exit:
+exit_success:
 	disk_announce(disk);
 
 	if (disk->queue->backing_dev_info->dev) {
 		err = sysfs_create_link(&ddev->kobj,
 			  &disk->queue->backing_dev_info->dev->kobj,
 			  "bdi");
-		WARN_ON(err);
+		if (WARN_ON(err))
+			goto exit_bdput;
 	}
+
+	return 0;
+
+exit_bdput:
+	if (bdev)
+		bdput(bdev);
+exit_sysfs_deprecated:
+	if (!sysfs_deprecated)
+		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
+exit_del_device:
+	device_del(ddev);
+
+	return err;
 }
 
 /**
@@ -867,21 +896,25 @@ static void register_disk(struct device *parent, struct gendisk *disk,
  *
  * This function registers the partitioning information in @disk
  * with the kernel.
- *
- * FIXME: error handling
  */
-static void __device_add_disk(struct device *parent, struct gendisk *disk,
-			      const struct attribute_group **groups,
-			      bool register_queue)
+
+static int __device_add_disk(struct device *parent, struct gendisk *disk,
+			     const struct attribute_group **groups,
+			     bool register_queue)
 {
 	dev_t devt;
 	int retval;
 
 	/*
 	 * Take an extra ref on queue which will be put on disk_release()
-	 * so that it sticks around as long as @disk is there.
+	 * so that it sticks around as long as @disk is there. The driver
+	 * must call blk_cleanup_queue() and then put_disk() on error from
+	 * this function.
 	 */
-	WARN_ON_ONCE(!blk_get_queue(disk->queue));
+	if (WARN_ON_ONCE(!blk_get_queue(disk->queue))) {
+		disk->queue = NULL;
+		return -ESHUTDOWN;
+	}
 
 	/*
 	 * The disk queue should now be all set with enough information about
@@ -896,21 +929,24 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 	 * be accompanied with EXT_DEVT flag.  Make sure all
 	 * parameters make sense.
 	 */
-	WARN_ON(disk->minors && !(disk->major || disk->first_minor));
-	WARN_ON(!disk->minors &&
-		!(disk->flags & (GENHD_FL_EXT_DEVT | GENHD_FL_HIDDEN)));
+	if (WARN_ON(disk->minors && !(disk->major || disk->first_minor)))
+		return -EINVAL;
+	if (WARN_ON(!disk->minors &&
+		    !(disk->flags & (GENHD_FL_EXT_DEVT | GENHD_FL_HIDDEN))))
+		return -EINVAL;
 
 	disk->flags |= GENHD_FL_UP;
 
 	retval = blk_alloc_devt(&disk->part0, &devt);
-	if (retval) {
-		WARN_ON(1);
-		return;
-	}
+	if (WARN_ON(retval))
+		return retval;
+
 	disk->major = MAJOR(devt);
 	disk->first_minor = MINOR(devt);
 
-	disk_alloc_events(disk);
+	retval = disk_alloc_events(disk);
+	if (retval)
+		goto exit_blk_free_devt;
 
 	if (disk->flags & GENHD_FL_HIDDEN) {
 		/*
@@ -920,35 +956,75 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 		disk->flags |= GENHD_FL_SUPPRESS_PARTITION_INFO;
 		disk->flags |= GENHD_FL_NO_PART_SCAN;
 	} else {
-		int ret;
-
 		/* Register BDI before referencing it from bdev */
 		disk_to_dev(disk)->devt = devt;
-		ret = bdi_register_owner(disk->queue->backing_dev_info,
-						disk_to_dev(disk));
-		WARN_ON(ret);
-		blk_register_region(disk_devt(disk), disk->minors, NULL,
-				    exact_match, exact_lock, disk);
+
+		retval = bdi_register_owner(disk->queue->backing_dev_info,
+					    disk_to_dev(disk));
+		if (WARN_ON(retval))
+			goto exit_disk_release_events;
+		retval = blk_register_region(disk_devt(disk), disk->minors,
+					     NULL,
+					     exact_match, exact_lock, disk);
+		if (retval)
+			goto exit_unregister_bdi;
 	}
-	register_disk(parent, disk, groups);
+
+	retval = register_disk(parent, disk, groups);
+	if (retval)
+		goto exit_unregister_regions;
+
+	if (register_queue) {
+		retval = blk_register_queue(disk);
+		if (retval)
+			goto exit_unregister_disk;
+	}
+
+	retval = disk_add_events(disk);
+	if (retval)
+		goto exit_unregister_queue;
+
+	retval = blk_integrity_add(disk);
+	if (retval)
+		goto exit_del_events;
+
+	return 0;
+
+exit_del_events:
+	disk_del_events(disk);
+exit_unregister_queue:
 	if (register_queue)
-		blk_register_queue(disk);
+		blk_unregister_queue(disk);
+exit_unregister_disk:
+	disk_invalidate(disk);
+	if (!(disk->flags & GENHD_FL_HIDDEN))
+		sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
+	unregister_disk(disk);
+exit_unregister_regions:
+	if (!(disk->flags & GENHD_FL_HIDDEN))
+		blk_unregister_region(disk_devt(disk), disk->minors);
+exit_unregister_bdi:
+	if (disk->queue && !(disk->flags & GENHD_FL_HIDDEN))
+		bdi_unregister(disk->queue->backing_dev_info);
+exit_disk_release_events:
+	disk_release_events(disk);
+exit_blk_free_devt:
+	blk_free_devt(disk_devt(disk));
 
-	disk_add_events(disk);
-	blk_integrity_add(disk);
+	return retval;
 }
 
-void device_add_disk(struct device *parent, struct gendisk *disk,
-		     const struct attribute_group **groups)
+int device_add_disk(struct device *parent, struct gendisk *disk,
+		    const struct attribute_group **groups)
 
 {
-	__device_add_disk(parent, disk, groups, true);
+	return __device_add_disk(parent, disk, groups, true);
 }
 EXPORT_SYMBOL(device_add_disk);
 
-void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
+int device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
 {
-	__device_add_disk(parent, disk, NULL, false);
+	return __device_add_disk(parent, disk, NULL, false);
 }
 EXPORT_SYMBOL(device_add_disk_no_queue_reg);
 
@@ -2313,17 +2389,17 @@ module_param_cb(events_dfl_poll_msecs, &disk_events_dfl_poll_msecs_param_ops,
 /*
  * disk_{alloc|add|del|release}_events - initialize and destroy disk_events.
  */
-static void disk_alloc_events(struct gendisk *disk)
+static int disk_alloc_events(struct gendisk *disk)
 {
 	struct disk_events *ev;
 
 	if (!disk->fops->check_events || !disk->events)
-		return;
+		return 0;
 
 	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
 	if (!ev) {
 		pr_warn("%s: failed to initialize events\n", disk->disk_name);
-		return;
+		return -ENOMEM;
 	}
 
 	INIT_LIST_HEAD(&ev->node);
@@ -2335,17 +2411,23 @@ static void disk_alloc_events(struct gendisk *disk)
 	INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn);
 
 	disk->ev = ev;
+
+	return 0;
 }
 
-static void disk_add_events(struct gendisk *disk)
+static int disk_add_events(struct gendisk *disk)
 {
-	/* FIXME: error handling */
-	if (sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs) < 0)
+	int ret;
+
+	ret = sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs);
+	if (ret < 0) {
 		pr_warn("%s: failed to create sysfs files for events\n",
 			disk->disk_name);
+		return ret;
+	}
 
 	if (!disk->ev)
-		return;
+		return 0;
 
 	mutex_lock(&disk_events_mutex);
 	list_add_tail(&disk->ev->node, &disk_events);
@@ -2356,6 +2438,8 @@ static void disk_add_events(struct gendisk *disk)
 	 * unblock kicks it into action.
 	 */
 	__disk_unblock_events(disk, true);
+
+	return 0;
 }
 
 static void disk_del_events(struct gendisk *disk)
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 899760cf8c37..76fc8abd5899 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -291,16 +291,16 @@ extern void disk_part_iter_exit(struct disk_part_iter *piter);
 extern bool disk_has_partitions(struct gendisk *disk);
 
 /* 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)
+extern 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 device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk);
-static inline void add_disk_no_queue_reg(struct gendisk *disk)
+extern int device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk);
+static inline int add_disk_no_queue_reg(struct gendisk *disk)
 {
-	device_add_disk_no_queue_reg(NULL, disk);
+	return device_add_disk_no_queue_reg(NULL, disk);
 }
 
 extern void del_gendisk(struct gendisk *gp);
@@ -350,7 +350,7 @@ extern struct gendisk *__alloc_disk_node(int minors, int node_id);
 extern struct kobject *get_disk_and_module(struct gendisk *disk);
 extern void put_disk(struct gendisk *disk);
 extern void put_disk_and_module(struct gendisk *disk);
-extern void blk_register_region(dev_t devt, unsigned long range,
+extern int blk_register_region(dev_t devt, unsigned long range,
 			struct module *module,
 			struct kobject *(*probe)(dev_t, int *, void *),
 			int (*lock)(dev_t, void *),
-- 
2.25.1

[RFC v1 6/6] loop: add error handling support for add_disk()

[Luis Chamberlain]

We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/loop.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6dccba22c9b5..dcb126f3a7e1 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2096,10 +2096,15 @@ static int loop_add(struct loop_device **l, int i)
 	disk->private_data	= lo;
 	disk->queue		= lo->lo_queue;
 	sprintf(disk->disk_name, "loop%d", i);
-	add_disk(disk);
+	err = add_disk(disk);
+	if (err)
+		goto out_put_disk;
+
 	*l = lo;
 	return lo->lo_number;
 
+out_put_disk:
+	put_disk(lo->lo_disk);
 out_free_queue:
 	blk_cleanup_queue(lo->lo_queue);
 out_cleanup_tags:
-- 
2.25.1

Re: [RFC v1 0/6] block: add error handling for *add_disk*()

[Bart Van Assche]

On 2020-04-29 00:48, Luis Chamberlain wrote:
> While working on some blktrace races I noticed that we don't do
> error handling on *add_disk*() and friends. This is my initial
> work on that.
> 
> This is based on linux-next tag next-20200428, you can also get this
> on my branch 20200428-block-fixes [0].
> 
> Let me know what you think.
Hi Luis,

Thank you for having done this work. Since triggering error paths can be
challenging, how about adding fault injection capabilities that make it
possible to trigger all modified error paths and how about adding
blktests that trigger these paths? That is the strategy that I followed
myself recently to fix an error path in blk_mq_realloc_hw_ctxs().

Thanks,

Bart.

Re: [RFC v1 0/6] block: add error handling for *add_disk*()

[Luis Chamberlain]

On Tue, May 05, 2020 at 06:18:22PM -0700, Bart Van Assche wrote:
> On 2020-04-29 00:48, Luis Chamberlain wrote:
> > While working on some blktrace races I noticed that we don't do
> > error handling on *add_disk*() and friends. This is my initial
> > work on that.
> > 
> > This is based on linux-next tag next-20200428, you can also get this
> > on my branch 20200428-block-fixes [0].
> > 
> > Let me know what you think.
> Hi Luis,
> 
> Thank you for having done this work.

My pleasure, I just made one minor change to this series, but that's
all so far. Note that break-blktrace run_0004.sh still yields:

debugfs: Directory 'loop0' with parent 'block' already present!

And so I suspect something else is up, this is even after. That's using
my latest:

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20200508-block-fixes

Some more eyebealls on that would be useful.

> Since triggering error paths can be
> challenging, how about adding fault injection capabilities that make it
> possible to trigger all modified error paths and how about adding
> blktests that trigger these paths? That is the strategy that I followed
> myself recently to fix an error path in blk_mq_realloc_hw_ctxs().

Sure thing, but I get the impression that adding this may make it odd
to or harder to review. Shouldn't this be done after we have *some*
error handling? Right now we shouldn't regress as we never fail, and
that seemss worse.

Let me know, either way, I'll start work on it.

  Luis