Re: [syzbot] possible deadlock in del_gendisk

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

On 2021/06/10 1:46, Tyler Hicks wrote:
> On 2021-06-10 01:31:17, Tetsuo Handa wrote:
>> Hello, Christoph.
>>
>> I'm currently trying full bisection.

I reached commit c76f48eb5c084b1e ("block: take bd_mutex around delete_partitions in del_gendisk").

  # bad: [fc0586062816559defb14c947319ef8c4c326fb3] Merge tag 'for-5.13/drivers-2021-04-27' of git://git.kernel.dk/linux-block
  # good: [42dec9a936e7696bea1f27d3c5a0068cd9aa95fd] Merge tag 'perf-core-2021-04-28' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
  # good: [68a32ba14177d4a21c4a9a941cf1d7aea86d436f] Merge tag 'drm-next-2021-04-28' of git://anongit.freedesktop.org/drm/drm
  # good: [a800abd3ecb9acc55821f7ac9bba6c956b36a595] net: enetc: move skb creation into enetc_build_skb
  # good: [6cc8e7430801fa238bd7d3acae1eb406c6e02fe1] loop: scale loop device by introducing per device lock
  git bisect start 'fc0586062816559defb14c947319ef8c4c326fb3' '42dec9a936e7696bea1f27d3c5a0068cd9aa95fd' '68a32ba14177d4a21c4a9a941cf1d7aea86d436f' 'a800abd3ecb9acc55821f7ac9bba6c956b36a595' '6cc8e7430801fa238bd7d3acae1eb406c6e02fe1'
  # good: [2958a995edc94654df690318df7b9b49e5a3ef88] block/rnbd-clt: Support polling mode for IO latency optimization
  git bisect good 2958a995edc94654df690318df7b9b49e5a3ef88
  # good: [16b3d0cf5bad844daaf436ad2e9061de0fe36e5c] Merge tag 'sched-core-2021-04-28' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
  git bisect good 16b3d0cf5bad844daaf436ad2e9061de0fe36e5c
  # bad: [cbb749cf377aa8aa32a036ebe9dd9f2d89037bf0] block: remove an incorrect check from blk_rq_append_bio
  git bisect bad cbb749cf377aa8aa32a036ebe9dd9f2d89037bf0
  # good: [9bb33f24abbd0fa2fadad01ec75438d7cc239189] block: refactor the bounce buffering code
  git bisect good 9bb33f24abbd0fa2fadad01ec75438d7cc239189
  # bad: [c76f48eb5c084b1e15c931ae8cc1826cd771d70d] block: take bd_mutex around delete_partitions in del_gendisk
  git bisect bad c76f48eb5c084b1e15c931ae8cc1826cd771d70d
  # good: [b896fa85e0ee4f09ba4be48a3f405fc82c38afb4] dasd: use bdev_disk_changed instead of blk_drop_partitions
  git bisect good b896fa85e0ee4f09ba4be48a3f405fc82c38afb4
  # good: [473338be3aaea117a7133720305f240eb7f68951] block: move more syncing and invalidation to delete_partition
  git bisect good 473338be3aaea117a7133720305f240eb7f68951
  # good: [d3c4a43d9291279c28b26757351a6ab72c110753] block: refactor blk_drop_partitions
  git bisect good d3c4a43d9291279c28b26757351a6ab72c110753
  # first bad commit: [c76f48eb5c084b1e15c931ae8cc1826cd771d70d] block: take bd_mutex around delete_partitions in del_gendisk

> 
> Thanks for doing this. I haven't had a chance to retry this commit with
> lockdep but I did re-review it and didn't think that it would be the
> cause of this lockdep report since it strictly reduced the usage of the
> loop_ctl_mutex.

OK. It seems that a patch shown below avoids this warning.

Since I think loop_remove() does not access resources in
loop module, I think we can drop loop_ctl_mutex as soon as
idr_remove(&loop_index_idr, lo->lo_number) completes.

Hmm, what protects lo_open() from loop_exit() destroying everything?
Is module's refcount taken before calling lo_open() ? If yes, does
module_exiting and/or loop_ctl_mutex in loop_exit() help protecting
something? If no, nothing protects and just crashes the kernel?

Is my understanding correct? Do you think this approach is OK?

 drivers/block/loop.c |   32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d58d68f3c7cd..5baa0207512e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -80,18 +80,19 @@
 #include <linux/ioprio.h>
 #include <linux/blk-cgroup.h>
 
 #include "loop.h"
 
 #include <linux/uaccess.h>
 
 static DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_ctl_mutex);
