Re: [PATCH v2 2/2] loop: use task_work for autoclear operation

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

On 2022/01/13 19:44, Jan Kara wrote:
> OK, so I think I understand the lockdep complaint better. Lockdep
> essentially complains about the following scenario:
> 
> blkdev_put()
>   lock disk->open_mutex
>   lo_release
>     __loop_clr_fd()
>         |
>         | wait for IO to complete
>         v
> loop worker
>   write to backing file
>     sb_start_write(file)
>         |
>         | wait for fs with backing file to unfreeze
>         v
> process holding fs frozen
>   freeze_super()
>         |
>         | wait for remaining writers on the fs so that fs can be frozen
>         v
> sendfile()
>   sb_start_write()
>   do_splice_direct()
>         |
>         | blocked on read from /sys/power/resume, waiting for kernfs file
>         | lock
>         v
> write() "/dev/loop0" (in whatever form) to /sys/power/resume
>   calls blkdev_get_by_dev("/dev/loop0")
>     lock disk->open_mutex => deadlock
> 

Then, not calling flush_workqueue() from destroy_workqueue() from __loop_clr_fd() will not help
because the reason we did not need to call flush_workqueue() is that blk_mq_freeze_queue() waits
until all "struct work_struct" which flush_workqueue() would have waited completes?

If flush_workqueue() cannot complete because an I/O request cannot complete, blk_mq_freeze_queue()
as well cannot complete because it waits for I/O requests which flush_workqueue() would have waited?

Then, any attempt to revert commit 322c4293ecc58110 ("loop: make autoclear operation asynchronous")
(e.g. https://lkml.kernel.org/r/4e7b711f-744b-3a78-39be-c9432a3cecd2@i-love.sakura.ne.jp ) cannot be
used?

Now that commit 322c4293ecc58110 already arrived at linux.git, we need to quickly send a fixup patch
for these reported regressions. This "[PATCH v2 2/2] loop: use task_work for autoclear operation" did
not address "if (lo->lo_state == Lo_bound) { }" part. If we address this part, something like below diff?

----------------------------------------
 drivers/block/loop.c |  158 ++++++++++++++++++++++++++++++++++++++-------------
 drivers/block/loop.h |    1 
 2 files changed, 120 insertions(+), 39 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b1b05c45c07c..eb750602c94d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -89,6 +89,8 @@
 static DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_ctl_mutex);
 static DEFINE_MUTEX(loop_validate_mutex);
