[PATCH 0/2] block: 7th -- last batch of add_disk() error handling conversions

[PATCH 0/2] block: 7th -- last batch of add_disk() error handling conversions

[Luis Chamberlain]

This is the 7th and final set of changes to convert all drivers over to
use and finally enforce add_disk() error handling. This set deals with
those old drivers using __register_blkdev() which in turn have a
respective add_disk() call made. The last patch ensures we won't
add new drivers without a check for the add_disk() call.

You can find the full set of patches on my kernel.org linux
20210901-for-axboe-add-disk-error-handling branch [0] which is now based
on axboe/master.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210901-for-axboe-add-disk-error-handling

Luis Chamberlain (2):
  block: make __register_blkdev() return an error
  block: add __must_check for *add_disk*() callers

 block/genhd.c           | 21 ++++++++++++---------
 drivers/block/ataflop.c | 20 +++++++++++++++-----
 drivers/block/brd.c     |  7 +++++--
 drivers/block/floppy.c  | 14 ++++++++++----
 drivers/block/loop.c    |  6 +++---
 drivers/md/md.c         | 10 +++++++---
 drivers/scsi/sd.c       |  3 ++-
 fs/block_dev.c          |  5 ++++-
 include/linux/genhd.h   | 10 +++++-----
 9 files changed, 63 insertions(+), 33 deletions(-)

-- 
2.30.2

[PATCH 1/2] block: make __register_blkdev() return an error

[Luis Chamberlain]

This makes __register_blkdev() return an error, and
also changes the probe() call to return an error as well.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c           | 15 +++++++++------
 drivers/block/ataflop.c | 20 +++++++++++++++-----
 drivers/block/brd.c     |  7 +++++--
 drivers/block/floppy.c  | 14 ++++++++++----
 drivers/block/loop.c    |  6 +++---
 drivers/md/md.c         | 10 +++++++---
 drivers/scsi/sd.c       |  3 ++-
 fs/block_dev.c          |  5 ++++-
 include/linux/genhd.h   |  4 ++--
 9 files changed, 57 insertions(+), 27 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 567549a011d1..c4836296a974 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -180,7 +180,7 @@ static struct blk_major_name {
 	struct blk_major_name *next;
 	int major;
 	char name[16];
-	void (*probe)(dev_t devt);
+	int (*probe)(dev_t devt);
 } *major_names[BLKDEV_MAJOR_HASH_SIZE];
 static DEFINE_MUTEX(major_names_lock);
 
@@ -209,7 +209,7 @@ void blkdev_show(struct seq_file *seqf, off_t offset)
  * @major: the requested major device number [1..BLKDEV_MAJOR_MAX-1]. If
  *         @major = 0, try to allocate any unused major number.
  * @name: the name of the new block device as a zero terminated string
- * @probe: allback that is called on access to any minor number of @major
+ * @probe: callback that is called on access to any minor number of @major
  *
  * The @name must be unique within the system.
  *
@@ -227,7 +227,7 @@ void blkdev_show(struct seq_file *seqf, off_t offset)
  * Use register_blkdev instead for any new code.
  */
 int __register_blkdev(unsigned int major, const char *name,
-		void (*probe)(dev_t devt))
+		int (*probe)(dev_t devt))
 {
 	struct blk_major_name **n, *p;
 	int index, ret = 0;
@@ -621,17 +621,18 @@ static ssize_t disk_badblocks_store(struct device *dev,
 	return badblocks_store(disk->bb, page, len, 0);
 }
 
-void blk_request_module(dev_t devt)
+int blk_request_module(dev_t devt)
 {
 	unsigned int major = MAJOR(devt);
 	struct blk_major_name **n;
+	int err;
 
 	mutex_lock(&major_names_lock);
 	for (n = &major_names[major_to_index(major)]; *n; n = &(*n)->next) {
 		if ((*n)->major == major && (*n)->probe) {
-			(*n)->probe(devt);
+			err = (*n)->probe(devt);
 			mutex_unlock(&major_names_lock);
-			return;
+			return err;
 		}
 	}
 	mutex_unlock(&major_names_lock);
@@ -639,6 +640,8 @@ void blk_request_module(dev_t devt)
 	if (request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)) > 0)
 		/* Make old-style 2.4 aliases work */
 		request_module("block-major-%d", MAJOR(devt));