+static bool module_exiting;
 
 static int max_part;
 static int part_shift;
 
 static int transfer_xor(struct loop_device *lo, int cmd,
 			struct page *raw_page, unsigned raw_off,
 			struct page *loop_page, unsigned loop_off,
 			int size, sector_t real_block)
 {
@@ -1877,25 +1878,29 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 }
 #endif
 
 static int lo_open(struct block_device *bdev, fmode_t mode)
 {
 	struct loop_device *lo;
 	int err;
 
 	/*
-	 * take loop_ctl_mutex to protect lo pointer from race with
-	 * loop_control_ioctl(LOOP_CTL_REMOVE), however, to reduce contention
-	 * release it prior to updating lo->lo_refcnt.
+	 * Take loop_ctl_mutex to protect lo pointer from race with
+	 * loop_control_ioctl(LOOP_CTL_REMOVE) and loop_exit(). However,
+	 * to reduce contention, release it prior to updating lo->lo_refcnt.
 	 */
 	err = mutex_lock_killable(&loop_ctl_mutex);
 	if (err)
 		return err;
+	if (unlikely(module_exiting)) {
+		mutex_unlock(&loop_ctl_mutex);
+		return -EINVAL;
+	}
 	lo = bdev->bd_disk->private_data;
 	if (!lo) {
 		mutex_unlock(&loop_ctl_mutex);
 		return -ENXIO;
 	}
 	err = mutex_lock_killable(&lo->lo_mutex);
 	mutex_unlock(&loop_ctl_mutex);
 	if (err)
 		return err;
@@ -2073,18 +2078,19 @@ static int loop_init_request(struct blk_mq_tag_set *set, struct request *rq,
 	return 0;
 }
 
 static const struct blk_mq_ops loop_mq_ops = {
 	.queue_rq       = loop_queue_rq,
 	.init_request	= loop_init_request,
 	.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;
 	struct gendisk *disk;
 	int err;
 
 	err = -ENOMEM;
 	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
 	if (!lo)
@@ -2180,18 +2186,19 @@ static int loop_add(struct loop_device **l, int i)
 	blk_mq_free_tag_set(&lo->tag_set);
 out_free_idr:
 	idr_remove(&loop_index_idr, i);
 out_free_dev:
 	kfree(lo);
 out:
 	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);
 	blk_cleanup_queue(lo->lo_queue);
 	blk_mq_free_tag_set(&lo->tag_set);
 	put_disk(lo->lo_disk);
 	mutex_destroy(&lo->lo_mutex);
 	kfree(lo);
 }
@@ -2251,18 +2258,22 @@ static void loop_probe(dev_t dev)
 static long loop_control_ioctl(struct file *file, unsigned int cmd,
 			       unsigned long parm)
 {
 	struct loop_device *lo;
 	int ret;
 
 	ret = mutex_lock_killable(&loop_ctl_mutex);
 	if (ret)
 		return ret;
+	if (unlikely(module_exiting)) {
+		mutex_unlock(&loop_ctl_mutex);
+		return -EINVAL;
+	}
 
 	ret = -ENOSYS;
 	switch (cmd) {
 	case LOOP_CTL_ADD:
 		ret = loop_lookup(&lo, parm);
 		if (ret >= 0) {
 			ret = -EEXIST;
 			break;
 		}
@@ -2282,19 +2293,21 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 		}
 		if (atomic_read(&lo->lo_refcnt) > 0) {
 			ret = -EBUSY;
 			mutex_unlock(&lo->lo_mutex);
 			break;
 		}
 		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);
+		return ret;
 		break;
 	case LOOP_CTL_GET_FREE:
 		ret = loop_lookup(&lo, -1);
 		if (ret >= 0)
 			break;
 		ret = loop_add(&lo, -1);
 	}
 	mutex_unlock(&loop_ctl_mutex);
 
@@ -2391,28 +2404,35 @@ static int loop_exit_cb(int id, void *ptr, void *data)
 {
 	struct loop_device *lo = ptr;
 
 	loop_remove(lo);
 	return 0;
 }
 
 static void __exit loop_exit(void)
 {
-	mutex_lock(&loop_ctl_mutex);
+	module_exiting = true;
 
+	/* Block future lo_open()/loop_control_ioctl() callers. */
+	mutex_lock(&loop_ctl_mutex);
+	mutex_unlock(&loop_ctl_mutex);
+	/*
+	 * Since loop_exit_cb() calls loop_remove(), we can not take
+	 * loop_ctl_mutex here. But since module_exiting == true is blocking
+	 * lo_open() and loop_control_ioctl(), it is safe to access
+	 * loop_index_idr without taking loop_ctl_mutex here.
+	 */
 	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);
 module_exit(loop_exit);
 
 #ifndef MODULE
 static int __init max_loop_setup(char *str)
 {
 	max_loop = simple_strtol(str, NULL, 0);