[PATCH v7] loop: Fix NULL pointer dereference in lo_rw_aio()
From: Tetsuo Handa
Date: Fri Aug 28 2026 - 11:57:59 EST
syzbot is reporting NULL pointer dereference in lo_rw_aio() [1][2].
An analysis by the Gemini AI collaborator [3] considers that this problem
is caused by a timing shift primarily exposed by commit 65565ca5f99b
("block: unify the synchronous bi_end_io callbacks"), along with helper
refactorings like commit 92c3737a2473 ("block: add a bio_submit_or_kill
helper").
But due to difficulty of reproducing this race, discussion about what is
happening and how to fix this problem is stalling. Also, we haven't
identified how many filesystems are subjected to this problem.
Therefore, introduce a grace period for flushing pending I/O requests
(which should be a good thing from the perspective of defensive
programming) so that we won't hit NULL pointer dereference problem.
However, calling drain_workqueue() from __loop_clr_fd() with
disk->open_mutex held causes lockdep warnings. We need to flush pending
I/O requests without disk->open_mutex held. Therefore, defer
__loop_clr_fd() to WQ context, like commit 322c4293ecc5 ("loop: make
autoclear operation asynchronous") did.
The past attempt was reverted by commit bf23747ee053 ("loop: revert "make
autoclear operation asynchronous"") for two reasons:
(1) Userspace might be expecting that fput() on the backing file is
processed before lo_release() from close() returns to user mode.
But a debug patch [4] suggested me that this teardown operation is
racy regardless of whether disk->open_mutex is temporarily released
or not, and therefore the xfs/259 breakage should be addressed on
the xfstests side.
(2) Lockdep reported circular locking dependency caused by flushing
system-wide WQs. But we no longer need to worry that dependency
because all in-tree users no longer flush system-wide WQs.
Therefore, let's retry deferring __loop_clr_fd() to WQ context again.
Link: https://syzkaller.appspot.com/bug?extid=cd8a9a308e879a4e2c28 [1]
Link: https://syzkaller.appspot.com/bug?extid=bc273027d5643e48e5b3 [2]
Link: https://lkml.kernel.org/r/fbb3edda-f108-4e5b-acf2-266f043f8125@xxxxxxxxxxxxxxxxxxx [3]
Link: https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@xxxxxxxxxxxxxxxxxxx [4]
Fixes: 65565ca5f99b ("block: unify the synchronous bi_end_io callbacks")
Assisted-by: Gemini-Pro
Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
---
Sashiko reviewed this patch, and did not find problems
( https://sashiko.dev/#/patchset/5803da44-97c7-440e-a06b-d3cf4afff3c8%40I-love.SAKURA.ne.jp ).
Can we try this approach?
drivers/block/loop.c | 74 +++++++++++++++++++++++++++++++++++---------
1 file changed, 59 insertions(+), 15 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6f12976035b0..e2703f0ee75d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -75,6 +75,7 @@ struct loop_device {
struct gendisk *lo_disk;
struct mutex lo_mutex;
bool idr_visible;
+ struct work_struct lo_clr_work;
};
struct loop_cmd {
@@ -1134,13 +1135,35 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
return error;
}
-static void __loop_clr_fd(struct loop_device *lo)
+static void __loop_clr_fd(struct work_struct *work)
{
+ struct loop_device *lo = container_of(work, struct loop_device, lo_clr_work);
+ struct gendisk *disk = lo->lo_disk;
struct queue_limits lim;
struct file *filp;
gfp_t gfp = lo->old_gfp_mask;
int err;
+ /* Step 1: Flush all outstanding I/O, without open_mutex held. */
+ /*
+ * Now that loop_queue_rq() sees lo->lo_state != Lo_bound,
+ * wait for already started loop_queue_rq() to complete.
+ */
+ synchronize_rcu();
+ /*
+ * Now that no more works are scheduled by loop_queue_rq(),
+ * wait for already scheduled works to complete.
+ */
+ drain_workqueue(lo->workqueue);
+ /*
+ * Now that no more AIO requests are scheduled by lo_rw_aio(),
+ * wait for already started AIO to complete.
+ */
+ blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
+
+ /* Step 2: Perform remaining cleanup, with open_mutex held. */
+ mutex_lock(&disk->open_mutex);
+
spin_lock_irq(&lo->lo_lock);
filp = lo->lo_backing_file;
lo->lo_backing_file = NULL;
@@ -1151,12 +1174,7 @@ static void __loop_clr_fd(struct loop_device *lo)
lo->lo_sizelimit = 0;
memset(lo->lo_file_name, 0, LO_NAME_SIZE);
- /*
- * Reset the block size to the default.
- *
- * No queue freezing needed because this is called from the final
- * ->release call only, so there can't be any outstanding I/O.
- */
+ /* Reset the block size to the default. */
lim = queue_limits_start_update(lo->lo_queue);
lim.logical_block_size = SECTOR_SIZE;
lim.physical_block_size = SECTOR_SIZE;
@@ -1168,8 +1186,6 @@ static void __loop_clr_fd(struct loop_device *lo)
/* let user-space know about this change */
kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
mapping_set_gfp_mask(filp->f_mapping, gfp);
- /* This is safe: open() is still holding a reference. */
- module_put(THIS_MODULE);
disk_force_media_change(lo->lo_disk);
@@ -1199,12 +1215,24 @@ static void __loop_clr_fd(struct loop_device *lo)
WRITE_ONCE(lo->lo_state, Lo_unbound);
mutex_unlock(&lo->lo_mutex);
+ /* Step 3: Drop refcounts, without open_mutex held. */
+ mutex_unlock(&disk->open_mutex);
+
+ fput(filp);
+
/*
- * Need not hold lo_mutex to fput backing file. Calling fput holding
- * lo_mutex triggers a circular lock dependency possibility warning as
- * fput can take open_mutex which is usually taken before lo_mutex.
+ * Drop all references that would have been dropped as soon as
+ * returning from lo_release() and releasing disk->open_mutex.
*/
- fput(filp);
+ module_put(disk->fops->owner);
+ put_device(disk_to_dev(disk));
+
+ /*
+ * This is safe: flush_work() from loop_remove() from loop_exit() waits
+ * until this function returns; effectively dropping the final module
+ * references synchronously.
+ */
+ module_put(THIS_MODULE);
}
static int loop_clr_fd(struct loop_device *lo)
@@ -1769,8 +1797,20 @@ static void lo_release(struct gendisk *disk)
need_clear = (lo->lo_state == Lo_rundown);
mutex_unlock(&lo->lo_mutex);
- if (need_clear)
- __loop_clr_fd(lo);
+ /*
+ * In order to flush pending I/O requests before clearing the backing
+ * device, defer __loop_clr_fd() to WQ context. The Lo_rundown state
+ * guarantees that lo_open() will fail with -ENXIO.
+ */
+ if (need_clear) {
+ /*
+ * Grab all references that will be dropped as soon as
+ * returning from lo_release() and releasing disk->open_mutex.
+ */
+ get_device(disk_to_dev(disk));
+ __module_get(disk->fops->owner);
+ queue_work(system_long_wq, &lo->lo_clr_work);
+ }
}
static void lo_free_disk(struct gendisk *disk)
@@ -2034,6 +2074,7 @@ static int loop_add(int i)
lo = kzalloc_obj(*lo);
if (!lo)
goto out;
+ INIT_WORK(&lo->lo_clr_work, __loop_clr_fd);
lo->worker_tree = RB_ROOT;
INIT_LIST_HEAD(&lo->idle_worker_list);
timer_setup(&lo->timer, loop_free_idle_workers_timer, TIMER_DEFERRABLE);
@@ -2138,6 +2179,9 @@ static int loop_add(int i)
static void loop_remove(struct loop_device *lo)
{
+ /* Wait for __loop_clr_fd() to complete. */
+ flush_work(&lo->lo_clr_work);
+
/* Make this loop device unreachable from pathname. */
del_gendisk(lo->lo_disk);
blk_mq_free_tag_set(&lo->tag_set);
--
2.55.0