+
+	return 0;
 }
 
 /*
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 5dc9b3d32415..e55824e27188 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1989,24 +1989,34 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
 
 static DEFINE_MUTEX(ataflop_probe_lock);
 
-static void ataflop_probe(dev_t dev)
+static int ataflop_probe(dev_t dev)
 {
 	int drive = MINOR(dev) & 3;
 	int type  = MINOR(dev) >> 2;
+	int err = -ENODEV;
 
 	if (type)
 		type--;
 
-	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
-		return;
+	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS) {
+		err = -EINVAL;
+		goto out;
+	}
+
 	mutex_lock(&ataflop_probe_lock);
 	if (!unit[drive].disk[type]) {
-		if (ataflop_alloc_disk(drive, type) == 0) {
-			add_disk(unit[drive].disk[type]);
+		err = ataflop_alloc_disk(drive, type);
+		if (err == 0) {
+			err = add_disk(unit[drive].disk[type]);
+			if (err)
+				blk_cleanup_disk(unit[drive].disk[type]);
 			unit[drive].registered[type] = true;
 		}
 	}
 	mutex_unlock(&ataflop_probe_lock);
+
+out:
+	return err;
 }
 
 static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index c2bf4946f4e3..82a93044de95 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -426,10 +426,11 @@ static int brd_alloc(int i)
 	return err;
 }
 
-static void brd_probe(dev_t dev)
+static int brd_probe(dev_t dev)
 {
 	int i = MINOR(dev) / max_part;
 	struct brd_device *brd;
+	int err = 0;
 
 	mutex_lock(&brd_devices_mutex);
 	list_for_each_entry(brd, &brd_devices, brd_list) {
@@ -437,9 +438,11 @@ static void brd_probe(dev_t dev)
 			goto out_unlock;
 	}
 
-	brd_alloc(i);
+	err = brd_alloc(i);
 out_unlock:
 	mutex_unlock(&brd_devices_mutex);
+
+	return err;
 }
 
 static void brd_del_one(struct brd_device *brd)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 0434f28742e7..5ecffc7f6e22 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4517,21 +4517,27 @@ static int floppy_alloc_disk(unsigned int drive, unsigned int type)
 
 static DEFINE_MUTEX(floppy_probe_lock);
 
-static void floppy_probe(dev_t dev)
+static int floppy_probe(dev_t dev)
 {
 	unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
 	unsigned int type = (MINOR(dev) >> 2) & 0x1f;
+	int err = -ENODEV;
 
 	if (drive >= N_DRIVE || !floppy_available(drive) ||
 	    type >= ARRAY_SIZE(floppy_type))
-		return;
+		return -EINVAL;
 
 	mutex_lock(&floppy_probe_lock);
 	if (!disks[drive][type]) {
-		if (floppy_alloc_disk(drive, type) == 0)
-			add_disk(disks[drive][type]);
+		if (floppy_alloc_disk(drive, type) == 0) {
+			err = add_disk(disks[drive][type]);
+			if (err)
+				blk_cleanup_disk(disks[drive][type]);
+		}
 	}
 	mutex_unlock(&floppy_probe_lock);
+
+	return err;
 }
 
 static int __init do_floppy_init(void)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b8b9e2349e77..6bc1c075741e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2425,13 +2425,13 @@ static void loop_remove(struct loop_device *lo)
 	kfree(lo);
 }
 
-static void loop_probe(dev_t dev)
+static int loop_probe(dev_t dev)
 {
 	int idx = MINOR(dev) >> part_shift;
 
 	if (max_loop && idx >= max_loop)
-		return;
-	loop_add(idx);
+		return -EINVAL;
+	return loop_add(idx);
 }
 
 static int loop_control_remove(int idx)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 5c0d3536d7c7..fc2790979686 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5735,12 +5735,16 @@ static int md_alloc(dev_t dev, char *name)
 	return error;
 }
 
-static void md_probe(dev_t dev)
+static int md_probe(dev_t dev)
 {
+	int error = 0;
+
 	if (MAJOR(dev) == MD_MAJOR && MINOR(dev) >= 512)
-		return;
+		return -EINVAL;
 	if (create_on_open)
-		md_alloc(dev, NULL);
+		error = md_alloc(dev, NULL);
+
+	return error;
 }
 
 static int add_named_array(const char *val, const struct kernel_param *kp)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8c1273fff23e..50872c6ce030 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -630,8 +630,9 @@ static struct scsi_driver sd_template = {
  * Don't request a new module, as that could deadlock in multipath
  * environment.
  */
