linux-kernel - [PATCH 2/2] ubi: block: Fix deadlock on remove

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230523-ubiblock-remove-v1-2-240bed75849b@axis.com>
Date:   Tue, 23 May 2023 15:12:17 +0200
From:   Vincent Whitchurch <vincent.whitchurch@...s.com>
To:     Richard Weinberger <richard@....at>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Vignesh Raghavendra <vigneshr@...com>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <kernel@...s.com>, Vincent Whitchurch <vincent.whitchurch@...s.com>
Subject: [PATCH 2/2] ubi: block: Fix deadlock on remove

Lockdep warns about possible circular locking when the following
commands are run:

 ubiblock --create /dev/ubi0_0
 head -c1 /dev/ubiblock0_0 > /dev/null
 ubiblock --remove /dev/ubi0_0

 ======================================================
 WARNING: possible circular locking dependency detected

 ubiblock/364 is trying to acquire lock:
 (&disk->open_mutex){+.+.}-{3:3}, at: del_gendisk (block/genhd.c:616)

 but task is already holding lock:
 (&dev->dev_mutex){+.+.}-{3:3}, at: ubiblock_remove (drivers/mtd/ubi/block.c:476)

 which lock already depends on the new lock.

 the existing dependency chain (in reverse order) is:

 -> #1 (&dev->dev_mutex){+.+.}-{3:3}:
   ubiblock_open (drivers/mtd/ubi/block.c:236)
   blkdev_get_whole (block/bdev.c:607)
   blkdev_get_by_dev (block/bdev.c:756)
   blkdev_open (block/fops.c:493)
   ...
   do_sys_openat2 (fs/open.c:1356)

 -> #0 (&disk->open_mutex){+.+.}-{3:3}:
   del_gendisk (block/genhd.c:616)
   ubiblock_remove (drivers/mtd/ubi/block.c:456 drivers/mtd/ubi/block.c:483)
   vol_cdev_ioctl
   ...

 other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(&dev->dev_mutex);
                                lock(&disk->open_mutex);
                                lock(&dev->dev_mutex);
   lock(&disk->open_mutex);

 *** DEADLOCK ***

 Call Trace:
 del_gendisk (block/genhd.c:616)
 ubiblock_remove (drivers/mtd/ubi/block.c:456 drivers/mtd/ubi/block.c:483)
 vol_cdev_ioctl
 ...

The actual deadlock is also easily reproducible by running the above
commands in parallel in a loop.

Fix this by marking the device as going away and releasing the dev mutex
before del_gendisk().  This is similar to other drivers such as
drivers/block/zram/zram_drv.c.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@...s.com>
---
 drivers/mtd/ubi/block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 70caec4606cd..fcfea7cfdb6b 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -83,6 +83,8 @@ struct ubiblock {
 	struct mutex dev_mutex;
 	struct list_head list;
 	struct blk_mq_tag_set tag_set;
+
+	bool removing;
 };
 
 /* Linked list of all ubiblock instances */
@@ -233,6 +235,11 @@ static int ubiblock_open(struct block_device *bdev, fmode_t mode)
 	int ret;
 
 	mutex_lock(&dev->dev_mutex);
+	if (dev->removing) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
 	if (dev->refcnt > 0) {
 		/*
 		 * The volume is already open, just increase the reference
@@ -480,8 +487,15 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 
 	/* Remove from device list */
 	list_del(&dev->list);
-	ubiblock_cleanup(dev);
+
+	/*
+	 * Prevent further opens.  del_gendisk() will ensure that there are no
+	 * parallel openers.
+	 */
+	dev->removing = true;
 	mutex_unlock(&dev->dev_mutex);
+
+	ubiblock_cleanup(dev);
 	mutex_unlock(&devices_mutex);
 
 	kfree(dev);

-- 
2.34.1