* [PATCH 01/13] loop: Fix the code for recursion detection
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 02/13] loop: Reorder checks in loop_validate_file() Bart Van Assche
` (11 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche,
Theodore Ts'o, stable
The purpose of loop_validate_file() is to prevent that the file descriptor
of an already bound loop device is changed into a file descriptor
associated with a file that depends on the loop device. This must be
prevented because otherwise infinite I/O loops could be triggered and
filesystems involved in this loop would become impossible to unmount.
Fix loop_validate_file() by comparing gendisk pointers instead of dev_t
values.
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
Fixes: d2ac838e4cd7 ("loop: add recursion validation to LOOP_CHANGE_FD")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6f12976035b0..36deb08ee463 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -481,13 +481,26 @@ static void loop_update_dio_alignment(struct loop_device *lo)
lo->lo_dio_mem_align = SECTOR_SIZE - 1;
}
-static inline int is_loop_device(struct file *file)
+/* Returns the block device that underpins a file. */
+static inline struct block_device *loop_get_bdev(struct file *file)
{
- struct inode *i = file->f_mapping->host;
+ struct inode *inode = file->f_mapping->host;
+
+ if (S_ISBLK(inode->i_mode))
+ return I_BDEV(inode);
+ if (S_ISREG(inode->i_mode) && inode->i_sb)
+ return inode->i_sb->s_bdev;
+ return NULL;
+}
+
+static inline bool is_loop_device(struct file *file)
+{
+ struct block_device *bdev = loop_get_bdev(file);
- return i && S_ISBLK(i->i_mode) && imajor(i) == LOOP_MAJOR;
+ return bdev && bdev->bd_disk->major == LOOP_MAJOR;
}
+/* Returns 0 if and only if @file is not backed by loop device @bdev. */
static int loop_validate_file(struct file *file, struct block_device *bdev)
{
struct inode *inode = file->f_mapping->host;
@@ -496,12 +509,13 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
/* Avoid recursion */
while (is_loop_device(f)) {
struct loop_device *l;
+ struct block_device *f_bdev = loop_get_bdev(f);
lockdep_assert_held(&loop_validate_mutex);
- if (f->f_mapping->host->i_rdev == bdev->bd_dev)
+ if (f_bdev->bd_disk == bdev->bd_disk)
return -EBADF;
- l = I_BDEV(f->f_mapping->host)->bd_disk->private_data;
+ l = f_bdev->bd_disk->private_data;
if (l->lo_state != Lo_bound)
return -EINVAL;
/* Order wrt setting lo->lo_backing_file in loop_configure(). */
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 02/13] loop: Reorder checks in loop_validate_file()
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
2026-08-20 19:57 ` [PATCH 01/13] loop: Fix the code for recursion detection Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 03/13] loop: Enable context analysis Bart Van Assche
` (10 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Checking inode attributes after a loop that modifies the file pointer
the inode has been derived from confuses some static analyzers. Hence
swap the loop and the inode attribute check.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 36deb08ee463..4315c36d4042 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -506,6 +506,9 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
struct inode *inode = file->f_mapping->host;
struct file *f = file;
+ if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
+ return -EINVAL;
+
/* Avoid recursion */
while (is_loop_device(f)) {
struct loop_device *l;
@@ -522,8 +525,6 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
rmb();
f = l->lo_backing_file;
}
- if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
- return -EINVAL;
return 0;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 03/13] loop: Enable context analysis
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
2026-08-20 19:57 ` [PATCH 01/13] loop: Fix the code for recursion detection Bart Van Assche
2026-08-20 19:57 ` [PATCH 02/13] loop: Reorder checks in loop_validate_file() Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 04/13] loop: Assign a unique lockdep key to each lo_mutex instance Bart Van Assche
` (9 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Let the compiler verify __must_hold(), __guarded_by() etc. Suppress the
warnings reported for loop_global_{,un}_lock*() by adding a
__context_unsafe() annotation. Both __context_unsafe() annotations will be
removed by a later patch.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/Makefile | 2 ++
drivers/block/loop.c | 4 ++++
2 files changed, 6 insertions(+)
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index 2d8096eb8cdf..d4555949cc3c 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -6,6 +6,8 @@
# Rewritten to use lists instead of if-statements.
#
+CONTEXT_ANALYSIS_loop.o := y
+
# needed for trace events
ccflags-y += -I$(src)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 4315c36d4042..638d97293a79 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -108,6 +108,8 @@ static DEFINE_MUTEX(loop_validate_mutex);
* loop_configure()/loop_change_fd()/__loop_clr_fd() calls.
*/
static int loop_global_lock_killable(struct loop_device *lo, bool global)
+ __cond_acquires(0, &lo->lo_mutex)
+ __context_unsafe(conditional locking)
{
int err;
@@ -129,6 +131,8 @@ static int loop_global_lock_killable(struct loop_device *lo, bool global)
* @global: true if @lo was about to bind another "struct loop_device", false otherwise
*/
static void loop_global_unlock(struct loop_device *lo, bool global)
+ __releases(&lo->lo_mutex)
+ __context_unsafe(conditional locking)
{
mutex_unlock(&lo->lo_mutex);
if (global)
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 04/13] loop: Assign a unique lockdep key to each lo_mutex instance
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (2 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 03/13] loop: Enable context analysis Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 05/13] loop: Add more __must_hold() annotations Bart Van Assche
` (8 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Prepare for nesting lo_mutex in loop_validate_file().
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 638d97293a79..5c7f72e01345 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -20,6 +20,7 @@
#include <linux/suspend.h>
#include <linux/freezer.h>
#include <linux/mutex.h>
+#include <linux/lockdep.h>
#include <linux/writeback.h>
#include <linux/completion.h>
#include <linux/highmem.h>
@@ -74,6 +75,7 @@ struct loop_device {
struct blk_mq_tag_set tag_set;
struct gendisk *lo_disk;
struct mutex lo_mutex;
+ struct lock_class_key lo_mutex_key;
bool idr_visible;
};
@@ -1801,6 +1803,7 @@ static void lo_free_disk(struct gendisk *disk)
loop_free_idle_workers(lo, true);
timer_shutdown_sync(&lo->timer);
mutex_destroy(&lo->lo_mutex);
+ lockdep_unregister_key(&lo->lo_mutex_key);
kfree(lo);
}
@@ -2114,7 +2117,8 @@ static int loop_add(int i)
*/
if (!part_shift)
set_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
- mutex_init(&lo->lo_mutex);
+ lockdep_register_key(&lo->lo_mutex_key);
+ mutex_init_with_key(&lo->lo_mutex, &lo->lo_mutex_key);
lo->lo_number = i;
spin_lock_init(&lo->lo_lock);
spin_lock_init(&lo->lo_work_lock);
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 05/13] loop: Add more __must_hold() annotations
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (3 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 04/13] loop: Assign a unique lockdep key to each lo_mutex instance Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 06/13] loop: Protect all lo_backing_file accesses with lo->lo_mutex Bart Van Assche
` (7 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Annotate all functions that are called with lo_mutex held with
__must_hold(&lo->lo_mutex). Add a 'lo' argument to loop_validate_file()
such that a __must_hold(&lo->lo_mutex) annotation can be added to this
function too.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5c7f72e01345..aa40a6ed7f35 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -145,6 +145,7 @@ static int max_part;
static int part_shift;
static loff_t lo_calculate_size(struct loop_device *lo, struct file *file)
+ __must_hold(&lo->lo_mutex)
{
loff_t loopsize;
int ret;
@@ -186,6 +187,7 @@ static loff_t lo_calculate_size(struct loop_device *lo, struct file *file)
* the backing device.
*/
static bool lo_can_use_dio(struct loop_device *lo)
+ __must_hold(&lo->lo_mutex)
{
if (!(lo->lo_backing_file->f_mode & FMODE_CAN_ODIRECT))
return false;
@@ -205,6 +207,7 @@ static bool lo_can_use_dio(struct loop_device *lo)
* not the originally passed in one.
*/
static inline void loop_update_dio(struct loop_device *lo)
+ __must_hold(&lo->lo_mutex)
{
lockdep_assert_held(&lo->lo_mutex);
WARN_ON_ONCE(lo->lo_state == Lo_bound &&
@@ -223,6 +226,7 @@ static inline void loop_update_dio(struct loop_device *lo)
* a sector_t, eg using loop_validate_size()
*/
static void loop_set_size(struct loop_device *lo, loff_t size)
+ __must_hold(&lo->lo_mutex)
{
if (!set_capacity_and_notify(lo->lo_disk, size))
kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
@@ -455,6 +459,7 @@ static void loop_reread_partitions(struct loop_device *lo)
}
static void loop_update_dio_alignment(struct loop_device *lo)
+ __must_hold(&lo->lo_mutex)
{
struct file *file = lo->lo_backing_file;
struct block_device *sb_bdev = file->f_mapping->host->i_sb->s_bdev;
@@ -507,7 +512,9 @@ static inline bool is_loop_device(struct file *file)
}
/* Returns 0 if and only if @file is not backed by loop device @bdev. */
-static int loop_validate_file(struct file *file, struct block_device *bdev)
+static int loop_validate_file(struct loop_device *lo, struct file *file,
+ struct block_device *bdev)
+ __must_hold(&lo->lo_mutex)
{
struct inode *inode = file->f_mapping->host;
struct file *f = file;
@@ -535,6 +542,7 @@ 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)
+ __must_hold(&lo->lo_mutex)
{
lo->lo_backing_file = file;
lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
@@ -599,7 +607,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
goto out_err;
- error = loop_validate_file(file, bdev);
+ error = loop_validate_file(lo, file, bdev);
if (error)
goto out_err;
@@ -770,6 +778,7 @@ static void loop_sysfs_exit(struct loop_device *lo)
static void loop_get_discard_config(struct loop_device *lo,
u32 *granularity, u32 *max_discard_sectors)
+ __must_hold(&lo->lo_mutex)
{
struct file *file = lo->lo_backing_file;
struct inode *inode = file->f_mapping->host;
@@ -936,6 +945,7 @@ static void loop_free_idle_workers_timer(struct timer_list *timer)
static int
loop_set_status_from_info(struct loop_device *lo,
const struct loop_info64 *info)
+ __must_hold(&lo->lo_mutex)
{
if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE)
return -EINVAL;
@@ -974,6 +984,7 @@ static unsigned int loop_default_blocksize(struct loop_device *lo)
}
static void loop_set_dma_limit(struct loop_device *lo, struct queue_limits *lim)
+ __must_hold(&lo->lo_mutex)
{
/*
* Direct I/O forwards the user pages to the backing file unchanged, so
@@ -988,6 +999,7 @@ static void loop_set_dma_limit(struct loop_device *lo, struct queue_limits *lim)
static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim,
unsigned int bsize)
+ __must_hold(&lo->lo_mutex)
{
struct file *file = lo->lo_backing_file;
struct inode *inode = file->f_mapping->host;
@@ -1064,7 +1076,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
if (lo->lo_state != Lo_unbound)
goto out_unlock;
- error = loop_validate_file(file, bdev);
+ error = loop_validate_file(lo, file, bdev);
if (error)
goto out_unlock;
@@ -1443,6 +1455,7 @@ loop_get_status64(struct loop_device *lo, struct loop_info64 __user *arg) {
}
static int loop_set_capacity(struct loop_device *lo)
+ __must_hold(&lo->lo_mutex)
{
loff_t size;
@@ -1456,6 +1469,7 @@ static int loop_set_capacity(struct loop_device *lo)
}
static int loop_set_dio(struct loop_device *lo, unsigned long arg)
+ __must_hold(&lo->lo_mutex)
{
bool use_dio = !!arg;
unsigned int memflags;
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 06/13] loop: Protect all lo_backing_file accesses with lo->lo_mutex
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (4 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 05/13] loop: Add more __must_hold() annotations Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 07/13] loop: Fix race conditions in loop_validate_file() Bart Van Assche
` (6 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Protect all lo_backing_file accesses from the control path with
lo->lo_mutex. Use READ_ONCE() to read lo_backing_file from the data path.
Serialization of I/O path lo_backing_file reads and control path
lo_backing_file changes happens by freezing the request queue.
Remove lo_lock because it is no longer used.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 45 ++++++++++++++++++++++++++------------------
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index aa40a6ed7f35..c5f026520836 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -53,14 +53,13 @@ struct loop_device {
int lo_flags;
char lo_file_name[LO_NAME_SIZE];
- struct file *lo_backing_file;
+ struct file *lo_backing_file __guarded_by(&lo_mutex);
unsigned int lo_min_dio_size;
unsigned int lo_dio_mem_align;
struct block_device *lo_device;
gfp_t old_gfp_mask;
- spinlock_t lo_lock;
int lo_state;
spinlock_t lo_work_lock;
struct workqueue_struct *workqueue;
@@ -261,7 +260,7 @@ static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,
* We use fallocate to manipulate the space mappings used by the image
* a.k.a. discard/zerorange.
*/
- struct file *file = lo->lo_backing_file;
+ struct file *file = context_unsafe(READ_ONCE(lo->lo_backing_file));
int ret;
mode |= FALLOC_FL_KEEP_SIZE;
@@ -285,7 +284,7 @@ static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,
static int lo_req_flush(struct loop_device *lo, struct request *rq)
{
- int ret = vfs_fsync(lo->lo_backing_file, 0);
+ int ret = vfs_fsync(context_unsafe(READ_ONCE(lo->lo_backing_file)), 0);
if (unlikely(ret && ret != -EINVAL))
ret = -EIO;
@@ -354,7 +353,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
struct iov_iter iter;
struct req_iterator rq_iter;
struct request *rq = blk_mq_rq_from_pdu(cmd);
- struct file *file = lo->lo_backing_file;
+ struct file *file = context_unsafe(READ_ONCE(lo->lo_backing_file));
unsigned int nr_bvec;
int ret;
@@ -511,6 +510,19 @@ static inline bool is_loop_device(struct file *file)
return bdev && bdev->bd_disk->major == LOOP_MAJOR;
}
+static struct file *loop_get_backing_file(struct loop_device *lo)
+ __must_hold(&lo->lo_mutex)
+{
+ if (lo->lo_state != Lo_bound)
+ return NULL;
+ /*
+ * Order wrt setting lo->lo_backing_file in
+ * loop_configure().
+ */
+ rmb();
+ return lo->lo_backing_file;
+}
+
/* Returns 0 if and only if @file is not backed by loop device @bdev. */
static int loop_validate_file(struct loop_device *lo, struct file *file,
struct block_device *bdev)
@@ -532,11 +544,10 @@ static int loop_validate_file(struct loop_device *lo, struct file *file,
return -EBADF;
l = f_bdev->bd_disk->private_data;
- if (l->lo_state != Lo_bound)
+ scoped_guard(mutex, &l->lo_mutex)
+ f = loop_get_backing_file(l);
+ if (!f)
return -EINVAL;
- /* Order wrt setting lo->lo_backing_file in loop_configure(). */
- rmb();
- f = l->lo_backing_file;
}
return 0;
}
@@ -693,10 +704,9 @@ static ssize_t loop_attr_backing_file_show(struct loop_device *lo, char *buf)
ssize_t ret;
char *p = NULL;
- spin_lock_irq(&lo->lo_lock);
- if (lo->lo_backing_file)
- p = file_path(lo->lo_backing_file, buf, PAGE_SIZE - 1);
- spin_unlock_irq(&lo->lo_lock);
+ scoped_guard(mutex, &lo->lo_mutex)
+ if (lo->lo_backing_file)
+ p = file_path(lo->lo_backing_file, buf, PAGE_SIZE - 1);
if (IS_ERR_OR_NULL(p))
ret = PTR_ERR(p);
@@ -1174,10 +1184,10 @@ static void __loop_clr_fd(struct loop_device *lo)
gfp_t gfp = lo->old_gfp_mask;
int err;
- spin_lock_irq(&lo->lo_lock);
- filp = lo->lo_backing_file;
- lo->lo_backing_file = NULL;
- spin_unlock_irq(&lo->lo_lock);
+ scoped_guard(mutex, &lo->lo_mutex) {
+ filp = lo->lo_backing_file;
+ lo->lo_backing_file = NULL;
+ }
lo->lo_device = NULL;
lo->lo_offset = 0;
@@ -2134,7 +2144,6 @@ static int loop_add(int i)
lockdep_register_key(&lo->lo_mutex_key);
mutex_init_with_key(&lo->lo_mutex, &lo->lo_mutex_key);
lo->lo_number = i;
- spin_lock_init(&lo->lo_lock);
spin_lock_init(&lo->lo_work_lock);
INIT_WORK(&lo->rootcg_work, loop_rootcg_workfn);
INIT_LIST_HEAD(&lo->rootcg_cmd_list);
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (5 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 06/13] loop: Protect all lo_backing_file accesses with lo->lo_mutex Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-24 6:12 ` Nilay Shroff
2026-08-20 19:57 ` [PATCH 08/13] loop: Remove memory barriers Bart Van Assche
` (5 subsequent siblings)
12 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Fix race conditions in loop_validate_file() by adding reference counting
to the file chain traversal.
Ensure the file reference is kept alive during all dereferences by
calling get_file() before the loop and deferring fput() until after we
have locked the target device's lo_mutex and confirmed it is in the
Lo_bound state.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c5f026520836..8b633ea6e72f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -520,7 +520,7 @@ static struct file *loop_get_backing_file(struct loop_device *lo)
* loop_configure().
*/
rmb();
- return lo->lo_backing_file;
+ return get_file(lo->lo_backing_file);
}
/* Returns 0 if and only if @file is not backed by loop device @bdev. */
@@ -534,21 +534,27 @@ static int loop_validate_file(struct loop_device *lo, struct file *file,
if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
return -EINVAL;
+ get_file(f);
/* Avoid recursion */
while (is_loop_device(f)) {
struct loop_device *l;
+ struct file *prev_f = f;
struct block_device *f_bdev = loop_get_bdev(f);
lockdep_assert_held(&loop_validate_mutex);
- if (f_bdev->bd_disk == bdev->bd_disk)
+ if (f_bdev->bd_disk == bdev->bd_disk) {
+ fput(f);
return -EBADF;
+ }
l = f_bdev->bd_disk->private_data;
scoped_guard(mutex, &l->lo_mutex)
f = loop_get_backing_file(l);
+ fput(prev_f);
if (!f)
return -EINVAL;
}
+ fput(f);
return 0;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
2026-08-20 19:57 ` [PATCH 07/13] loop: Fix race conditions in loop_validate_file() Bart Van Assche
@ 2026-08-24 6:12 ` Nilay Shroff
2026-08-24 16:15 ` Bart Van Assche
0 siblings, 1 reply; 18+ messages in thread
From: Nilay Shroff @ 2026-08-24 6:12 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe; +Cc: linux-block, Christoph Hellwig
On 8/21/26 1:27 AM, Bart Van Assche wrote:
> Fix race conditions in loop_validate_file() by adding reference counting
> to the file chain traversal.
>
> Ensure the file reference is kept alive during all dereferences by
> calling get_file() before the loop and deferring fput() until after we
> have locked the target device's lo_mutex and confirmed it is in the
> Lo_bound state.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/block/loop.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c5f026520836..8b633ea6e72f 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -520,7 +520,7 @@ static struct file *loop_get_backing_file(struct loop_device *lo)
> * loop_configure().
> */
> rmb();
> - return lo->lo_backing_file;
> + return get_file(lo->lo_backing_file);
> }
>
> /* Returns 0 if and only if @file is not backed by loop device @bdev. */
> @@ -534,21 +534,27 @@ static int loop_validate_file(struct loop_device *lo, struct file *file,
> if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
> return -EINVAL;
>
> + get_file(f);
> /* Avoid recursion */
> while (is_loop_device(f)) {
> struct loop_device *l;
> + struct file *prev_f = f;
> struct block_device *f_bdev = loop_get_bdev(f);
>
> lockdep_assert_held(&loop_validate_mutex);
> - if (f_bdev->bd_disk == bdev->bd_disk)
> + if (f_bdev->bd_disk == bdev->bd_disk) {
> + fput(f);
> return -EBADF;
> + }
>
> l = f_bdev->bd_disk->private_data;
> scoped_guard(mutex, &l->lo_mutex)
> f = loop_get_backing_file(l);
> + fput(prev_f);
> if (!f)
> return -EINVAL;
> }
> + fput(f);
> return 0;
> }
>
I see that with the refactoring and the changes in this patch, where we now
take an explicit reference to each backing file while traversing the loop-device
chain and hold the corresponding lo_mutex while checking lo_state and acquiring
that reference, loop_validate_mutex may no longer be necessary.
In particular, loop_get_backing_file() now atomically checks that the loop device
is in Lo_bound state and takes a reference to lo_backing_file while holding
lo_mutex. Therefore, if loop_clr_fd() or loop_change_fd() concurrently replaces
or clears the backing file, the validator still holds its own reference. It also
appears that loop_change_fd() and loop_clr_fd() for the same loop device are
already serialized by lo_mutex.
So I am wondering whether loop_validate_mutex now be redundant? If so, it may
be worth consider removing it as part of this series. That would simplify the
locking and make the context annotations considerably cleaner as well.
Thanks,
--Nilay
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
2026-08-24 6:12 ` Nilay Shroff
@ 2026-08-24 16:15 ` Bart Van Assche
2026-08-24 18:06 ` Nilay Shroff
0 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2026-08-24 16:15 UTC (permalink / raw)
To: Nilay Shroff, Jens Axboe; +Cc: linux-block, Christoph Hellwig
On 8/23/26 11:12 PM, Nilay Shroff wrote:
> I see that with the refactoring and the changes in this patch, where
> we now take an explicit reference to each backing file while
> traversing the loop-device chain and hold the corresponding lo_mutex
> while checking lo_state and acquiring that reference,
> loop_validate_mutex may no longer be necessary.
>
> In particular, loop_get_backing_file() now atomically checks that
> the loop device is in Lo_bound state and takes a reference to
> lo_backing_file while holding lo_mutex. Therefore, if loop_clr_fd()
> or loop_change_fd() concurrently replaces or clears the backing
> file, the validator still holds its own reference. It also appears
> that loop_change_fd() and loop_clr_fd() for the same loop device are
> already serialized by lo_mutex.
>
> So I am wondering whether loop_validate_mutex now be redundant? If
> so, it may be worth consider removing it as part of this series.
> That would simplify the locking and make the context annotations
> considerably cleaner as well.
Hi Nilay,
That's an interesting question. I think we still need
loop_validate_mutex. Without that mutex the hierarchy could be
changed from linear into recursive after loop_validate_file()
has verified the hierarchy and before the loop fd is changed.
Removing loop_validate_mutex might introduce other race conditions
than the one mentioned above.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
2026-08-24 16:15 ` Bart Van Assche
@ 2026-08-24 18:06 ` Nilay Shroff
2026-08-24 20:23 ` Bart Van Assche
0 siblings, 1 reply; 18+ messages in thread
From: Nilay Shroff @ 2026-08-24 18:06 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe; +Cc: linux-block, Christoph Hellwig
On 8/24/26 9:45 PM, Bart Van Assche wrote:
> On 8/23/26 11:12 PM, Nilay Shroff wrote:
>> I see that with the refactoring and the changes in this patch, where
>> we now take an explicit reference to each backing file while
>> traversing the loop-device chain and hold the corresponding lo_mutex
>> while checking lo_state and acquiring that reference,
>> loop_validate_mutex may no longer be necessary.
>>
>> In particular, loop_get_backing_file() now atomically checks that
>> the loop device is in Lo_bound state and takes a reference to
>> lo_backing_file while holding lo_mutex. Therefore, if loop_clr_fd()
>> or loop_change_fd() concurrently replaces or clears the backing
>> file, the validator still holds its own reference. It also appears
>> that loop_change_fd() and loop_clr_fd() for the same loop device are already serialized by lo_mutex.
>>
>> So I am wondering whether loop_validate_mutex now be redundant? If
>> so, it may be worth consider removing it as part of this series.
>> That would simplify the locking and make the context annotations
>> considerably cleaner as well.
>
> Hi Nilay,
>
> That's an interesting question. I think we still need
> loop_validate_mutex. Without that mutex the hierarchy could be
> changed from linear into recursive after loop_validate_file()
> has verified the hierarchy and before the loop fd is changed.
> Removing loop_validate_mutex might introduce other race conditions
> than the one mentioned above.
>
Okay, that makes sense. So it looks like we can't easily get rid
of loop_validate_mutex. In that case, would it make sense to always
acquire loop_validate_mutex rather than acquiring it conditionally
only when the new backing file is a loop device?
I think loop_configure() and loop_change_fd() are control-plane
operations and are relatively infrequent, so taking
loop_validate_mutex should not add any noticeable overhead.
In return, we would have a single locking hierarchy and
the locking-context annotations would be much simpler and easier
to reason about.
What do you think?
Thanks,
--Nilay
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
2026-08-24 18:06 ` Nilay Shroff
@ 2026-08-24 20:23 ` Bart Van Assche
0 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-24 20:23 UTC (permalink / raw)
To: Nilay Shroff, Jens Axboe; +Cc: linux-block, Christoph Hellwig
On 8/24/26 11:06 AM, Nilay Shroff wrote:
> Okay, that makes sense. So it looks like we can't easily get rid
> of loop_validate_mutex. In that case, would it make sense to always
> acquire loop_validate_mutex rather than acquiring it conditionally
> only when the new backing file is a loop device?
>
> I think loop_configure() and loop_change_fd() are control-plane
> operations and are relatively infrequent, so taking
> loop_validate_mutex should not add any noticeable overhead.
> In return, we would have a single locking hierarchy and
> the locking-context annotations would be much simpler and easier
> to reason about.
That's an interesting question. I will look into this.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 08/13] loop: Remove memory barriers
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (6 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 07/13] loop: Fix race conditions in loop_validate_file() Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 09/13] loop: Split loop_change_fd() Bart Van Assche
` (4 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Now that all lo_state and lo_backing_file accesses from the control path
are serialized by lo_mutex, it is no longer necessary to use memory
barriers to order the lo_state and lo_backing_file accesses. Hence,
remove these memory barriers. While several lockless accesses of these
two member variables remain in the I/O path, these are serialized with
backing file changes by freezing the request queue. See also
loop_change_fd().
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 8b633ea6e72f..93b589bc6e3d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -515,11 +515,6 @@ static struct file *loop_get_backing_file(struct loop_device *lo)
{
if (lo->lo_state != Lo_bound)
return NULL;
- /*
- * Order wrt setting lo->lo_backing_file in
- * loop_configure().
- */
- rmb();
return get_file(lo->lo_backing_file);
}
@@ -1149,9 +1144,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
size = lo_calculate_size(lo, file);
loop_set_size(lo, size);
- /* Order wrt reading lo_state in loop_validate_file(). */
- wmb();
-
WRITE_ONCE(lo->lo_state, Lo_bound);
if (part_shift)
lo->lo_flags |= LO_FLAGS_PARTSCAN;
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 09/13] loop: Split loop_change_fd()
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (7 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 08/13] loop: Remove memory barriers Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 10/13] loop: Split loop_configure() Bart Van Assche
` (3 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Prepare for adding a second call of __loop_change_fd().
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 86 ++++++++++++++++++++++++--------------------
1 file changed, 47 insertions(+), 39 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 93b589bc6e3d..5ba4cf435103 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -576,6 +576,50 @@ static int loop_check_backing_file(struct file *file)
return 0;
}
+static int __loop_change_fd(struct loop_device *lo, struct block_device *bdev,
+ struct file *file, struct file **old_file,
+ bool *partscan)
+ __must_hold(&lo->lo_mutex)
+{
+ unsigned int memflags;
+ int error;
+
+ if (lo->lo_state != Lo_bound)
+ return -ENXIO;
+
+ /* the loop device has to be read-only */
+ if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
+ return -EINVAL;
+
+ error = loop_validate_file(lo, file, bdev);
+ if (error)
+ return error;
+
+ *old_file = lo->lo_backing_file;
+
+ /* size of the new backing store needs to be the same */
+ if (lo_calculate_size(lo, file) != lo_calculate_size(lo, *old_file))
+ return -EINVAL;
+
+ /*
+ * We might switch to direct I/O mode for the loop device, write back
+ * all dirty data the page cache now that so that the individual I/O
+ * operations don't have to do that.
+ */
+ vfs_fsync(file, 0);
+
+ /* and ... switch */
+ disk_force_media_change(lo->lo_disk);
+ memflags = blk_mq_freeze_queue(lo->lo_queue);
+ mapping_set_gfp_mask((*old_file)->f_mapping, lo->old_gfp_mask);
+ loop_assign_backing_file(lo, file);
+ loop_update_dio(lo);
+ blk_mq_unfreeze_queue(lo->lo_queue, memflags);
+ *partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
+
+ return 0;
+}
+
/*
* loop_change_fd switched the backing store of a loopback device to
* a new file. This is useful for operating system installers to free up
@@ -589,7 +633,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
{
struct file *file = fget(arg);
struct file *old_file;
- unsigned int memflags;
int error;
bool partscan;
bool is_loop;
@@ -610,43 +653,10 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
error = loop_global_lock_killable(lo, is_loop);
if (error)
goto out_putf;
- error = -ENXIO;
- if (lo->lo_state != Lo_bound)
- goto out_err;
-
- /* the loop device has to be read-only */
- error = -EINVAL;
- if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
- goto out_err;
-
- error = loop_validate_file(lo, file, bdev);
- if (error)
- goto out_err;
-
- old_file = lo->lo_backing_file;
-
- error = -EINVAL;
-
- /* size of the new backing store needs to be the same */
- if (lo_calculate_size(lo, file) != lo_calculate_size(lo, old_file))
- goto out_err;
-
- /*
- * We might switch to direct I/O mode for the loop device, write back
- * all dirty data the page cache now that so that the individual I/O
- * operations don't have to do that.
- */
- vfs_fsync(file, 0);
-
- /* and ... switch */
- disk_force_media_change(lo->lo_disk);
- memflags = blk_mq_freeze_queue(lo->lo_queue);
- mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
- loop_assign_backing_file(lo, file);
- loop_update_dio(lo);
- blk_mq_unfreeze_queue(lo->lo_queue, memflags);
- partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
+ error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
loop_global_unlock(lo, is_loop);
+ if (error)
+ goto out_putf;
/*
* Flush loop_validate_file() before fput(), for l->lo_backing_file
@@ -671,8 +681,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
return error;
-out_err:
- loop_global_unlock(lo, is_loop);
out_putf:
fput(file);
dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 10/13] loop: Split loop_configure()
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (8 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 09/13] loop: Split loop_change_fd() Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 11/13] loop: Remove the "bool global" function argument Bart Van Assche
` (2 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Prepare for adding a second __loop_configure() call.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 110 +++++++++++++++++++++++--------------------
1 file changed, 58 insertions(+), 52 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5ba4cf435103..cf65d88d73c2 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1052,61 +1052,29 @@ static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim,
lim->discard_granularity = 0;
}
-static int loop_configure(struct loop_device *lo, blk_mode_t mode,
- struct block_device *bdev,
- const struct loop_config *config)
+static int __loop_configure(struct loop_device *lo, blk_mode_t mode,
+ struct block_device *bdev,
+ const struct loop_config *config, struct file *file,
+ bool *partscan)
+ __must_hold(&lo->lo_mutex)
{
- struct file *file = fget(config->fd);
struct queue_limits lim;
- int error;
loff_t size;
- bool partscan;
- bool is_loop;
-
- if (!file)
- return -EBADF;
-
- error = loop_check_backing_file(file);
- if (error) {
- fput(file);
- return error;
- }
-
- is_loop = is_loop_device(file);
-
- /* This is safe, since we have a reference from open(). */
- __module_get(THIS_MODULE);
-
- /*
- * If we don't hold exclusive handle for the device, upgrade to it
- * here to avoid changing device under exclusive owner.
- */
- if (!(mode & BLK_OPEN_EXCL)) {
- error = bd_prepare_to_claim(bdev, loop_configure, NULL);
- if (error)
- goto out_putf;
- }
-
- error = loop_global_lock_killable(lo, is_loop);
- if (error)
- goto out_bdev;
+ int error;
- error = -EBUSY;
if (lo->lo_state != Lo_unbound)
- goto out_unlock;
+ return -EBUSY;
error = loop_validate_file(lo, file, bdev);
if (error)
- goto out_unlock;
+ return error;
- if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0) {
- error = -EINVAL;
- goto out_unlock;
- }
+ if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0)
+ return -EINVAL;
error = loop_set_status_from_info(lo, &config->info);
if (error)
- goto out_unlock;
+ return error;
lo->lo_flags = config->info.lo_flags;
if (!(file->f_mode & FMODE_WRITE) || !(mode & BLK_OPEN_WRITE) ||
@@ -1117,10 +1085,8 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
lo->workqueue = alloc_workqueue("loop%d",
WQ_UNBOUND | WQ_FREEZABLE,
0, lo->lo_number);
- if (!lo->workqueue) {
- error = -ENOMEM;
- goto out_unlock;
- }
+ if (!lo->workqueue)
+ return -ENOMEM;
}
/* suppress uevents while reconfiguring the device */
@@ -1137,7 +1103,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
/* No need to freeze the queue as the device isn't bound yet. */
error = queue_limits_commit_update(lo->lo_queue, &lim);
if (error)
- goto out_unlock;
+ return error;
/*
* We might switch to direct I/O mode for the loop device, write back
@@ -1155,14 +1121,56 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
WRITE_ONCE(lo->lo_state, Lo_bound);
if (part_shift)
lo->lo_flags |= LO_FLAGS_PARTSCAN;
- partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
- if (partscan)
+ *partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
+ if (*partscan)
clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
+ return 0;
+}
+
+static int loop_configure(struct loop_device *lo, blk_mode_t mode,
+ struct block_device *bdev,
+ const struct loop_config *config)
+{
+ struct file *file = fget(config->fd);
+ int error;
+ bool partscan;
+ bool is_loop;
+
+ if (!file)
+ return -EBADF;
+
+ error = loop_check_backing_file(file);
+ if (error) {
+ fput(file);
+ return error;
+ }
+
+ is_loop = is_loop_device(file);
+
+ /* This is safe, since we have a reference from open(). */
+ __module_get(THIS_MODULE);
+
+ /*
+ * If we don't hold exclusive handle for the device, upgrade to it
+ * here to avoid changing device under exclusive owner.
+ */
+ if (!(mode & BLK_OPEN_EXCL)) {
+ error = bd_prepare_to_claim(bdev, loop_configure, NULL);
+ if (error)
+ goto out_putf;
+ }
+
+ error = loop_global_lock_killable(lo, is_loop);
+ if (error)
+ goto out_bdev;
+ error = __loop_configure(lo, mode, bdev, config, file, &partscan);
loop_global_unlock(lo, is_loop);
+ if (error)
+ goto out_bdev;
if (partscan)
loop_reread_partitions(lo);
@@ -1171,8 +1179,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
return 0;
-out_unlock:
- loop_global_unlock(lo, is_loop);
out_bdev:
if (!(mode & BLK_OPEN_EXCL))
bd_abort_claiming(bdev, loop_configure);
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 11/13] loop: Remove the "bool global" function argument
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (9 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 10/13] loop: Split loop_configure() Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 12/13] loop: Modify the loop_process_work() calling convention Bart Van Assche
2026-08-20 19:57 ` [PATCH 13/13] loop: Add __guarded_by() annotations Bart Van Assche
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Keep the behavior in loop_global_lock_killable() for the global == true
case. Expand loop_global_lock_killable(lo, false) calls into a
mutex_lock_killable() and a mutex_unlock() call.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 71 ++++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 29 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index cf65d88d73c2..8fde5f2d3473 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -100,27 +100,24 @@ static DEFINE_MUTEX(loop_validate_mutex);
* loop_global_lock_killable() - take locks for safe loop_validate_file() test
*
* @lo: struct loop_device
- * @global: true if @lo is about to bind another "struct loop_device", false otherwise
*
* Returns 0 on success, -EINTR otherwise.
*
- * Since loop_validate_file() traverses on other "struct loop_device" if
- * is_loop_device() is true, we need a global lock for serializing concurrent
+ * Since loop_validate_file() traverses on other "struct loop_device", we need a
+ * global lock for serializing concurrent
* loop_configure()/loop_change_fd()/__loop_clr_fd() calls.
*/
-static int loop_global_lock_killable(struct loop_device *lo, bool global)
+static int loop_global_lock_killable(struct loop_device *lo)
+ __cond_acquires(0, &loop_validate_mutex)
__cond_acquires(0, &lo->lo_mutex)
- __context_unsafe(conditional locking)
{
int err;
- if (global) {
- err = mutex_lock_killable(&loop_validate_mutex);
- if (err)
- return err;
- }
+ err = mutex_lock_killable(&loop_validate_mutex);
+ if (err)
+ return err;
err = mutex_lock_killable(&lo->lo_mutex);
- if (err && global)
+ if (err)
mutex_unlock(&loop_validate_mutex);
return err;
}
@@ -129,15 +126,13 @@ static int loop_global_lock_killable(struct loop_device *lo, bool global)
* loop_global_unlock() - release locks taken by loop_global_lock_killable()
*
* @lo: struct loop_device
- * @global: true if @lo was about to bind another "struct loop_device", false otherwise
*/
-static void loop_global_unlock(struct loop_device *lo, bool global)
+static void loop_global_unlock(struct loop_device *lo)
__releases(&lo->lo_mutex)
- __context_unsafe(conditional locking)
+ __releases(&loop_validate_mutex)
{
mutex_unlock(&lo->lo_mutex);
- if (global)
- mutex_unlock(&loop_validate_mutex);
+ mutex_unlock(&loop_validate_mutex);
}
static int max_part;
@@ -650,11 +645,19 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
is_loop = is_loop_device(file);
- error = loop_global_lock_killable(lo, is_loop);
- if (error)
- goto out_putf;
- error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
- loop_global_unlock(lo, is_loop);
+ if (is_loop) {
+ error = loop_global_lock_killable(lo);
+ if (error)
+ goto out_putf;
+ error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
+ loop_global_unlock(lo);
+ } else {
+ error = mutex_lock_killable(&lo->lo_mutex);
+ if (error)
+ goto out_putf;
+ error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
+ mutex_unlock(&lo->lo_mutex);
+ }
if (error)
goto out_putf;
@@ -1164,11 +1167,21 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
goto out_putf;
}
- error = loop_global_lock_killable(lo, is_loop);
- if (error)
- goto out_bdev;
- error = __loop_configure(lo, mode, bdev, config, file, &partscan);
- loop_global_unlock(lo, is_loop);
+ if (is_loop) {
+ error = loop_global_lock_killable(lo);
+ if (error)
+ goto out_bdev;
+ error = __loop_configure(lo, mode, bdev, config, file,
+ &partscan);
+ loop_global_unlock(lo);
+ } else {
+ error = mutex_lock_killable(&lo->lo_mutex);
+ if (error)
+ goto out_bdev;
+ error = __loop_configure(lo, mode, bdev, config, file,
+ &partscan);
+ mutex_unlock(&lo->lo_mutex);
+ }
if (error)
goto out_bdev;
if (partscan)
@@ -1275,11 +1288,11 @@ static int loop_clr_fd(struct loop_device *lo)
* which loop_configure()/loop_change_fd() found via fget() was this
* loop device.
*/
- err = loop_global_lock_killable(lo, true);
+ err = loop_global_lock_killable(lo);
if (err)
return err;
if (lo->lo_state != Lo_bound) {
- loop_global_unlock(lo, true);
+ loop_global_unlock(lo);
return -ENXIO;
}
/*
@@ -1291,7 +1304,7 @@ static int loop_clr_fd(struct loop_device *lo)
lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
if (disk_openers(lo->lo_disk) == 1)
WRITE_ONCE(lo->lo_state, Lo_rundown);
- loop_global_unlock(lo, true);
+ loop_global_unlock(lo);
return 0;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 12/13] loop: Modify the loop_process_work() calling convention
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (10 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 11/13] loop: Remove the "bool global" function argument Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
2026-08-20 19:57 ` [PATCH 13/13] loop: Add __guarded_by() annotations Bart Van Assche
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Prepare for annotating the rootcg_cmd_list member with __guarded_by().
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 8fde5f2d3473..637212af70d2 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2037,13 +2037,15 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
}
static void loop_process_work(struct loop_worker *worker,
- struct list_head *cmd_list, struct loop_device *lo)
+ struct loop_device *lo, bool rootcg)
{
int orig_flags = current->flags;
+ struct list_head *cmd_list;
struct loop_cmd *cmd;
current->flags |= PF_LOCAL_THROTTLE | PF_MEMALLOC_NOIO;
spin_lock_irq(&lo->lo_work_lock);
+ cmd_list = rootcg ? &lo->rootcg_cmd_list : &worker->cmd_list;
while (!list_empty(cmd_list)) {
cmd = container_of(
cmd_list->next, struct loop_cmd, list_entry);
@@ -2074,14 +2076,14 @@ static void loop_workfn(struct work_struct *work)
{
struct loop_worker *worker =
container_of(work, struct loop_worker, work);
- loop_process_work(worker, &worker->cmd_list, worker->lo);
+ loop_process_work(worker, worker->lo, false);
}
static void loop_rootcg_workfn(struct work_struct *work)
{
struct loop_device *lo =
container_of(work, struct loop_device, rootcg_work);
- loop_process_work(NULL, &lo->rootcg_cmd_list, lo);
+ loop_process_work(NULL, lo, true);
}
static const struct blk_mq_ops loop_mq_ops = {
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 13/13] loop: Add __guarded_by() annotations
2026-08-20 19:57 [PATCH 00/13] Improve the loop driver Bart Van Assche
` (11 preceding siblings ...)
2026-08-20 19:57 ` [PATCH 12/13] loop: Modify the loop_process_work() calling convention Bart Van Assche
@ 2026-08-20 19:57 ` Bart Van Assche
12 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-08-20 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Nilay Shroff, Bart Van Assche
Document which synchronization object protects which variable. Suppress
complaints about accesses without locking in initialization and cleanup
functions with context_unsafe() and __assume_ctx_lock().
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/loop.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 637212af70d2..8a6082d887eb 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -60,13 +60,13 @@ struct loop_device {
gfp_t old_gfp_mask;
- int lo_state;
+ int lo_state __guarded_by(&lo_mutex);
spinlock_t lo_work_lock;
struct workqueue_struct *workqueue;
struct work_struct rootcg_work;
- struct list_head rootcg_cmd_list;
- struct list_head idle_worker_list;
- struct rb_root worker_tree;
+ struct list_head rootcg_cmd_list __guarded_by(&lo_work_lock);
+ struct list_head idle_worker_list __guarded_by(&lo_work_lock);
+ struct rb_root worker_tree __guarded_by(&lo_work_lock);
struct timer_list timer;
bool sysfs_inited;
@@ -92,8 +92,8 @@ struct loop_cmd {
#define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
#define LOOP_DEFAULT_HW_Q_DEPTH 128
-static DEFINE_IDR(loop_index_idr);
static DEFINE_MUTEX(loop_ctl_mutex);
+static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);
static DEFINE_MUTEX(loop_validate_mutex);
/**
@@ -2107,10 +2107,10 @@ static int loop_add(int i)
lo = kzalloc_obj(*lo);
if (!lo)
goto out;
- lo->worker_tree = RB_ROOT;
- INIT_LIST_HEAD(&lo->idle_worker_list);
+ context_unsafe(lo->worker_tree = RB_ROOT);
+ context_unsafe(INIT_LIST_HEAD(&lo->idle_worker_list));
timer_setup(&lo->timer, loop_free_idle_workers_timer, TIMER_DEFERRABLE);
- WRITE_ONCE(lo->lo_state, Lo_unbound);
+ context_unsafe(WRITE_ONCE(lo->lo_state, Lo_unbound));
err = mutex_lock_killable(&loop_ctl_mutex);
if (err)
@@ -2173,7 +2173,7 @@ static int loop_add(int i)
lo->lo_number = i;
spin_lock_init(&lo->lo_work_lock);
INIT_WORK(&lo->rootcg_work, loop_rootcg_workfn);
- INIT_LIST_HEAD(&lo->rootcg_cmd_list);
+ context_unsafe(INIT_LIST_HEAD(&lo->rootcg_cmd_list));
disk->major = LOOP_MAJOR;
disk->first_minor = i << part_shift;
disk->minors = 1 << part_shift;
@@ -2405,6 +2405,7 @@ static void __exit loop_exit(void)
* module unloading is requested). If this is not a clean unloading,
* we have no means to avoid kernel crash.
*/
+ __assume_ctx_lock(&loop_ctl_mutex);
idr_for_each_entry(&loop_index_idr, lo, id)
loop_remove(lo);
^ permalink raw reply related [flat|nested] 18+ messages in thread