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

STALE1789d

Revision v1 of 2 in this series.

11 messages, 3 authors, 2021-09-08 · open the first message on its own page

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

From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2021-09-04 01:39:43

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

From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2021-09-04 01:39:46

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

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: 2021-09-04 02:49:22

On 2021/09/04 10:39, Luis Chamberlain wrote:
quoted hunk
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

From: Jens Axboe <axboe@kernel.dk>
Date: 2021-09-04 04:14:28

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

From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2021-09-05 18:11:42

On Sat, Sep 04, 2021 at 11:49:06AM +0900, Tetsuo Handa wrote:
On 2021/09/04 10:39, Luis Chamberlain wrote:
quoted
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

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: 2021-09-06 05:59:55

On 2021/09/06 3:11, Luis Chamberlain wrote:
On Sat, Sep 04, 2021 at 11:49:06AM +0900, Tetsuo Handa wrote:
quoted
On 2021/09/04 10:39, Luis Chamberlain wrote:
quoted
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

From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2021-09-07 14:58:00

On Mon, Sep 06, 2021 at 02:59:38PM +0900, Tetsuo Handa wrote:
On 2021/09/06 3:11, Luis Chamberlain wrote:
quoted
On Sat, Sep 04, 2021 at 11:49:06AM +0900, Tetsuo Handa wrote:
quoted
On 2021/09/04 10:39, Luis Chamberlain wrote:
quoted
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

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: 2021-09-07 15:23:17

On 2021/09/07 23:57, Luis Chamberlain wrote:
quoted
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

From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2021-09-07 15:28:18

On Wed, Sep 08, 2021 at 12:23:02AM +0900, Tetsuo Handa wrote:
On 2021/09/07 23:57, Luis Chamberlain wrote:
quoted
quoted
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

From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2021-09-08 14:00:31

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:
quoted
On 2021/09/07 23:57, Luis Chamberlain wrote:
quoted
quoted
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

From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2021-09-04 01:40:08

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help