loop: defer the queue limits clear to a workqueue

PATCH Superseded Tao Cui 2 participants 6 messages Aug 28 – Sep 1 Block layer

lore

Share & tools

lore

Tao Cui

author

2026-08-28 07:20 UTC #0 lore raw

PATCH 1/1 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 and update the limits using queue_limits_commit_update_frozen(). Accumulate pending modes in lo->clear_limits_mode so that failures between scheduling and execution of the work item are not lost, and cancel the work item before the device is freed. If the device is reconfigured to a backing file that does support the operation in that window, the stale clear takes effect and discard is disabled until the next reconfiguration.

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

---
 drivers/block/loop.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

drivers/block/loop.c +15 -9

index 6f12976035b0..5a1e8b6794ed 100644
@@ -67,6 +67,8 @@ 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;
+	int			clear_limits_mode;
 	struct timer_list       timer;
 	bool			sysfs_inited;

@@ -222,9 +224,12 @@ 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);
+	int mode = lo->clear_limits_mode;
 
 	if (mode & FALLOC_FL_ZERO_RANGE)
 		lim.max_write_zeroes_sectors = 0;

@@ -234,14 +239,13 @@ static void loop_clear_limits(struct loop_device *lo, int mode)
 		lim.discard_granularity = 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.
-	 */
-	queue_limits_commit_update(lo->lo_queue, &lim);
+	queue_limits_commit_update_frozen(lo->lo_queue, &lim);
+}
+
+static void loop_clear_limits(struct loop_device *lo, int mode)
+{
+	lo->clear_limits_mode |= mode;
+	schedule_work(&lo->clear_limits_work);
 }
 
 static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,

@@ -1781,6 +1785,7 @@ 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);
 	kfree(lo);
 }

@@ -2100,6 +2105,7 @@ 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);
 	INIT_LIST_HEAD(&lo->rootcg_cmd_list);
 	disk->major		= LOOP_MAJOR;
 	disk->first_minor	= i << part_shift;

Signature · 1 line

2.43.0

Bart Van Assche

2026-08-28 16:34 UTC #1 lore raw

Quoted 9 lines · Tao Cui

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.

Quoted 10 lines

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 and update the limits using queue_limits_commit_update_frozen(). Accumulate pending modes in lo->clear_limits_mode so that failures between scheduling and execution of the work item are not lost, and cancel the work item before the device is freed. If the device is reconfigured to a backing file that does support the operation in that window, the stale clear takes effect and discard is disabled until the next reconfiguration.

Please help with reviewing this patch, which seems more complete to me than this patch:
https://lore.kernel.org/linux-block/[email protected]/

Thanks,

Bart.

Tao Cui

author

2026-08-31 13:34 UTC #2 lore raw

Hi Bart,

Quoted 20 lines · Bart Van Assche

Quoted 9 lines · Tao Cui
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.
Quoted 10 lines
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 and update the limits using queue_limits_commit_update_frozen(). Accumulate pending modes in lo->clear_limits_mode so that failures between scheduling and execution of the work item are not lost, and cancel the work item before the device is freed. If the device is reconfigured to a backing file that does support the operation in that window, the stale clear takes effect and discard is disabled until the next reconfiguration.

Quoted 4 lines

Please help with reviewing this patch, which seems more complete to me than this patch:
https://lore.kernel.org/linux-block/[email protected]/

Thanks for the pointer. The two patches fix different races though, so they are not alternatives to each other.

Tetsuo's v7 fixes the teardown path: __loop_clr_fd() racing with in-flight requests, which is the syzbot NULL deref in lo_rw_aio(). My patch fixes a runtime race: loop_clear_limits() still updates q->limits from the loop workqueue without freezing the queue when a discard or write-zeroes request fails with -EOPNOTSUPP, so lockless readers of q->limits can observe torn values. The freeze in Tetsuo's patch only happens at teardown and does not cover that path - the XXX comment and the unfrozen queue_limits_commit_update() in loop_clear_limits() are still there in his tree. The two patches can coexist.

I'll go review Tetsuo's v7 and reply to his thread.

Thanks,
Tao

Quoted 3 lines

Thanks,
Bart.

Bart Van Assche

2026-08-31 16:25 UTC #3 lore raw

In reply to Tao Cui

Quoted 4 lines · Tao Cui

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 and update the limits using queue_limits_commit_update_frozen().

The patch description explains why queue freezing is necessary but does not add queue freeze and unfreeze calls. Is that perhaps an oversight?

Thanks,

Bart.

Tao Cui

author

2026-09-01 13:14 UTC #4 lore raw

Hi Bart,

Quoted 9 lines · Bart Van Assche

Quoted 4 lines · Tao Cui
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 and update the limits using queue_limits_commit_update_frozen().
The patch description explains why queue freezing is necessary but does not add queue freeze and unfreeze calls. Is that perhaps an oversight?

Good question, I should have spelled this out in the commit message. The freeze and unfreeze are inside queue_limits_commit_update_frozen() itself (block/blk-settings.c):

	memflags = blk_mq_freeze_queue(q);
	ret = queue_limits_commit_update(q, lim);
	blk_mq_unfreeze_queue(q, memflags);

The naming is a bit counterintuitive: the _frozen variant is the one that freezes the queue itself, while the plain queue_limits_commit_update() expects the caller to have frozen the queue already ("The caller must have frozen the queue or ensure that there are no outstanding I/Os by other means"). The in-tree callers use it that way too - blk_integrity_unregister() and the sd.c revalidation paths call the _frozen variant without freezing the queue themselves.

The old loop_clear_limits() was exactly a caller that did not meet that expectation, since it ran in the loop workqueue where the queue cannot be frozen, so moving to the work item and using the _frozen wrapper is the fix.

Thanks,
Tao

Quoted 3 lines

Thanks,
Bart.

Bart Van Assche

Reviewed-by on v1 1/1

2026-09-01 17:29 UTC #5 lore raw

In reply to Tao Cui

Quoted 9 lines · Tao Cui

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.

Quoted 10 lines

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 and update the limits using queue_limits_commit_update_frozen(). Accumulate pending modes in lo->clear_limits_mode so that failures between scheduling and execution of the work item are not lost, and cancel the work item before the device is freed. If the device is reconfigured to a backing file that does support the operation in that window, the stale clear takes effect and discard is disabled until the next reconfiguration.

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