loop: defer the queue limits clear to a workqueue

From: Tao Cui <[email protected]>

loop_clear_limits() calls queue_limits_commit_update() directly from the loop workqueue that processes the request. That does a non-atomic struct assignment to q->limits without freezing the queue, which races with lockless readers of q->limits on other CPUs - bio splitting reads max_hw_sectors, the discard path reads max_hw_discard_sectors - and can let them observe torn values. The trigger is a discard or write-zeroes request on a loop device whose backing file does not support the corresponding fallocate operation.

The code already has an XXX comment saying this should move to a workqueue. Do that: schedule a work item on the system workqueue, where it is safe to freeze the queue around the limits update. The pending modes and the rebind generation live under a new mutex, lo->clear_limits_lock. The work item takes the mutex with the queue frozen and holds it over the limits commit, so a rebind cannot slip in between the generation check and the commit. Rebinding the device drops the accumulated modes and invalidates an already scheduled work item, so a stale clear cannot hit the new backing file. The work item is cancelled before the device is freed.

Suggested-by Bart Van Assche <[email protected]>

Signed-off-by Tao Cui <[email protected]>

---
Changes since v4:

- Rebase onto the current block tree as a single commit.

- Hold clear_limits_lock over the limits commit in the work item. The queue freeze is counted, not mutually exclusive, so loop_change_fd() could rebind between the generation check and the commit, and a stale clear could disable discard and write zeroes on the new backing file.

Changes since v3:

- Replace the three atomic variables (clear_limits_mode, rebind_gen, clear_limits_gen) with plain variables protected by a new clear_limits_lock mutex, as suggested by Bart. The mutex is taken after the queue freeze in the work item, which keeps the same freeze -> mutex ordering as loop_change_fd(), the only rebinding path that freezes the queue.

Changes since v2:

- Skip the clear when the device was rebound since the work item was scheduled: the modes used to be captured before the freeze, so a LOOP_CHANGE_FD completing in between could apply the old modes to the new backing file. The rebind generation is now checked with the queue frozen, which excludes loop_change_fd() because it assigns the new backing file under the same freeze, so a rebind cannot slip in between the check and the commit. Cancelling from loop_assign_backing_file() would instead deadlock on the freeze held by loop_change_fd().

- Also bump the rebind generation from __loop_clr_fd(): unbinding does not go through loop_assign_backing_file(), so a work item scheduled before the last close could otherwise commit a stale clear to the queue limits of the unbound device.

Changes since v1:

- Reset clear_limits_mode when assigning a new backing file, so stale modes do not clear limits of the new file.

- Cancel the work item from loop_remove() before del_gendisk():
the queue can already be in RCU-delayed freeing when lo_free_disk() cancels it.

Tested on x86-64 (qemu, vfat-backed loop device): a 30s discard and reconfigure loop exercises the clear path 288 times, no torn sysfs reads, no difference against the unpatched kernel.

Link https://lore.kernel.org/r/[email protected]/ open here

---
 drivers/block/loop.c | 83 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 69 insertions(+), 14 deletions(-)

drivers/block/loop.c +69 -14

index 758c20678bf6c..0b89036c982ad 100644
@@ -67,6 +67,11 @@ struct loop_device {
 	struct list_head        rootcg_cmd_list;
 	struct list_head        idle_worker_list;
 	struct rb_root          worker_tree;
+	struct work_struct      clear_limits_work;
+	struct mutex		clear_limits_lock;
+	unsigned int		clear_limits_mode;
+	unsigned int		rebind_gen;
+	unsigned int		clear_limits_gen;
 	struct timer_list       timer;
 	bool			sysfs_inited;
 

@@ -222,26 +227,53 @@ static void loop_set_size(struct loop_device *lo, loff_t size)
 		kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 }
 
