[PATCH] loop: defer the queue limits clear to a workqueue

[PATCH] loop: defer the queue limits clear to a workqueue

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

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 <cuitao@kylinos.cn>
---
 drivers/block/loop.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6f12976035b0..5a1e8b6794ed 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -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;
-- 
2.43.0

Re: [PATCH] loop: defer the queue limits clear to a workqueue

[Bart Van Assche]

On 8/28/26 12:20 AM, Tao Cui wrote:
> 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.
Please help with reviewing this patch, which seems more complete to me
than this patch:
https://lore.kernel.org/linux-block/c2ab2547-63b3-48cf-87c1-fc53219e360a@I-love.SAKURA.ne.jp/

Thanks,

Bart.

Re: [PATCH] loop: defer the queue limits clear to a workqueue

[Tao Cui]

Hi Bart,

在 2026/8/29 00:34, Bart Van Assche 写道:
> On 8/28/26 12:20 AM, Tao Cui wrote:
>> 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.
> Please help with reviewing this patch, which seems more complete to me
> than this patch:
> https://lore.kernel.org/linux-block/c2ab2547-63b3-48cf-87c1-fc53219e360a@I-love.SAKURA.ne.jp/
> 

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

> Thanks,
> 
> Bart.

Re: [PATCH] loop: defer the queue limits clear to a workqueue

[Bart Van Assche]

On 8/28/26 12:20 AM, Tao Cui wrote:
> 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.

Re: [PATCH] loop: defer the queue limits clear to a workqueue

[Tao Cui]

Hi Bart,

在 2026/9/1 00:25, Bart Van Assche 写道:
> On 8/28/26 12:20 AM, Tao Cui wrote:
>> 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

> Thanks,
> 
> Bart.

Re: [PATCH] loop: defer the queue limits clear to a workqueue

[Bart Van Assche]

On 8/28/26 12:20 AM, Tao Cui wrote:
> 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.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>