-static void sd_default_probe(dev_t devt)
+static int sd_default_probe(dev_t devt)
 {
+	return 0;
 }
 
 /*
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 45df6cbccf12..81a4738910a8 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1144,10 +1144,13 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 {
 	struct block_device *bdev;
 	struct inode *inode;
+	int ret;
 
 	inode = ilookup(blockdev_superblock, dev);
 	if (!inode) {
-		blk_request_module(dev);
+		ret = blk_request_module(dev);
+		if (ret)
+			return NULL;
 		inode = ilookup(blockdev_superblock, dev);
 		if (!inode)
 			return NULL;
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index c68d83c87f83..5828ecda5c49 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -281,7 +281,7 @@ struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass);
 void blk_cleanup_disk(struct gendisk *disk);
 
 int __register_blkdev(unsigned int major, const char *name,
-		void (*probe)(dev_t devt));
+		int (*probe)(dev_t devt));
 #define register_blkdev(major, name) \
 	__register_blkdev(major, name, NULL)
 void unregister_blkdev(unsigned int major, const char *name);
@@ -317,7 +317,7 @@ static inline int bd_register_pending_holders(struct gendisk *disk)
 dev_t part_devt(struct gendisk *disk, u8 partno);
 void inc_diskseq(struct gendisk *disk);
 dev_t blk_lookup_devt(const char *name, int partno);
-void blk_request_module(dev_t devt);
+int blk_request_module(dev_t devt);
 #ifdef CONFIG_BLOCK
 void printk_all_partitions(void);
 #else /* CONFIG_BLOCK */
-- 
2.30.2

Re: [PATCH 1/2] block: make __register_blkdev() return an error

[Tetsuo Handa]

