[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 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 <bvanassche@acm.org>
Signed-off-by: Tao Cui <cuitao@kylinos.cn>

---
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/20260828072004.273519-1-cui.tao@linux.dev/
---
 drivers/block/loop.c | 83 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 69 insertions(+), 14 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 758c20678bf6c..0b89036c982ad 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -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);
-- 
2.43.0

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

[Tao Cui]

在 2026/9/24 18:20, 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.
> 

Oops, I messed up the subject, that was supposed to be [PATCH v5].
The patch in it is the final version: rebased as a single commit,
plus one fix over v4, clear_limits_lock now covers the limits
commit too, since the queue freeze is counted and doesn't exclude
a concurrent loop_change_fd(). Please ignore the subject and treat
it as v5.

Sorry for the noise,
Tao.

> 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 <bvanassche@acm.org>
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> 
> ---
> 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/20260828072004.273519-1-cui.tao@linux.dev/
> ---
>  drivers/block/loop.c | 83 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 69 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 758c20678bf6c..0b89036c982ad 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -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);

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>

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:
> 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/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:
> 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.

[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