[PATCH 00/13] Improve the loop driver

[PATCH 00/13] Improve the loop driver

[Bart Van Assche]

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 01/13] loop: Fix the code for recursion detection

[Bart Van Assche]

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(). */

[PATCH 02/13] loop: Reorder checks in loop_validate_file()

[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;
 }
 

[PATCH 03/13] loop: Enable context analysis

[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)

[PATCH 04/13] loop: Assign a unique lockdep key to each lo_mutex instance

[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);

[PATCH 05/13] loop: Add more __must_hold() annotations

[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;

[PATCH 06/13] loop: Protect all lo_backing_file accesses with lo->lo_mutex

[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);

[PATCH 07/13] loop: Fix race conditions in loop_validate_file()

[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;
 }
 

[PATCH 08/13] loop: Remove memory barriers

[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;

[PATCH 09/13] loop: Split loop_change_fd()

[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);

[PATCH 10/13] loop: Split loop_configure()

[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);

[PATCH 11/13] loop: Remove the "bool global" function argument

[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;
 }

[PATCH 12/13] loop: Modify the loop_process_work() calling convention

[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 = {

[PATCH 13/13] loop: Add __guarded_by() annotations

[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);