+static DECLARE_WAIT_QUEUE_HEAD(loop_rundown_wait);
+static DEFINE_SPINLOCK(loop_open_spinlock);
 
 /**
  * loop_global_lock_killable() - take locks for safe loop_validate_file() test
@@ -1172,33 +1174,10 @@ static void loop_rundown_completed(struct loop_device *lo)
 	mutex_lock(&lo->lo_mutex);
 	lo->lo_state = Lo_unbound;
 	mutex_unlock(&lo->lo_mutex);
+	wake_up_all(&loop_rundown_wait);
 	module_put(THIS_MODULE);
 }
 
-static void loop_rundown_workfn(struct work_struct *work)
-{
-	struct loop_device *lo = container_of(work, struct loop_device,
-					      rundown_work);
-	struct block_device *bdev = lo->lo_device;
-	struct gendisk *disk = lo->lo_disk;
-
-	__loop_clr_fd(lo);
-	kobject_put(&bdev->bd_device.kobj);
-	module_put(disk->fops->owner);
-	loop_rundown_completed(lo);
-}
-
-static void loop_schedule_rundown(struct loop_device *lo)
-{
-	struct block_device *bdev = lo->lo_device;
-	struct gendisk *disk = lo->lo_disk;
-
-	__module_get(disk->fops->owner);
-	kobject_get(&bdev->bd_device.kobj);
-	INIT_WORK(&lo->rundown_work, loop_rundown_workfn);
-	queue_work(system_long_wq, &lo->rundown_work);
-}
-
 static int loop_clr_fd(struct loop_device *lo)
 {
 	int err;
@@ -1721,30 +1700,91 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 }
 #endif
 
+struct loop_rundown_waiter {
+	struct callback_head callback;
+	struct loop_device *lo;
+};
+
+static void loop_rundown_waiter_callbackfn(struct callback_head *callback)
+{
+	struct loop_rundown_waiter *lrw =
+		container_of(callback, struct loop_rundown_waiter, callback);
+
+	/*
+	 * Locklessly wait for completion of __loop_clr_fd().
+	 * This should be safe because of the following rules.
+	 *
+	 *  (a) From locking dependency perspective, this function is called
+	 *      without any locks held.
+	 *  (b) From execution ordering perspective, this function is called
+	 *      by the moment lo_open() from open() syscall returns to user
+	 *      mode.
+	 *  (c) From use-after-free protection perspective, this function is
+	 *      called before loop_remove() is called, for lo->lo_refcnt taken
+	 *      by lo_open() prevents loop_control_remove() and loop_exit().
+	 */
+	wait_event_killable(loop_rundown_wait, data_race(lrw->lo->lo_state) != Lo_rundown);
+	kfree(lrw);
+}
+
 static int lo_open(struct block_device *bdev, fmode_t mode)
 {
 	struct loop_device *lo = bdev->bd_disk->private_data;
-	int err;
+	int err = 0;
 
-	err = mutex_lock_killable(&lo->lo_mutex);
-	if (err)
-		return err;
-	if (lo->lo_state == Lo_deleting)
+	/*
+	 * Avoid creating disk->open_mutex => lo->lo_mutex locking chain, for
+	 * calling blk_mq_freeze_queue() with lo->lo_mutex held will form a
+	 * circular locking dependency chain due to waiting for I/O requests
+	 * to complete.
+	 */
+	spin_lock(&loop_open_spinlock);
+	/*
+	 * Since the purpose of lo_open() is to protect this loop device from
+	 * entering Lo_rundown or Lo_deleting state, only serialization against
+	 * loop_control_remove() is sufficient.
+	 */
+	if (data_race(lo->lo_state) == Lo_deleting)
 		err = -ENXIO;
 	else
 		atomic_inc(&lo->lo_refcnt);
-	mutex_unlock(&lo->lo_mutex);
+	spin_unlock(&loop_open_spinlock);
+	/*
+	 * But loop_clr_fd() or __lo_release() might have already set this loop
+	 * device to Lo_rundown state. Since accessing Lo_rundown loop device
+	 * confuses userspace programs, try to wait for __loop_clr_fd() to
+	 * complete without disk->open_mutex held.
+	 */
+	if (!err && !(current->flags & PF_KTHREAD)) {
+		struct loop_rundown_waiter *lrw =
+			kmalloc(sizeof(*lrw), GFP_KERNEL | __GFP_NOFAIL);
+
+		init_task_work(&lrw->callback, loop_rundown_waiter_callbackfn);
+		lrw->lo = lo;
+		if (task_work_add(current, &lrw->callback, TWA_RESUME))
+			kfree(lrw);
+	}
 	return err;
 }
 
