[PATCH] loop: drop loop_ctl_mutex around del_gendisk() in loop_remove()
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: 2021-06-11 15:14:30
Subsystem:
block layer, the rest · Maintainers:
Jens Axboe, Linus Torvalds
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").
But calling del_gendisk() from loop_remove() without loop_ctl_mutex held
triggers a different race problem regarding sysfs entry management. We
somehow need to serialize "add_disk() from loop_add()" and "del_gendisk()
from loop_remove()". Fortunately, since loop_control_ioctl() is called
with no locks held, we can use "sleep and retry" approach without risking
deadlock.
Since "struct loop_device"->lo_disk->private_data is set to non-NULL at
loop_add() and is reset to NULL before calling loop_remove(), we can use
it as a flag for taking appropriate action ("sleep and retry" or "skip")
when loop_remove() is in progress.
Link: https://syzkaller.appspot.com/bug?extid=61e04e51b7ac86930589 [1]
Reported-by: syzbot <redacted>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <redacted>
Fixes: c76f48eb5c084b1e ("block: take bd_mutex around delete_partitions in del_gendisk")
---
drivers/block/loop.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d58d68f3c7cd..d4c9567b2904 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c@@ -2188,7 +2188,9 @@ static int loop_add(struct loop_device **l, int i) static void loop_remove(struct loop_device *lo) { + mutex_unlock(&loop_ctl_mutex); del_gendisk(lo->lo_disk); + mutex_lock(&loop_ctl_mutex); blk_cleanup_queue(lo->lo_queue); blk_mq_free_tag_set(&lo->tag_set); put_disk(lo->lo_disk);
@@ -2201,7 +2203,7 @@ static int find_free_cb(int id, void *ptr, void *data) struct loop_device *lo = ptr; struct loop_device **l = data; - if (lo->lo_state == Lo_unbound) { + if (lo->lo_state == Lo_unbound && lo->lo_disk->private_data) { *l = lo; return 1; }
@@ -2254,6 +2256,13 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd, struct loop_device *lo; int ret; + if (0) { +unlock_and_retry: + mutex_unlock(&loop_ctl_mutex); + if (schedule_timeout_killable(HZ / 10)) + return -EINTR; + } + debug_check_no_locks_held(); ret = mutex_lock_killable(&loop_ctl_mutex); if (ret) return ret;
@@ -2263,6 +2272,8 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd, case LOOP_CTL_ADD: ret = loop_lookup(&lo, parm); if (ret >= 0) { + if (!lo->lo_disk->private_data) + goto unlock_and_retry; ret = -EEXIST; break; }
@@ -2272,6 +2283,8 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd, ret = loop_lookup(&lo, parm); if (ret < 0) break; + if (!lo->lo_disk->private_data) + goto unlock_and_retry; ret = mutex_lock_killable(&lo->lo_mutex); if (ret) break;
@@ -2286,9 +2299,10 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd, break; } lo->lo_disk->private_data = NULL; + parm = lo->lo_number; mutex_unlock(&lo->lo_mutex); - idr_remove(&loop_index_idr, lo->lo_number); loop_remove(lo); + idr_remove(&loop_index_idr, parm); break; case LOOP_CTL_GET_FREE: ret = loop_lookup(&lo, -1);
--
2.25.1