-static void loop_clear_limits(struct loop_device *lo, int mode)
+static void loop_clear_limits_workfn(struct work_struct *work)
 {
+	struct loop_device *lo =
+		container_of(work, struct loop_device, clear_limits_work);
 	struct queue_limits lim = queue_limits_start_update(lo->lo_queue);
-
-	if (mode & FALLOC_FL_ZERO_RANGE)
-		lim.max_write_zeroes_sectors = 0;
-
-	if (mode & FALLOC_FL_PUNCH_HOLE) {
-		lim.max_hw_discard_sectors = 0;
-		lim.discard_granularity = 0;
-	}
+	unsigned int memflags;
+	int mode = 0;
 
 	/*
-	 * XXX: this updates the queue limits without freezing the queue, which
-	 * is against the locking protocol and dangerous.  But we can't just
-	 * freeze the queue as we're inside the ->queue_rq method here.  So this
-	 * should move out into a workqueue unless we get the file operations to
-	 * advertise if they support specific fallocate operations.
+	 * Commit the unmodified limits if the device was rebound since
+	 * the work item was scheduled.  The freeze does not exclude a
+	 * rebind through loop_change_fd(), which freezes the queue
+	 * itself, so hold clear_limits_lock over the generation check
+	 * and the commit: loop_assign_backing_file() bumps rebind_gen
+	 * under the same mutex, so a rebind either precedes the check
+	 * or follows the commit, and a stale clear cannot hit the new
+	 * backing file.  The other rebinding paths, loop_configure()
+	 * and __loop_clr_fd(), bump rebind_gen under the same mutex
+	 * without freezing the queue; the generation check detects
+	 * them as well.
 	 */
+	memflags = blk_mq_freeze_queue(lo->lo_queue);
+	mutex_lock(&lo->clear_limits_lock);
+	if (lo->clear_limits_gen == lo->rebind_gen) {
+		mode = lo->clear_limits_mode;
+		lo->clear_limits_mode = 0;
+
+		if (mode & FALLOC_FL_ZERO_RANGE)
+			lim.max_write_zeroes_sectors = 0;
+
+		if (mode & FALLOC_FL_PUNCH_HOLE) {
+			lim.max_hw_discard_sectors = 0;
+			lim.discard_granularity = 0;
+		}
+	}
 	queue_limits_commit_update(lo->lo_queue, &lim);
+	mutex_unlock(&lo->clear_limits_lock);
+	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
+}
+
+static void loop_clear_limits(struct loop_device *lo, int mode)
+{
+	mutex_lock(&lo->clear_limits_lock);
+	lo->clear_limits_gen = lo->rebind_gen;
+	lo->clear_limits_mode |= mode;
+	mutex_unlock(&lo->clear_limits_lock);
+	schedule_work(&lo->clear_limits_work);
 }
 
 static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,

@@ -518,6 +550,10 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
 static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
 {
 	lo->lo_backing_file = file;
+	mutex_lock(&lo->clear_limits_lock);
+	lo->rebind_gen++;
+	lo->clear_limits_mode = 0;
+	mutex_unlock(&lo->clear_limits_lock);
 	lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
 	mapping_set_gfp_mask(file->f_mapping,
 			lo->old_gfp_mask & ~(__GFP_IO | __GFP_FS));

@@ -1148,6 +1184,15 @@ static void __loop_clr_fd(struct loop_device *lo)
 	lo->lo_backing_file = NULL;
 	spin_unlock_irq(&lo->lo_lock);
 
+	/*
+	 * Invalidate any pending clear that was scheduled against the old
+	 * backing file, like loop_assign_backing_file() does on rebind.
+	 */
+	mutex_lock(&lo->clear_limits_lock);
+	lo->rebind_gen++;
+	lo->clear_limits_mode = 0;
+	mutex_unlock(&lo->clear_limits_lock);
+
 	lo->lo_device = NULL;
 	lo->lo_offset = 0;
 	lo->lo_sizelimit = 0;

@@ -1783,7 +1828,9 @@ static void lo_free_disk(struct gendisk *disk)
 		destroy_workqueue(lo->workqueue);
 	loop_free_idle_workers(lo, true);
 	timer_shutdown_sync(&lo->timer);
+	cancel_work_sync(&lo->clear_limits_work);
 	mutex_destroy(&lo->lo_mutex);
+	mutex_destroy(&lo->clear_limits_lock);
 	kfree(lo);
 }
 

@@ -2102,6 +2149,8 @@ static int loop_add(int i)
 	spin_lock_init(&lo->lo_lock);
 	spin_lock_init(&lo->lo_work_lock);
 	INIT_WORK(&lo->rootcg_work, loop_rootcg_workfn);
+	INIT_WORK(&lo->clear_limits_work, loop_clear_limits_workfn);
+	mutex_init(&lo->clear_limits_lock);
 	INIT_LIST_HEAD(&lo->rootcg_cmd_list);
 	disk->major		= LOOP_MAJOR;
 	disk->first_minor	= i << part_shift;