-static void lo_release(struct gendisk *disk, fmode_t mode)
+struct loop_release_trigger {
+	union {
+		struct work_struct   work;
+		struct callback_head callback;
+	};
+	struct loop_device *lo;
+};
+
+/* Perform actual creanup if nobody is using this loop device. */
+static void __lo_release(struct loop_release_trigger *lrt)
 {
-	struct loop_device *lo = disk->private_data;
+	struct loop_device *lo = lrt->lo;
+	struct gendisk *disk = lo->lo_disk;
+	bool cleared = false;
 
 	mutex_lock(&lo->lo_mutex);
-	if (atomic_dec_return(&lo->lo_refcnt))
+	if (atomic_read(&lo->lo_refcnt))
 		goto out_unlock;
-
 	if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
 		if (lo->lo_state != Lo_bound)
 			goto out_unlock;
@@ -1754,8 +1794,8 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
 		 * In autoclear mode, stop the loop thread
 		 * and remove configuration after last close.
 		 */
-		loop_schedule_rundown(lo);
-		return;
+		__loop_clr_fd(lo);
+		cleared = true;
 	} else if (lo->lo_state == Lo_bound) {
 		/*
 		 * Otherwise keep thread (if running) and config,
@@ -1764,9 +1804,48 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
 		blk_mq_freeze_queue(lo->lo_queue);
 		blk_mq_unfreeze_queue(lo->lo_queue);
 	}
+ out_unlock:
+	if (!cleared)
+		mutex_unlock(&lo->lo_mutex);
+	module_put(disk->fops->owner);
+	if (cleared)
+		loop_rundown_completed(lo);
+}
 
-out_unlock:
-	mutex_unlock(&lo->lo_mutex);
+static void loop_release_callbackfn(struct callback_head *callback)
+{
+	__lo_release(container_of(callback, struct loop_release_trigger,
+				  callback));
+}
+
+static void loop_release_workfn(struct work_struct *work)
+{
+	__lo_release(container_of(work, struct loop_release_trigger, work));
+}
+
+static void lo_release(struct gendisk *disk, fmode_t mode)
+{
+	struct loop_device *lo = disk->private_data;
+	struct loop_release_trigger *lrt;
+
+	if (atomic_dec_return(&lo->lo_refcnt))
+		return;
+	/*
+	 * In order to avoid waiting for I/O requests to complete under
+	 * disk->open_mutex held, defer actual clreanup to __lo_release().
+	 * Prefer task work context if possible in order to make sure that
+	 * __lo_release() completes before close() returns to user mode,
+	 */
+	lrt = kmalloc(sizeof(*lrt), GFP_KERNEL | __GFP_NOFAIL);
+	lrt->lo = lo;
+	__module_get(disk->fops->owner);
+	if (!(current->flags & PF_KTHREAD)) {
+		init_task_work(&lrt->callback, loop_release_callbackfn);
+		if (!task_work_add(current, &lrt->callback, TWA_RESUME))
+			return;
+	}
+	INIT_WORK(&lrt->work, loop_release_workfn);
+	queue_work(system_long_wq, &lrt->work);
 }
 
 static const struct block_device_operations lo_fops = {
@@ -2119,14 +2198,17 @@ static int loop_control_remove(int idx)
 	ret = mutex_lock_killable(&lo->lo_mutex);
 	if (ret)
 		goto mark_visible;
+	spin_lock(&loop_open_spinlock);
 	if (lo->lo_state != Lo_unbound ||
 	    atomic_read(&lo->lo_refcnt) > 0) {
+		spin_unlock(&loop_open_spinlock);
 		mutex_unlock(&lo->lo_mutex);
 		ret = -EBUSY;
 		goto mark_visible;
 	}
 	/* Mark this loop device no longer open()-able. */
 	lo->lo_state = Lo_deleting;
+	spin_unlock(&loop_open_spinlock);
 	mutex_unlock(&lo->lo_mutex);
 
 	loop_remove(lo);
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index 918a7a2dc025..082d4b6bfc6a 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -56,7 +56,6 @@ struct loop_device {
 	struct gendisk		*lo_disk;
 	struct mutex		lo_mutex;
 	bool			idr_visible;
-	struct work_struct      rundown_work;
 };
 
 struct loop_cmd {
----------------------------------------

This diff avoids disk->open_mutex => lo->lo_mutex chain.

ffffffff84a06140 OPS:    7944 FD:  305 BD:    4 +.+.: &disk->open_mutex
 -> [ffffffffa0158a48] sd_ref_mutex
 -> [ffffffff831be200] fs_reclaim
 -> [ffffffff849d7ee0] &n->list_lock
 -> [ffffffff8332ce18] depot_lock
 -> [ffffffff8312d508] tk_core.seq.seqcount
 -> [ffffffff84a06940] &obj_hash[i].lock
 -> [ffffffff84a05680] &hctx->lock
 -> [ffffffff84a05620] &x->wait#20
 -> [ffffffff849c10e0] &base->lock
 -> [ffffffff83d93000] &rq->__lock
 -> [ffffffff849c1060] (&timer.timer)
 -> [ffff88811ac44b48] lock#2
 -> [ffffffff849cebe0] &____s->seqcount
 -> [ffffffff84a05420] &q->sysfs_dir_lock
 -> [ffffffff84a04980] &bdev->bd_size_lock
 -> [ffffffff831bcdd8] free_vmap_area_lock
 -> [ffffffff831bcd78] vmap_area_lock
 -> [ffffffff849e5900] &xa->xa_lock#3
 -> [ffff88811ac44390] lock#6
 -> [ffffffff849e58f0] &mapping->private_lock
 -> [ffffffff84a06740] &dd->lock
 -> [ffffffff82ef8d78] (console_sem).lock
 -> [ffffffff83307c28] &sb->s_type->i_lock_key#3
 -> [ffffffff849e2420] &s->s_inode_list_lock
 -> [ffffffff831a9f68] pcpu_alloc_mutex
 -> [ffffffff84b8dd60] &x->wait#9
 -> [ffffffff84b77980] &k->list_lock
 -> [ffff88811ac45780] lock#3
 -> [ffffffff849ea3e0] &root->kernfs_rwsem
 -> [ffffffff8335b830] bus_type_sem
 -> [ffffffff8321b1b8] sysfs_symlink_target_lock
 -> [ffffffff84b8d7c0] &dev->power.lock
 -> [ffffffff833e2e28] dpm_list_mtx
 -> [ffffffff833e03b8] req_lock
 -> [ffffffff83d8e820] &p->pi_lock
 -> [ffffffff84b8dbc0] &x->wait#10
 -> [ffffffff84b77960] &k->k_lock
 -> [ffffffff84a06180] subsys mutex#48
 -> [ffffffff84a061a0] &xa->xa_lock#5
 -> [ffffffff82e14698] inode_hash_lock
 -> [ffffffff831bcf98] purge_vmap_area_lock
 -> [ffffffff849cd420] &lruvec->lru_lock
 -> [ffffffff849ccec0] &folio_wait_table[i]
 -> [ffffffff83410a68] sr_ref_mutex
 -> [ffffffff84a06240] &ev->block_mutex
 -> [ffffffff84a06220] &ev->lock
 -> [ffffffff831e7398] sb_lock
 -> [ffffffff84b8f280] &cd->lock
 -> [ffffffff82ea6878] pgd_lock
 -> [ffffffff82e14758] bdev_lock
 -> [ffffffff849ce0e0] &wb->list_lock
 -> [ffffffffa04b9378] loop_open_spinlock
 -> [ffffffff83d8e800] &____s->seqcount#2
 -> [ffffffff849c0b00] rcu_node_0
 -> [ffffffff83d94320] &lock->wait_lock

ffffffffa04b90c0 OPS:    3257 FD:  280 BD:    1 +.+.: &lo->lo_mutex
 -> [ffffffff831be200] fs_reclaim
 -> [ffffffff849d7ee0] &n->list_lock
 -> [ffffffff8332ce18] depot_lock
 -> [ffffffff82ec1450] cpu_hotplug_lock
 -> [ffffffff82ed3848] wq_pool_mutex
 -> [ffffffff83330448] uevent_sock_mutex
 -> [ffffffff84a06940] &obj_hash[i].lock
 -> [ffffffff831e7398] sb_lock
 -> [ffffffff83d8e800] &____s->seqcount#2
 -> [ffff88811ac44b48] lock#2
 -> [ffffffff849cebe0] &____s->seqcount
 -> [ffff88811ac45780] lock#3
 -> [ffffffff849ea3e0] &root->kernfs_rwsem
 -> [ffffffff83d94330] &sem->wait_lock
 -> [ffffffff83d8e820] &p->pi_lock
 -> [ffffffff84a04980] &bdev->bd_size_lock
 -> [ffffffff82ef8d78] (console_sem).lock
 -> [ffffffff84a05480] &q->mq_freeze_lock
 -> [ffffffff83322e18] percpu_ref_switch_lock
 -> [ffffffff84a05460] &q->mq_freeze_wq
 -> [ffffffff83d93000] &rq->__lock
 -> [ffffffff831cf1a0] quarantine_lock
 -> [ffffffff849e57a0] &dentry->d_lock
 -> [ffffffff83d94320] &lock->wait_lock
 -> [ffffffff83110f98] console_owner_lock

I think lo_open() and lo_release() are doing too much things. I wish "struct block_device_operations"
provides open() and release() callbacks without disk->open_mutex held...