Improve the loop driver
Subsystems (2)
cover Improve the loop driver
Hi Jens,
This patch series fixes two bugs and enables lock context analysis in the loop driver. While I am aware that the merge window is open, I'm posting this patch series anyway such that reviewing of this patch series can start.
This series of patches is a reworked version of a subset of the series "[PATCH v2 00/12] Enable lock context analysis in drivers/block/". In this version Nilay's feedback has been addressed.
Thanks,
Bart.
Bart Van Assche (13):
loop: Fix the code for recursion detection
loop: Reorder checks in loop_validate_file()
loop: Enable context analysis
loop: Assign a unique lockdep key to each lo_mutex instance
loop: Add more __must_hold() annotations
loop: Protect all lo_backing_file accesses with lo->lo_mutex
loop: Fix race conditions in loop_validate_file()
loop: Remove memory barriers
loop: Split loop_change_fd()
loop: Split loop_configure()
loop: Remove the "bool global" function argument
loop: Modify the loop_process_work() calling convention
loop: Add __guarded_by() annotations
drivers/block/Makefile | 2 + drivers/block/loop.c | 382 ++++++++++++++++++++++++----------------- 2 files changed, 230 insertions(+), 154 deletions(-)
PATCH 1/13 loop: Fix the code for recursion detection
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.
--- drivers/block/loop.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
drivers/block/loop.c +19 -5
@@ -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(). */
PATCH 2/13 loop: Reorder checks in loop_validate_file()
In reply to Bart Van AsscheChecking 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.
--- drivers/block/loop.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/block/loop.c +3 -2
@@ -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 ; }
PATCH 3/13 loop: Enable context analysis
In reply to Bart Van AsscheLet 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.
--- drivers/block/Makefile | 2 ++ drivers/block/loop.c | 4 ++++ 2 files changed, 6 insertions(+)
drivers/block/Makefile +2 -0
@@ -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)
drivers/block/loop.c +4 -0
@@ -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)
PATCH 4/13 loop: Assign a unique lockdep key to each lo_mutex instance
In reply to Bart Van AsschePrepare for nesting lo_mutex in loop_validate_file().
--- drivers/block/loop.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/block/loop.c +5 -1
@@ -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);
PATCH 5/13 loop: Add more __must_hold() annotations
In reply to Bart Van AsscheAnnotate 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.
--- drivers/block/loop.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
drivers/block/loop.c +17 -3
@@ -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;
PATCH 6/13 loop: Protect all lo_backing_file accesses with lo->lo_mutex
In reply to Bart Van AsscheProtect 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.
--- drivers/block/loop.c | 45 ++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)
drivers/block/loop.c +27 -18
@@ -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);
PATCH 7/13 loop: Fix race conditions in loop_validate_file()
In reply to Bart Van AsscheFix 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.
--- drivers/block/loop.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/block/loop.c +8 -2
@@ -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 ; }
Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
Quoted 45 lines · 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 deferringfput()until after we have locked the target device'slo_mutexand confirmed it is in theLo_boundstate.Signed-off-by Bart Van Assche <[email protected]>--- drivers/block/loop.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/block/loop.c +6
-2@@ -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;
drivers/block/loop.c static int loop_validate_file(struct loop_device *lo, struct file *file, · line 560
Quoted 10 lines
drivers/block/loop.c +2
-0scoped_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
Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
Quoted 13 lines · Nilay Shroff
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_mutexwhile checkinglo_stateand acquiring that reference,loop_validate_mutexmay no longer be necessary.In particular,
loop_get_backing_file()now atomically checks that
the loop device is inLo_boundstate and takes a reference tolo_backing_filewhile holdinglo_mutex. Therefore, ifloop_clr_fd()
orloop_change_fd()concurrently replaces or clears the backing
file, the validator still holds its own reference. It also appears
thatloop_change_fd()andloop_clr_fd()for the same loop device are already serialized bylo_mutex.
Quoted 4 lines
So I am wondering whether
loop_validate_mutexnow 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.
Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
Quoted 19 lines · Bart Van Assche
Quoted 13 lines · Nilay Shroff
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_mutexwhile checkinglo_stateand acquiring that reference,loop_validate_mutexmay no longer be necessary.In particular,
loop_get_backing_file()now atomically checks that
the loop device is inLo_boundstate and takes a reference tolo_backing_filewhile holdinglo_mutex. Therefore, ifloop_clr_fd()
orloop_change_fd()concurrently replaces or clears the backing
file, the validator still holds its own reference. It also appears
thatloop_change_fd()andloop_clr_fd()for the same loop device are already serialized bylo_mutex.Quoted 4 lines
So I am wondering whether
loop_validate_mutexnow 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.
Quoted 9 lines
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 afterloop_validate_file()has verified the hierarchy and before the loop fd is changed. Removingloop_validate_mutexmight 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
Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()
Quoted 11 lines · Nilay Shroff
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 acquireloop_validate_mutexrather than acquiring it conditionally only when the new backing file is a loop device?I think
loop_configure()andloop_change_fd()are control-plane operations and are relatively infrequent, so takingloop_validate_mutexshould 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.
PATCH 8/13 loop: Remove memory barriers
In reply to Bart Van AsscheNow 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().
--- drivers/block/loop.c | 8 -------- 1 file changed, 8 deletions(-)
drivers/block/loop.c +0 -8
@@ -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 ;
PATCH 9/13 loop: Split loop_change_fd()
In reply to Bart Van AsschePrepare for adding a second call of __loop_change_fd().
--- drivers/block/loop.c | 86 ++++++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 39 deletions(-)
drivers/block/loop.c +47 -39
@@ -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 );
PATCH 10/13 loop: Split loop_configure()
In reply to Bart Van AsschePrepare for adding a second __loop_configure() call.
--- drivers/block/loop.c | 110 +++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 52 deletions(-)
drivers/block/loop.c +58 -52
@@ -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);
PATCH 11/13 loop: Remove the "bool global" function argument
In reply to Bart Van AsscheKeep 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.
--- drivers/block/loop.c | 71 ++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 29 deletions(-)
drivers/block/loop.c +42 -29
@@ -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 ; }
PATCH 12/13 loop: Modify the loop_process_work() calling convention
In reply to Bart Van AsschePrepare for annotating the rootcg_cmd_list member with __guarded_by().
--- drivers/block/loop.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/block/loop.c +5 -3
@@ -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 = {
PATCH 13/13 loop: Add __guarded_by() annotations
In reply to Bart Van AsscheDocument which synchronization object protects which variable. Suppress complaints about accesses without locking in initialization and cleanup functions with context_unsafe() and __assume_ctx_lock().
--- drivers/block/loop.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
drivers/block/loop.c +10 -9
@@ -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);