@@ -2140,6 +2189,12 @@ static int loop_add(int i)
 
 static void loop_remove(struct loop_device *lo)
 {
+	/*
+	 * Cancel early: the queue may already be in RCU-delayed freeing
+	 * by the time lo_free_disk() cancels the work item.
+	 */
+	cancel_work_sync(&lo->clear_limits_work);
+
 	/* Make this loop device unreachable from pathname. */
 	del_gendisk(lo->lo_disk);
 	blk_mq_free_tag_set(&lo->tag_set);

drivers/block/loop.c +4 -0

+	memflags = blk_mq_freeze_queue(lo->lo_queue);
+	mutex_lock(&lo->clear_limits_lock);
+	if (lo->clear_limits_gen == lo->rebind_gen) {
+		mode = lo->clear_limits_mode;

Quoted 12 lines · Tao Cui

+    memflags = blk_mq_freeze_queue(lo->lo_queue); +    mutex_lock(&lo->clear_limits_lock);
+    if (lo->clear_limits_gen == lo->rebind_gen) { +        mode = lo->clear_limits_mode;
+        lo->clear_limits_mode = 0;

drivers/block/loop.c +7 -0

+
+        if (mode & FALLOC_FL_ZERO_RANGE)
+            lim.max_write_zeroes_sectors = 0;
+
+        if (mode & FALLOC_FL_PUNCH_HOLE) {
+            lim.max_hw_discard_sectors = 0;
+            lim.discard_granularity = 0;

Quoted 2 lines

+        }
+    }

I propose to remove the member variables clear_limits_gen and rebind_gen. These member variables complicate verifying correctness of the code and IMHO are not necessary. If loop_clear_limits() races with loop_clear_limits_workfn(), the work will be rescheduled and loop_clear_limits_workfn() will be called another time.

Quoted 2 lines

      queue_limits_commit_update(lo->lo_queue, &lim); +    mutex_unlock(&lo->clear_limits_lock);

Since the mutex_lock(&lo->clear_limits_lock) call occurs after queue_limits_start_update(), please move the mutex_unlock(&lo->clear_limits_lock) call above the queue_limits_commit_update() call. This has no impact on correctness nor on the lock nesting order but makes the code easier to follow.

Thanks,

Bart.

Quoted 11 lines · Bart Van Assche

Quoted 2 lines · Tao Cui

      queue_limits_commit_update(lo->lo_queue, &lim);
+    mutex_unlock(&lo->clear_limits_lock);

Since the mutex_lock(&lo->clear_limits_lock) call occurs after queue_limits_start_update(), please move the mutex_unlock(&lo->clear_limits_lock) call above the queue_limits_commit_update() call. This has no impact on correctness nor on the lock nesting order but makes the code easier to follow.

On moving mutex_unlock() above queue_limits_commit_update(): with the generation counters removed, the mutex is what keeps consuming clear_limits_mode and committing the resulting queue limits atomic with respect to a rebind. blk_mq_freeze_queue() is reference-counted, so the workfn and loop_change_fd() can both hold a freeze concurrently. If the workfn drops the mutex after consuming a non-zero mode but before queue_limits_commit_update(), loop_change_fd() could install a new backing file in that window, and the pending clear would then be committed to the new file. Since loop_change_fd() does not recompute the queue limits afterward, the incorrectly cleared discard limits would persist until the next reconfiguration.

For that reason, I'd prefer to keep the mutex held until after queue_limits_commit_update(). I can add a comment explaining this if that would help.

Quoted 26 lines · Tao Cui

Quoted 11 lines · Bart Van Assche

Quoted 2 lines · Tao Cui

       queue_limits_commit_update(lo->lo_queue, &lim); +    mutex_unlock(&lo->clear_limits_lock);

Since the mutex_lock(&lo->clear_limits_lock) call occurs after queue_limits_start_update(), please move the mutex_unlock(&lo->clear_limits_lock) call above the queue_limits_commit_update() call. This has no impact on correctness nor on the lock nesting order but makes the code easier to follow.

On moving mutex_unlock() above queue_limits_commit_update(): with the generation counters removed, the mutex is what keeps consuming clear_limits_mode and committing the resulting queue limits atomic with respect to a rebind. blk_mq_freeze_queue() is reference-counted, so the workfn and loop_change_fd() can both hold a freeze concurrently. If the workfn drops the mutex after consuming a non-zero mode but before queue_limits_commit_update(), loop_change_fd() could install a new backing file in that window, and the pending clear would then be committed to the new file. Since loop_change_fd() does not recompute the queue limits afterward, the incorrectly cleared discard limits would persist until the next reconfiguration.

Quoted 3 lines

For that reason, I'd prefer to keep the mutex held until after queue_limits_commit_update(). I can add a comment explaining this if that would help.

This seems fragile to me. Can this race be solved by calling cancel_work_sync(&lo->clear_limits_work) before the backing file is changed?

Thanks,

Bart.