On 2021/11/11 9:55, Dan Schatzberg wrote:
>> . Can you somehow call destroy_workqueue() without holding a lock (e.g.
>> breaking &lo->lo_mutex => (wq_completion)loop$M dependency chain by
>> making sure that below change is safe) ?
>
> It's really not clear to me - the lo_mutex protects a lot of entry
> points (ioctls and others) and it's hard to tell if the observed state
> will be sane if they can race in the middle of __loop_clr_fd.
>
I'm not familiar with the block layer, but I think it is clear.
We check lo_state with lo_mutex held.
The loop functions fail if lo_state is not what each function expected.
Is blk_mq_freeze_queue() for waiting for "loop_queue_work() from loop_queue_rq()" to
complete and for blocking further loop_queue_rq() until blk_mq_unfreeze_queue() ?
If yes, we need to call blk_mq_freeze_queue() before destroy_workqueue().
But I think lo_state is helping us, like a patch shown below.
>From e36f23c081e0b8ed08239f4e3fbc954b4d7d3feb Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Date: Thu, 11 Nov 2021 23:55:43 +0900
Subject: [PATCH] loop: destroy workqueue without holding locks
syzbot is reporting circular locking problem at __loop_clr_fd() [1], for
commit 87579e9b7d8dc36e ("loop: use worker per cgroup instead of kworker")
is calling destroy_workqueue() with lo->lo_mutex held.
Since all functions where lo->lo_state matters are already checking
lo->lo_state with lo->lo_mutex held (in order to avoid racing with e.g.
ioctl(LOOP_CTL_REMOVE)), and __loop_clr_fd() can be called from either
ioctl(LOOP_CLR_FD) or close(), lo->lo_state == Lo_rundown is considered
as an exclusive lock for __loop_clr_fd(). Therefore, it is safe to
temporarily drop lo->lo_mutex when calling destroy_workqueue().
Link: https://syzkaller.appspot.com/bug?extid=63614029dfb79abd4383 [1]
Reported-by: syzbot <syzbot+63614029dfb79abd4383@xxxxxxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
---
drivers/block/loop.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index a154cab6cd98..b98ec1c2d950 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1104,16 +1104,20 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
*/
mutex_lock(&lo->lo_mutex);
- if (WARN_ON_ONCE(lo->lo_state != Lo_rundown)) {
- err = -ENXIO;
- goto out_unlock;
- }
+ /*
+ * Since this function is called upon "ioctl(LOOP_CLR_FD)" xor "close()
+ * after ioctl(LOOP_CLR_FD)", it is a sign of something going wrong if
+ * lo->lo_state has changed while waiting for lo->lo_mutex.
+ */
+ BUG_ON(lo->lo_state != Lo_rundown);
+ /*
+ * Since ioctl(LOOP_CLR_FD) depends on lo->lo_state == Lo_bound, it is
+ * a sign of something going wrong if lo->lo_backing_file was not
+ * assigned by ioctl(LOOP_SET_FD) or ioctl(LOOP_CONFIGURE).
+ */
filp = lo->lo_backing_file;
- if (filp == NULL) {
- err = -EINVAL;
- goto out_unlock;
- }
+ BUG_ON(!filp);
if (test_bit(QUEUE_FLAG_WC, &lo->lo_queue->queue_flags))
blk_queue_write_cache(lo->lo_queue, false, false);
@@ -1121,7 +1125,20 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
/* freeze request queue during the transition */
blk_mq_freeze_queue(lo->lo_queue);
+ /*
+ * To avoid circular locking dependency, call destroy_workqueue()
+ * without holding lo->lo_mutex.
+ */
+ mutex_unlock(&lo->lo_mutex);
destroy_workqueue(lo->workqueue);
+ mutex_lock(&lo->lo_mutex);
+
+ /*
+ * As explained above, lo->lo_state cannot be changed while waiting for
+ * lo->lo_mutex.
+ */
+ BUG_ON(lo->lo_state != Lo_rundown);
+
spin_lock_irq(&lo->lo_work_lock);
list_for_each_entry_safe(worker, pos, &lo->idle_worker_list,
idle_list) {
@@ -1156,7 +1173,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
lo_number = lo->lo_number;
disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);
-out_unlock:
mutex_unlock(&lo->lo_mutex);
if (partscan) {
/*
--
2.18.4
