Re: [syzbot] possible deadlock in del_gendisk

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

I hoped that below patch fixes the problem, but it did not ( https://syzkaller.appspot.com/text?tag=CrashReport&x=111bc8f7d00000 ).
I guess we need to somehow serialize loop_add() and loop_remove() if we can't use loop_ctl_mutex...

From 2dfbd8e4a10c7883a85a2d4c32d83c93b2c3d485 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri, 11 Jun 2021 05:54:25 +0000
Subject: [not-yet-a-PATCH] loop: drop loop_ctl_mutex before calling del_gendisk()

syzbot is reporting circular locking dependency between loop_ctl_mutex and
bdev->bd_mutex [1] due to commit c76f48eb5c084b1e ("block: take bd_mutex
around delete_partitions in del_gendisk").

Since the lo becomes unreachable before del_gendisk() is called by
loop_remove(lo) from loop_control_ioctl(LOOP_CTL_REMOVE), we can drop
loop_ctl_mutex before calling loop_remove().

Holding loop_ctl_mutex in loop_exit() is pointless, for module's refcount
being 0 (unless forced module unload is requested) guarantees that nobody
is using /dev/loop* interface.

Link: https://syzkaller.appspot.com/bug?extid=61e04e51b7ac86930589 [1]
Reported-by: syzbot <syzbot+61e04e51b7ac86930589@syzkaller.appspotmail.com>
Fixes: c76f48eb5c084b1e ("block: take bd_mutex around delete_partitions in del_gendisk")
---
 drivers/block/loop.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d58d68f3c7cd..3045bcb9f7ed 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2079,6 +2079,7 @@ static const struct blk_mq_ops loop_mq_ops = {
 	.complete	= lo_complete_rq,
 };
 
+/* Must be called with loop_ctl_mutex held. */
 static int loop_add(struct loop_device **l, int i)
 {
 	struct loop_device *lo;
@@ -2186,6 +2187,7 @@ static int loop_add(struct loop_device **l, int i)
 	return err;
 }
 
+/* Must not be called with loop_ctl_mutex or lo->lo_mutex held. */
 static void loop_remove(struct loop_device *lo)
 {
 	del_gendisk(lo->lo_disk);
@@ -2288,8 +2290,9 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 		lo->lo_disk->private_data = NULL;
 		mutex_unlock(&lo->lo_mutex);
 		idr_remove(&loop_index_idr, lo->lo_number);
+		mutex_unlock(&loop_ctl_mutex);
 		loop_remove(lo);
-		break;
+		return 0;
 	case LOOP_CTL_GET_FREE:
 		ret = loop_lookup(&lo, -1);
 		if (ret >= 0)
@@ -2397,16 +2400,12 @@ static int loop_exit_cb(int id, void *ptr, void *data)
 
 static void __exit loop_exit(void)
 {
-	mutex_lock(&loop_ctl_mutex);
-
 	idr_for_each(&loop_index_idr, &loop_exit_cb, NULL);
 	idr_destroy(&loop_index_idr);
 
 	unregister_blkdev(LOOP_MAJOR, "loop");
 
 	misc_deregister(&loop_misc);
-
-	mutex_unlock(&loop_ctl_mutex);
 }
 
 module_init(loop_init);
-- 
2.25.1