On 2021/09/04 10:39, Luis Chamberlain wrote:
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 45df6cbccf12..81a4738910a8 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1144,10 +1144,13 @@ struct block_device *blkdev_get_no_open(dev_t dev)
>  {
>  	struct block_device *bdev;
>  	struct inode *inode;
> +	int ret;
>  
>  	inode = ilookup(blockdev_superblock, dev);
>  	if (!inode) {
> -		blk_request_module(dev);
> +		ret = blk_request_module(dev);
> +		if (ret)
> +			return NULL;

Since e.g. loop_add() from loop_probe() returns -EEXIST when /dev/loop$num already
exists (e.g. raced with ioctl(LOOP_CTL_ADD)), isn't unconditionally failing an over-failing?

>  		inode = ilookup(blockdev_superblock, dev);
>  		if (!inode)
>  			return NULL;

By the way, Jens, will you pick up
https://lkml.kernel.org/r/adb1e792-fc0e-ee81-7ea0-0906fc36419d@i-love.sakura.ne.jp
before these "add error handling" changes?

Re: [PATCH 1/2] block: make __register_blkdev() return an error

[Jens Axboe]

On 9/3/21 8:49 PM, Tetsuo Handa wrote:
> By the way, Jens, will you pick up
> https://lkml.kernel.org/r/adb1e792-fc0e-ee81-7ea0-0906fc36419d@i-love.sakura.ne.jp
> before these "add error handling" changes?

Yes, that one is 5.15 material, the rest of the error handling changes
are not. Hence the ordering follows naturally from that.

-- 
Jens Axboe

Re: [PATCH 1/2] block: make __register_blkdev() return an error

[Luis Chamberlain]

On Sat, Sep 04, 2021 at 11:49:06AM +0900, Tetsuo Handa wrote:
> On 2021/09/04 10:39, Luis Chamberlain wrote:
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index 45df6cbccf12..81a4738910a8 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -1144,10 +1144,13 @@ struct block_device *blkdev_get_no_open(dev_t dev)
> >  {
> >  	struct block_device *bdev;
> >  	struct inode *inode;
> > +	int ret;
> >  
> >  	inode = ilookup(blockdev_superblock, dev);
> >  	if (!inode) {
> > -		blk_request_module(dev);
> > +		ret = blk_request_module(dev);
> > +		if (ret)
> > +			return NULL;
> 
> Since e.g. loop_add() from loop_probe() returns -EEXIST when /dev/loop$num already
> exists (e.g. raced with ioctl(LOOP_CTL_ADD)), isn't unconditionally failing an over-failing?

It's not clear to me how. What do we loose by capturing the failure on
blk_request_module()?

  Luis

Re: [PATCH 1/2] block: make __register_blkdev() return an error

[Tetsuo Handa]

On 2021/09/06 3:11, Luis Chamberlain wrote:
> On Sat, Sep 04, 2021 at 11:49:06AM +0900, Tetsuo Handa wrote:
>> On 2021/09/04 10:39, Luis Chamberlain wrote:
>>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>>> index 45df6cbccf12..81a4738910a8 100644
>>> --- a/fs/block_dev.c
>>> +++ b/fs/block_dev.c
>>> @@ -1144,10 +1144,13 @@ struct block_device *blkdev_get_no_open(dev_t dev)
>>>  {
>>>  	struct block_device *bdev;
>>>  	struct inode *inode;
>>> +	int ret;
>>>  
>>>  	inode = ilookup(blockdev_superblock, dev);
>>>  	if (!inode) {
>>> -		blk_request_module(dev);
>>> +		ret = blk_request_module(dev);
>>> +		if (ret)
>>> +			return NULL;
>>
>> Since e.g. loop_add() from loop_probe() returns -EEXIST when /dev/loop$num already
>> exists (e.g. raced with ioctl(LOOP_CTL_ADD)), isn't unconditionally failing an over-failing?
> 
> It's not clear to me how. What do we loose by capturing the failure on
> blk_request_module()?
> 

We loose ability to handle concurrent request.

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 45df6cbccf12..9559c8aabc88 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -36,6 +36,7 @@
 #include <linux/suspend.h>
 #include "internal.h"
 #include "../block/blk.h"
+#include <linux/debug_locks.h>

 struct bdev_inode {
        struct block_device bdev;
@@ -1147,7 +1148,11 @@ struct block_device *blkdev_get_no_open(dev_t dev)

        inode = ilookup(blockdev_superblock, dev);
        if (!inode) {
+               dump_stack();
+               debug_show_held_locks(current);
                blk_request_module(dev);
+               if (!strcmp(current->comm, "cat"))
+                       schedule_timeout_killable(10 * HZ);
                inode = ilookup(blockdev_superblock, dev);
                if (!inode)
                        return NULL;

Try "mknod /dev/loop$num b 7 $num" followed by concurrent "cat /dev/loop$num" requests.

[  535.847823] [ T5261] CPU: 1 PID: 5261 Comm: cat Tainted: G            E     5.14.0-next-20210903+ #3
[  535.851419] [ T5261] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[  535.855563] [ T5261] Call Trace:
[  535.856746] [ T5261]  dump_stack_lvl+0x79/0xbf
[  535.858332] [ T5261]  blkdev_get_no_open+0x86/0xe0
[  535.860036] [ T5261]  blkdev_get_by_dev+0x5f/0x540
[  535.861747] [ T5261]  ? block_ioctl+0x40/0x40
[  535.863305] [ T5261]  blkdev_open+0x58/0x90
[  535.864865] [ T5261]  do_dentry_open+0x144/0x3a0
[  535.866538] [ T5261]  path_openat+0xa57/0xda0
[  535.868139] [ T5261]  do_filp_open+0x9f/0x140
[  535.869933] [ T5261]  ? _raw_spin_unlock+0x1a/0x30
[  535.871570] [ T5261]  do_sys_openat2+0x71/0x150
[  535.873195] [ T5261]  __x64_sys_openat+0x78/0xa0
[  535.874740] [ T5261]  do_syscall_64+0x3d/0xb0
[  535.876234] [ T5261]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  535.878428] [ T5261] RIP: 0033:0x7f6b997d28db
[  535.879927] [ T5261] Code: 25 00 00 41 00 3d 00 00 41 00 74 4b 64 8b 04 25 18 00 00 00 85 c0 75 67 44 89 e2 48 89 ee bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 0f 87 91 00 00 00 48 8b 4c 24 28 64 48 2b 0c 25
[  535.886778] [ T5261] RSP: 002b:00007ffe8bdad880 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
[  535.889727] [ T5261] RAX: ffffffffffffffda RBX: 000055f741e5b634 RCX: 00007f6b997d28db
[  535.892466] [ T5261] RDX: 0000000000000000 RSI: 00007ffe8bdaf764 RDI: 00000000ffffff9c
[  535.895226] [ T5261] RBP: 00007ffe8bdaf764 R08: 0000000000000001 R09: 0000000000000000
[  535.898162] [ T5261] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[  535.900858] [ T5261] R13: 00007ffe8bdadb68 R14: 0000000000000000 R15: 0000000000020000
[  535.903752] [ T5262] CPU: 2 PID: 5262 Comm: cat Tainted: G            E     5.14.0-next-20210903+ #3
[  535.904759] [ T5261] no locks held by cat/5261.
[  535.906905] [ T5262] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[  535.906908] [ T5262] Call Trace:
[  535.906912] [ T5262]  dump_stack_lvl+0x79/0xbf
[  535.915279] [ T5262]  blkdev_get_no_open+0x86/0xe0
[  535.916942] [ T5262]  blkdev_get_by_dev+0x5f/0x540
[  535.918608] [ T5262]  ? block_ioctl+0x40/0x40
[  535.920250] [ T5262]  blkdev_open+0x58/0x90
[  535.921696] [ T5262]  do_dentry_open+0x144/0x3a0
[  535.923291] [ T5262]  path_openat+0xa57/0xda0
[  535.924816] [ T5262]  do_filp_open+0x9f/0x140
[  535.926359] [ T5262]  ? _raw_spin_unlock+0x1a/0x30
[  535.928030] [ T5262]  do_sys_openat2+0x71/0x150
[  535.929600] [ T5262]  __x64_sys_openat+0x78/0xa0
[  535.931215] [ T5262]  do_syscall_64+0x3d/0xb0
[  535.932768] [ T5262]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  535.934817] [ T5262] RIP: 0033:0x7efe3a0aa8db
[  535.936354] [ T5262] Code: 25 00 00 41 00 3d 00 00 41 00 74 4b 64 8b 04 25 18 00 00 00 85 c0 75 67 44 89 e2 48 89 ee bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 0f 87 91 00 00 00 48 8b 4c 24 28 64 48 2b 0c 25
[  535.943021] [ T5262] RSP: 002b:00007ffe1697dfb0 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
[  535.945883] [ T5262] RAX: ffffffffffffffda RBX: 000055982a4e1634 RCX: 00007efe3a0aa8db
[  535.948580] [ T5262] RDX: 0000000000000000 RSI: 00007ffe1697e764 RDI: 00000000ffffff9c
[  535.951298] [ T5262] RBP: 00007ffe1697e764 R08: 0000000000000001 R09: 0000000000000000
[  535.954064] [ T5262] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[  535.956789] [ T5262] R13: 00007ffe1697e298 R14: 0000000000000000 R15: 0000000000020000
[  535.959522] [ T5263] CPU: 0 PID: 5263 Comm: cat Tainted: G            E     5.14.0-next-20210903+ #3
[  535.962995] [ T5263] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[  535.965787] [ T5262] no locks held by cat/5262.
[  535.966998] [ T5263] Call Trace:
[  535.970068] [ T5263]  dump_stack_lvl+0x79/0xbf
[  535.971572] [ T5263]  blkdev_get_no_open+0x86/0xe0
[  535.973199] [ T5263]  blkdev_get_by_dev+0x5f/0x540
[  535.974893] [ T5263]  ? block_ioctl+0x40/0x40
[  535.976364] [ T5263]  blkdev_open+0x58/0x90
[  535.977781] [ T5263]  do_dentry_open+0x144/0x3a0
[  535.979378] [ T5263]  path_openat+0xa57/0xda0
[  535.980882] [ T5263]  do_filp_open+0x9f/0x140
[  535.982333] [ T5263]  ? _raw_spin_unlock+0x1a/0x30
[  535.984002] [ T5263]  do_sys_openat2+0x71/0x150
[  535.985554] [ T5263]  __x64_sys_openat+0x78/0xa0
[  535.987232] [ T5263]  do_syscall_64+0x3d/0xb0
[  535.988790] [ T5263]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  535.990804] [ T5263] RIP: 0033:0x7ff84ab408db
[  535.992275] [ T5263] Code: 25 00 00 41 00 3d 00 00 41 00 74 4b 64 8b 04 25 18 00 00 00 85 c0 75 67 44 89 e2 48 89 ee bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 0f 87 91 00 00 00 48 8b 4c 24 28 64 48 2b 0c 25
[  535.998888] [ T5263] RSP: 002b:00007ffda9b89a70 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
[  536.001628] [ T5263] RAX: ffffffffffffffda RBX: 000055626f376634 RCX: 00007ff84ab408db
[  536.004350] [ T5263] RDX: 0000000000000000 RSI: 00007ffda9b8a764 RDI: 00000000ffffff9c
[  536.007009] [ T5263] RBP: 00007ffda9b8a764 R08: 0000000000000001 R09: 0000000000000000
[  536.009753] [ T5263] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[  536.012493] [ T5263] R13: 00007ffda9b89d58 R14: 0000000000000000 R15: 0000000000020000
[  536.015854] [ T5263] no locks held by cat/5263.

If blk_request_module() does not ignore -EEXIST error, only the first open() request would succeed
because loop_add() returns 0 and all other concurrent requests would fail because loop_add() returns -EEXIST.

Actually, blk_request_module() failures should be ignored, for subsequent ilookup() will
fail if blk_request_module() failed to create the requested block device.

Re: [PATCH 1/2] block: make __register_blkdev() return an error

[Luis Chamberlain]

On Mon, Sep 06, 2021 at 02:59:38PM +0900, Tetsuo Handa wrote:
> On 2021/09/06 3:11, Luis Chamberlain wrote:
> > On Sat, Sep 04, 2021 at 11:49:06AM +0900, Tetsuo Handa wrote:
> >> On 2021/09/04 10:39, Luis Chamberlain wrote:
> >>> diff --git a/fs/block_dev.c b/fs/block_dev.c
> >>> index 45df6cbccf12..81a4738910a8 100644
> >>> --- a/fs/block_dev.c
> >>> +++ b/fs/block_dev.c
> >>> @@ -1144,10 +1144,13 @@ struct block_device *blkdev_get_no_open(dev_t dev)
> >>>  {
> >>>  	struct block_device *bdev;
> >>>  	struct inode *inode;
> >>> +	int ret;
> >>>  
> >>>  	inode = ilookup(blockdev_superblock, dev);
> >>>  	if (!inode) {
> >>> -		blk_request_module(dev);
> >>> +		ret = blk_request_module(dev);
> >>> +		if (ret)
> >>> +			return NULL;
> >>
> >> Since e.g. loop_add() from loop_probe() returns -EEXIST when /dev/loop$num already
> >> exists (e.g. raced with ioctl(LOOP_CTL_ADD)), isn't unconditionally failing an over-failing?
> > 
> > It's not clear to me how. What do we loose by capturing the failure on
> > blk_request_module()?
> > 
> 
> We loose ability to handle concurrent request.
> If blk_request_module() does not ignore -EEXIST error, only the first
> open() request would succeed because loop_add() returns 0 and all
> other concurrent requests would fail because loop_add() returns
> -EEXIST.

Yes I see that now thanks!

> Actually, blk_request_module() failures should be ignored, for
> subsequent ilookup() will fail if blk_request_module() failed to
> create the requested block device.

Then how about this:

Since we would like to use __must_check for add_disk() we proceed with
the change to capture the errors and propagate them and we just document on
fs/block_dev.c's use of blk_request_module() about the above issue and
how we prefer the errror that ilookup() would return.

  Luis

Re: [PATCH 1/2] block: make __register_blkdev() return an error

[Tetsuo Handa]

On 2021/09/07 23:57, Luis Chamberlain wrote:
>> Actually, blk_request_module() failures should be ignored, for
>> subsequent ilookup() will fail if blk_request_module() failed to
>> create the requested block device.
> 
> Then how about this:
> 
> Since we would like to use __must_check for add_disk() we proceed with
> the change to capture the errors and propagate them and we just document on
> fs/block_dev.c's use of blk_request_module() about the above issue and
> how we prefer the errror that ilookup() would return.

Marking add_disk() as __must_check makes it possible to enforce "don't leave
partially initialized devices". That's already an enough improvement.

Probe functions can remain "void", and hence blk_request_module() can remain "void".
That is, I would drop "[PATCH 1/2] block: make __register_blkdev() return an error".

Re: [PATCH 1/2] block: make __register_blkdev() return an error

[Luis Chamberlain]

On Wed, Sep 08, 2021 at 12:23:02AM +0900, Tetsuo Handa wrote:
> On 2021/09/07 23:57, Luis Chamberlain wrote:
> >> Actually, blk_request_module() failures should be ignored, for
> >> subsequent ilookup() will fail if blk_request_module() failed to
> >> create the requested block device.
> > 
> > Then how about this:
> > 
> > Since we would like to use __must_check for add_disk() we proceed with
> > the change to capture the errors and propagate them and we just document on
> > fs/block_dev.c's use of blk_request_module() about the above issue and
> > how we prefer the errror that ilookup() would return.
> 
> Marking add_disk() as __must_check makes it possible to enforce "don't leave
> partially initialized devices". That's already an enough improvement.
> 
> Probe functions can remain "void", and hence blk_request_module() can remain "void".
> That is, I would drop "[PATCH 1/2] block: make __register_blkdev() return an error".

Probe calls can be left voide, but because of the new __must_check we'd
still have to modify all probe calls as they use add_disk() and it would
seem odd to just capture the error to ignore it without documenting
any of this.

  Luis

Re: [PATCH 1/2] block: make __register_blkdev() return an error

[Luis Chamberlain]

On Tue, Sep 07, 2021 at 08:28:06AM -0700, Luis Chamberlain wrote:
> On Wed, Sep 08, 2021 at 12:23:02AM +0900, Tetsuo Handa wrote:
> > On 2021/09/07 23:57, Luis Chamberlain wrote:
> > >> Actually, blk_request_module() failures should be ignored, for
> > >> subsequent ilookup() will fail if blk_request_module() failed to
> > >> create the requested block device.
> > > 
> > > Then how about this:
> > > 
> > > Since we would like to use __must_check for add_disk() we proceed with
> > > the change to capture the errors and propagate them and we just document on
> > > fs/block_dev.c's use of blk_request_module() about the above issue and
> > > how we prefer the errror that ilookup() would return.
> > 
> > Marking add_disk() as __must_check makes it possible to enforce "don't leave
> > partially initialized devices". That's already an enough improvement.
> > 
> > Probe functions can remain "void", and hence blk_request_module() can remain "void".
> > That is, I would drop "[PATCH 1/2] block: make __register_blkdev() return an error".
> 
> Probe calls can be left voide, but because of the new __must_check we'd
> still have to modify all probe calls as they use add_disk() and it would
> seem odd to just capture the error to ignore it without documenting
> any of this.

So indeed, all probe() routines would need to be modified anyway because
of the added final __must_check. Because of this I still think that if
we want to *ignore* the error, we should just document and ignore it on
the caller side. That is, still modify __register_blkdev() to propagate
the error, and the caller can decide to ignore or not. In this case
we can document on fs/block_dev.c *why* we are ignoring the error.

Thoughts?

  Luis

[PATCH 2/2] block: add __must_check for *add_disk*() callers

[Luis Chamberlain]

Now that we have done a spring cleaning on all drivers and added
error checking / handling, let's keep it that way and ensure
no new drivers fail to stick with it.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c         | 6 +++---
 include/linux/genhd.h | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index c4836296a974..4c80e6c3d8dd 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -383,8 +383,8 @@ static void disk_scan_partitions(struct gendisk *disk)
  * This function registers the partitioning information in @disk
  * with the kernel.
  */
-int device_add_disk(struct device *parent, struct gendisk *disk,
-		     const struct attribute_group **groups)
+int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
+				 const struct attribute_group **groups)
 
 {
 	struct device *ddev = disk_to_dev(disk);
@@ -529,7 +529,7 @@ int device_add_disk(struct device *parent, struct gendisk *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 */
+	return ret;
 }
 EXPORT_SYMBOL(device_add_disk);
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 5828ecda5c49..8d78d36c424e 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -214,9 +214,9 @@ static inline dev_t disk_devt(struct gendisk *disk)
 void disk_uevent(struct gendisk *disk, enum kobject_action action);
 
 /* block/genhd.c */
-int device_add_disk(struct device *parent, struct gendisk *disk,
-		const struct attribute_group **groups);
-static inline int add_disk(struct gendisk *disk)
+int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
+				 const struct attribute_group **groups);
+static inline int __must_check add_disk(struct gendisk *disk)
 {
 	return device_add_disk(NULL, disk, NULL);
 }
-- 
2.30.2