All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation.
@ 2018-09-25 15:26 Tetsuo Handa
  2018-09-25 15:26 ` [PATCH 2/4] block/loop: Use global lock for ioctl() operation Tetsuo Handa
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Tetsuo Handa @ 2018-09-25 15:26 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Jan Kara, Tetsuo Handa

vfs_getattr() needs "struct path" rather than "struct file".
Let's use path_get()/path_put() rather than get_file()/fput().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 drivers/block/loop.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index abad6d1..c2745e6 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1206,7 +1206,7 @@ static int loop_clr_fd(struct loop_device *lo)
 static int
 loop_get_status(struct loop_device *lo, struct loop_info64 *info)
 {
-	struct file *file;
+	struct path path;
 	struct kstat stat;
 	int ret;
 
@@ -1231,16 +1231,16 @@ static int loop_clr_fd(struct loop_device *lo)
 	}
 
 	/* Drop lo_ctl_mutex while we call into the filesystem. */
-	file = get_file(lo->lo_backing_file);
+	path = lo->lo_backing_file->f_path;
+	path_get(&path);
 	mutex_unlock(&lo->lo_ctl_mutex);
-	ret = vfs_getattr(&file->f_path, &stat, STATX_INO,
-			  AT_STATX_SYNC_AS_STAT);
+	ret = vfs_getattr(&path, &stat, STATX_INO, AT_STATX_SYNC_AS_STAT);
 	if (!ret) {
 		info->lo_device = huge_encode_dev(stat.dev);
 		info->lo_inode = stat.ino;
 		info->lo_rdevice = huge_encode_dev(stat.rdev);
 	}
-	fput(file);
+	path_put(&path);
 	return ret;
 }
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 2/4] block/loop: Use global lock for ioctl() operation.
  2018-09-25 15:26 [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation Tetsuo Handa
@ 2018-09-25 15:26 ` Tetsuo Handa
  2018-09-25 15:26 ` [PATCH 3/4] block/loop: Reorganize loop_reread_partitions() callers Tetsuo Handa
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Tetsuo Handa @ 2018-09-25 15:26 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Jan Kara, Tetsuo Handa, syzbot

syzbot is reporting NULL pointer dereference [1] which is caused by
race condition between ioctl(loop_fd, LOOP_CLR_FD, 0) versus
ioctl(other_loop_fd, LOOP_SET_FD, loop_fd) due to traversing other
loop devices at loop_validate_file() without holding corresponding
lo->lo_ctl_mutex locks.

Since ioctl() request on loop devices is not frequent operation, we don't
need fine grained locking. Let's use global lock in order to allow safe
traversal at loop_validate_file().

Note that syzbot is also reporting circular locking dependency between
bdev->bd_mutex and lo->lo_ctl_mutex [2] which is caused by calling
blkdev_reread_part() with lock held. This patch does not address it.

[1] https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3
[2] https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+bf89c128e05dd6c62523@syzkaller.appspotmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 drivers/block/loop.c | 58 ++++++++++++++++++++++++++--------------------------
 drivers/block/loop.h |  1 -
 2 files changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c2745e6..920cbb1 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -85,6 +85,7 @@
 
 static DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_index_mutex);
+static DEFINE_MUTEX(loop_ctl_mutex);
 
 static int max_part;
 static int part_shift;
@@ -1048,7 +1049,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	 */
 	if (atomic_read(&lo->lo_refcnt) > 1) {
 		lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
-		mutex_unlock(&lo->lo_ctl_mutex);
+		mutex_unlock(&loop_ctl_mutex);
 		return 0;
 	}
 
@@ -1101,12 +1102,12 @@ static int loop_clr_fd(struct loop_device *lo)
 	if (!part_shift)
 		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
 	loop_unprepare_queue(lo);
-	mutex_unlock(&lo->lo_ctl_mutex);
+	mutex_unlock(&loop_ctl_mutex);
 	/*
-	 * Need not hold lo_ctl_mutex to fput backing file.
-	 * Calling fput holding lo_ctl_mutex triggers a circular
+	 * Need not hold loop_ctl_mutex to fput backing file.
+	 * Calling fput holding loop_ctl_mutex triggers a circular
 	 * lock dependency possibility warning as fput can take
-	 * bd_mutex which is usually taken before lo_ctl_mutex.
+	 * bd_mutex which is usually taken before loop_ctl_mutex.
 	 */
 	fput(filp);
 	return 0;
@@ -1211,7 +1212,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	int ret;
 
 	if (lo->lo_state != Lo_bound) {
-		mutex_unlock(&lo->lo_ctl_mutex);
+		mutex_unlock(&loop_ctl_mutex);
 		return -ENXIO;
 	}
 
@@ -1230,10 +1231,10 @@ static int loop_clr_fd(struct loop_device *lo)
 		       lo->lo_encrypt_key_size);
 	}
 
-	/* Drop lo_ctl_mutex while we call into the filesystem. */
+	/* Drop loop_ctl_mutex while we call into the filesystem. */
 	path = lo->lo_backing_file->f_path;
 	path_get(&path);
-	mutex_unlock(&lo->lo_ctl_mutex);
+	mutex_unlock(&loop_ctl_mutex);
 	ret = vfs_getattr(&path, &stat, STATX_INO, AT_STATX_SYNC_AS_STAT);
 	if (!ret) {
 		info->lo_device = huge_encode_dev(stat.dev);
@@ -1325,7 +1326,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	int err;
 
 	if (!arg) {
-		mutex_unlock(&lo->lo_ctl_mutex);
+		mutex_unlock(&loop_ctl_mutex);
 		return -EINVAL;
 	}
 	err = loop_get_status(lo, &info64);
@@ -1343,7 +1344,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	int err;
 
 	if (!arg) {
-		mutex_unlock(&lo->lo_ctl_mutex);
+		mutex_unlock(&loop_ctl_mutex);
 		return -EINVAL;
 	}
 	err = loop_get_status(lo, &info64);
@@ -1401,7 +1402,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 	struct loop_device *lo = bdev->bd_disk->private_data;
 	int err;
 
-	err = mutex_lock_killable_nested(&lo->lo_ctl_mutex, 1);
+	err = mutex_lock_killable_nested(&loop_ctl_mutex, 1);
 	if (err)
 		goto out_unlocked;
 
@@ -1413,7 +1414,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		err = loop_change_fd(lo, bdev, arg);
 		break;
 	case LOOP_CLR_FD:
-		/* loop_clr_fd would have unlocked lo_ctl_mutex on success */
+		/* loop_clr_fd would have unlocked loop_ctl_mutex on success */
 		err = loop_clr_fd(lo);
 		if (!err)
 			goto out_unlocked;
@@ -1426,7 +1427,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		break;
 	case LOOP_GET_STATUS:
 		err = loop_get_status_old(lo, (struct loop_info __user *) arg);
-		/* loop_get_status() unlocks lo_ctl_mutex */
+		/* loop_get_status() unlocks loop_ctl_mutex */
 		goto out_unlocked;
 	case LOOP_SET_STATUS64:
 		err = -EPERM;
@@ -1436,7 +1437,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		break;
 	case LOOP_GET_STATUS64:
 		err = loop_get_status64(lo, (struct loop_info64 __user *) arg);
-		/* loop_get_status() unlocks lo_ctl_mutex */
+		/* loop_get_status() unlocks loop_ctl_mutex */
 		goto out_unlocked;
 	case LOOP_SET_CAPACITY:
 		err = -EPERM;
@@ -1456,7 +1457,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 	default:
 		err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
 	}
-	mutex_unlock(&lo->lo_ctl_mutex);
+	mutex_unlock(&loop_ctl_mutex);
 
 out_unlocked:
 	return err;
@@ -1573,7 +1574,7 @@ struct compat_loop_info {
 	int err;
 
 	if (!arg) {
-		mutex_unlock(&lo->lo_ctl_mutex);
+		mutex_unlock(&loop_ctl_mutex);
 		return -EINVAL;
 	}
 	err = loop_get_status(lo, &info64);
@@ -1590,19 +1591,19 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 
 	switch(cmd) {
 	case LOOP_SET_STATUS:
-		err = mutex_lock_killable(&lo->lo_ctl_mutex);
+		err = mutex_lock_killable(&loop_ctl_mutex);
 		if (!err) {
 			err = loop_set_status_compat(lo,
 						     (const struct compat_loop_info __user *)arg);
-			mutex_unlock(&lo->lo_ctl_mutex);
+			mutex_unlock(&loop_ctl_mutex);
 		}
 		break;
 	case LOOP_GET_STATUS:
-		err = mutex_lock_killable(&lo->lo_ctl_mutex);
+		err = mutex_lock_killable(&loop_ctl_mutex);
 		if (!err) {
 			err = loop_get_status_compat(lo,
 						     (struct compat_loop_info __user *)arg);
-			/* loop_get_status() unlocks lo_ctl_mutex */
+			/* loop_get_status() unlocks loop_ctl_mutex */
 		}
 		break;
 	case LOOP_SET_CAPACITY:
@@ -1649,7 +1650,7 @@ static void __lo_release(struct loop_device *lo)
 	if (atomic_dec_return(&lo->lo_refcnt))
 		return;
 
-	mutex_lock(&lo->lo_ctl_mutex);
+	mutex_lock(&loop_ctl_mutex);
 	if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
 		/*
 		 * In autoclear mode, stop the loop thread
@@ -1667,7 +1668,7 @@ static void __lo_release(struct loop_device *lo)
 		blk_mq_unfreeze_queue(lo->lo_queue);
 	}
 
-	mutex_unlock(&lo->lo_ctl_mutex);
+	mutex_unlock(&loop_ctl_mutex);
 }
 
 static void lo_release(struct gendisk *disk, fmode_t mode)
@@ -1713,10 +1714,10 @@ static int unregister_transfer_cb(int id, void *ptr, void *data)
 	struct loop_device *lo = ptr;
 	struct loop_func_table *xfer = data;
 
-	mutex_lock(&lo->lo_ctl_mutex);
+	mutex_lock(&loop_ctl_mutex);
 	if (lo->lo_encryption == xfer)
 		loop_release_xfer(lo);
-	mutex_unlock(&lo->lo_ctl_mutex);
+	mutex_unlock(&loop_ctl_mutex);
 	return 0;
 }
 
@@ -1897,7 +1898,6 @@ static int loop_add(struct loop_device **l, int i)
 	if (!part_shift)
 		disk->flags |= GENHD_FL_NO_PART_SCAN;
 	disk->flags |= GENHD_FL_EXT_DEVT;
-	mutex_init(&lo->lo_ctl_mutex);
 	atomic_set(&lo->lo_refcnt, 0);
 	lo->lo_number		= i;
 	spin_lock_init(&lo->lo_lock);
@@ -2010,21 +2010,21 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 		ret = loop_lookup(&lo, parm);
 		if (ret < 0)
 			break;
-		ret = mutex_lock_killable(&lo->lo_ctl_mutex);
+		ret = mutex_lock_killable(&loop_ctl_mutex);
 		if (ret)
 			break;
 		if (lo->lo_state != Lo_unbound) {
 			ret = -EBUSY;
-			mutex_unlock(&lo->lo_ctl_mutex);
+			mutex_unlock(&loop_ctl_mutex);
 			break;
 		}
 		if (atomic_read(&lo->lo_refcnt) > 0) {
 			ret = -EBUSY;
-			mutex_unlock(&lo->lo_ctl_mutex);
+			mutex_unlock(&loop_ctl_mutex);
 			break;
 		}
 		lo->lo_disk->private_data = NULL;
-		mutex_unlock(&lo->lo_ctl_mutex);
+		mutex_unlock(&loop_ctl_mutex);
 		idr_remove(&loop_index_idr, lo->lo_number);
 		loop_remove(lo);
 		break;
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index 4d42c7a..af75a5e 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -54,7 +54,6 @@ struct loop_device {
 
 	spinlock_t		lo_lock;
 	int			lo_state;
-	struct mutex		lo_ctl_mutex;
 	struct kthread_worker	worker;
 	struct task_struct	*worker_task;
 	bool			use_dio;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 3/4] block/loop: Reorganize loop_reread_partitions() callers.
  2018-09-25 15:26 [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation Tetsuo Handa
  2018-09-25 15:26 ` [PATCH 2/4] block/loop: Use global lock for ioctl() operation Tetsuo Handa
@ 2018-09-25 15:26 ` Tetsuo Handa
  2018-09-25 15:26 ` [PATCH 4/4] block/loop: Fix circular locking dependency at blkdev_reread_part() Tetsuo Handa
  2018-09-25 18:59 ` [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation Omar Sandoval
  3 siblings, 0 replies; 8+ messages in thread
From: Tetsuo Handa @ 2018-09-25 15:26 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Jan Kara, Tetsuo Handa, Ming Lei

We will drop loop_ctl_mutex before calling blkdev_reread_part() in
order to fix circular locking dependency between bdev->bd_mutex and
loop_ctl_mutex. To do that we need to make sure that we won't touch
"struct loop_device" after releasing loop_ctl_mutex.

As a preparation step, this patch reorganizes loop_reread_partitions()
callers. According to Ming Lei, calling loop_unprepare_queue() before
loop_reread_partitions() (like we did until 3.19) is fine. Therefore,
this patch will not cause user visible changes.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/loop.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 920cbb1..4b05a27 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -632,6 +632,11 @@ static void loop_reread_partitions(struct loop_device *lo,
 				   struct block_device *bdev)
 {
 	int rc;
+	char filename[LO_NAME_SIZE];
+	const int num = lo->lo_number;
+	const int count = atomic_read(&lo->lo_refcnt);
+
+	memcpy(filename, lo->lo_file_name, sizeof(filename));
 
 	/*
 	 * bd_mutex has been held already in release path, so don't
@@ -641,13 +646,13 @@ static void loop_reread_partitions(struct loop_device *lo,
 	 * must be at least one and it can only become zero when the
 	 * current holder is released.
 	 */
-	if (!atomic_read(&lo->lo_refcnt))
+	if (!count)
 		rc = __blkdev_reread_part(bdev);
 	else
 		rc = blkdev_reread_part(bdev);
 	if (rc)
 		pr_warn("%s: partition scan of loop%d (%s) failed (rc=%d)\n",
-			__func__, lo->lo_number, lo->lo_file_name, rc);
+			__func__, num, filename, rc);
 }
 
 static inline int is_loop_device(struct file *file)
@@ -730,9 +735,9 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	loop_update_dio(lo);
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
-	fput(old_file);
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN)
 		loop_reread_partitions(lo, bdev);
+	fput(old_file);
 	return 0;
 
  out_putf:
@@ -971,16 +976,18 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	set_blocksize(bdev, S_ISBLK(inode->i_mode) ?
 		      block_size(inode->i_bdev) : PAGE_SIZE);
 
+	/*
+	 * Grab the block_device to prevent its destruction after we
+	 * put /dev/loopXX inode. Later in loop_clr_fd() we bdput(bdev).
+	 */
+	bdgrab(bdev);
+
 	lo->lo_state = Lo_bound;
 	if (part_shift)
 		lo->lo_flags |= LO_FLAGS_PARTSCAN;
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN)
 		loop_reread_partitions(lo, bdev);
 
-	/* Grab the block_device to prevent its destruction after we
-	 * put /dev/loopXX inode. Later in loop_clr_fd() we bdput(bdev).
-	 */
-	bdgrab(bdev);
 	return 0;
 
  out_putf:
@@ -1033,6 +1040,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	struct file *filp = lo->lo_backing_file;
 	gfp_t gfp = lo->old_gfp_mask;
 	struct block_device *bdev = lo->lo_device;
+	bool reread;
 
 	if (lo->lo_state != Lo_bound)
 		return -ENXIO;
@@ -1096,12 +1104,13 @@ static int loop_clr_fd(struct loop_device *lo)
 	module_put(THIS_MODULE);
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
-	if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev)
-		loop_reread_partitions(lo, bdev);
+	reread = (lo->lo_flags & LO_FLAGS_PARTSCAN) && bdev;
+	loop_unprepare_queue(lo);
 	lo->lo_flags = 0;
 	if (!part_shift)
 		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
-	loop_unprepare_queue(lo);
+	if (reread)
+		loop_reread_partitions(lo, bdev);
 	mutex_unlock(&loop_ctl_mutex);
 	/*
 	 * Need not hold loop_ctl_mutex to fput backing file.
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 4/4] block/loop: Fix circular locking dependency at blkdev_reread_part().
  2018-09-25 15:26 [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation Tetsuo Handa
  2018-09-25 15:26 ` [PATCH 2/4] block/loop: Use global lock for ioctl() operation Tetsuo Handa
  2018-09-25 15:26 ` [PATCH 3/4] block/loop: Reorganize loop_reread_partitions() callers Tetsuo Handa
@ 2018-09-25 15:26 ` Tetsuo Handa
  2018-09-27 11:27   ` Jan Kara
  2018-09-25 18:59 ` [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation Omar Sandoval
  3 siblings, 1 reply; 8+ messages in thread
From: Tetsuo Handa @ 2018-09-25 15:26 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Jan Kara, Tetsuo Handa, syzbot

syzbot is reporting circular locking dependency between bdev->bd_mutex
and lo->lo_ctl_mutex [1] which is caused by calling blkdev_reread_part()
with lock held. We need to drop lo->lo_ctl_mutex in order to fix it.

This patch fixes it by combining loop_index_mutex and loop_ctl_mutex into
loop_mutex, and releasing loop_mutex before calling blkdev_reread_part()
or fput() or path_put() or leaving ioctl().

The rule is that current thread calls lock_loop() before accessing
"struct loop_device", and current thread no longer accesses "struct
loop_device" after unlock_loop() is called.

Since syzbot is reporting various bugs [2] where a race in the loop module
is suspected, let's check whether this patch affects these bugs too.

[1] https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889
[2] https://syzkaller.appspot.com/bug?id=b3c7e1440aa8ece16bf557dbac427fdff1dad9d6

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+4684a000d5abdade83fac55b1e7d1f935ef1936e@syzkaller.appspotmail.com>
---
 drivers/block/loop.c | 187 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 101 insertions(+), 86 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 4b05a27..04389bb 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -84,12 +84,50 @@
 #include <linux/uaccess.h>
 
 static DEFINE_IDR(loop_index_idr);
-static DEFINE_MUTEX(loop_index_mutex);
-static DEFINE_MUTEX(loop_ctl_mutex);
+static DEFINE_MUTEX(loop_mutex);
+static void *loop_mutex_owner; /* == __mutex_owner(&loop_mutex) */
 
 static int max_part;
 static int part_shift;
 
+/*
+ * lock_loop - Lock loop_mutex.
+ */
+static void lock_loop(void)
+{
+	mutex_lock(&loop_mutex);
+	loop_mutex_owner = current;
+}
+
+/*
+ * lock_loop_killable - Lock loop_mutex unless killed.
+ */
+static int lock_loop_killable(void)
+{
+	int err = mutex_lock_killable(&loop_mutex);
+
+	if (err)
+		return err;
+	loop_mutex_owner = current;
+	return 0;
+}
+
+/*
+ * unlock_loop - Unlock loop_mutex as needed.
+ *
+ * Explicitly call this function before calling fput() or blkdev_reread_part()
+ * in order to avoid circular lock dependency. After this function is called,
+ * current thread is no longer allowed to access "struct loop_device" memory,
+ * for another thread would access that memory as soon as loop_mutex is held.
+ */
+static void unlock_loop(void)
+{
+	if (loop_mutex_owner == current) {
+		loop_mutex_owner = NULL;
+		mutex_unlock(&loop_mutex);
+	}
+}
+
 static int transfer_xor(struct loop_device *lo, int cmd,
 			struct page *raw_page, unsigned raw_off,
 			struct page *loop_page, unsigned loop_off,
@@ -637,6 +675,7 @@ static void loop_reread_partitions(struct loop_device *lo,
 	const int count = atomic_read(&lo->lo_refcnt);
 
 	memcpy(filename, lo->lo_file_name, sizeof(filename));
+	unlock_loop();
 
 	/*
 	 * bd_mutex has been held already in release path, so don't
@@ -699,6 +738,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	struct file	*file, *old_file;
 	int		error;
 
+	lockdep_assert_held(&loop_mutex);
 	error = -ENXIO;
 	if (lo->lo_state != Lo_bound)
 		goto out;
@@ -737,10 +777,12 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN)
 		loop_reread_partitions(lo, bdev);
+	unlock_loop();
 	fput(old_file);
 	return 0;
 
  out_putf:
+	unlock_loop();
 	fput(file);
  out:
 	return error;
@@ -918,6 +960,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	int		error;
 	loff_t		size;
 
+	lockdep_assert_held(&loop_mutex);
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
 
@@ -991,6 +1034,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	return 0;
 
  out_putf:
+	unlock_loop();
 	fput(file);
  out:
 	/* This is safe: open() is still holding a reference. */
@@ -1042,6 +1086,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	struct block_device *bdev = lo->lo_device;
 	bool reread;
 
+	lockdep_assert_held(&loop_mutex);
 	if (lo->lo_state != Lo_bound)
 		return -ENXIO;
 
@@ -1057,7 +1102,6 @@ static int loop_clr_fd(struct loop_device *lo)
 	 */
 	if (atomic_read(&lo->lo_refcnt) > 1) {
 		lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
-		mutex_unlock(&loop_ctl_mutex);
 		return 0;
 	}
 
@@ -1111,13 +1155,7 @@ static int loop_clr_fd(struct loop_device *lo)
 		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
 	if (reread)
 		loop_reread_partitions(lo, bdev);
-	mutex_unlock(&loop_ctl_mutex);
-	/*
-	 * Need not hold loop_ctl_mutex to fput backing file.
-	 * Calling fput holding loop_ctl_mutex triggers a circular
-	 * lock dependency possibility warning as fput can take
-	 * bd_mutex which is usually taken before loop_ctl_mutex.
-	 */
+	unlock_loop();
 	fput(filp);
 	return 0;
 }
@@ -1129,6 +1167,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	struct loop_func_table *xfer;
 	kuid_t uid = current_uid();
 
+	lockdep_assert_held(&loop_mutex);
 	if (lo->lo_encrypt_key_size &&
 	    !uid_eq(lo->lo_key_owner, uid) &&
 	    !capable(CAP_SYS_ADMIN))
@@ -1220,10 +1259,9 @@ static int loop_clr_fd(struct loop_device *lo)
 	struct kstat stat;
 	int ret;
 
-	if (lo->lo_state != Lo_bound) {
-		mutex_unlock(&loop_ctl_mutex);
+	lockdep_assert_held(&loop_mutex);
+	if (lo->lo_state != Lo_bound)
 		return -ENXIO;
-	}
 
 	memset(info, 0, sizeof(*info));
 	info->lo_number = lo->lo_number;
@@ -1240,10 +1278,10 @@ static int loop_clr_fd(struct loop_device *lo)
 		       lo->lo_encrypt_key_size);
 	}
 
-	/* Drop loop_ctl_mutex while we call into the filesystem. */
+	/* Drop loop_mutex while we call into the filesystem. */
 	path = lo->lo_backing_file->f_path;
 	path_get(&path);
-	mutex_unlock(&loop_ctl_mutex);
+	unlock_loop();
 	ret = vfs_getattr(&path, &stat, STATX_INO, AT_STATX_SYNC_AS_STAT);
 	if (!ret) {
 		info->lo_device = huge_encode_dev(stat.dev);
@@ -1334,10 +1372,8 @@ static int loop_clr_fd(struct loop_device *lo)
 	struct loop_info64 info64;
 	int err;
 
-	if (!arg) {
-		mutex_unlock(&loop_ctl_mutex);
+	if (!arg)
 		return -EINVAL;
-	}
 	err = loop_get_status(lo, &info64);
 	if (!err)
 		err = loop_info64_to_old(&info64, &info);
@@ -1352,10 +1388,8 @@ static int loop_clr_fd(struct loop_device *lo)
 	struct loop_info64 info64;
 	int err;
 
-	if (!arg) {
-		mutex_unlock(&loop_ctl_mutex);
+	if (!arg)
 		return -EINVAL;
-	}
 	err = loop_get_status(lo, &info64);
 	if (!err && copy_to_user(arg, &info64, sizeof(info64)))
 		err = -EFAULT;
@@ -1365,6 +1399,7 @@ static int loop_clr_fd(struct loop_device *lo)
 
 static int loop_set_capacity(struct loop_device *lo)
 {
+	lockdep_assert_held(&loop_mutex);
 	if (unlikely(lo->lo_state != Lo_bound))
 		return -ENXIO;
 
@@ -1374,6 +1409,8 @@ static int loop_set_capacity(struct loop_device *lo)
 static int loop_set_dio(struct loop_device *lo, unsigned long arg)
 {
 	int error = -ENXIO;
+
+	lockdep_assert_held(&loop_mutex);
 	if (lo->lo_state != Lo_bound)
 		goto out;
 
@@ -1387,6 +1424,7 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
 
 static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
 {
+	lockdep_assert_held(&loop_mutex);
 	if (lo->lo_state != Lo_bound)
 		return -ENXIO;
 
@@ -1409,11 +1447,10 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 	unsigned int cmd, unsigned long arg)
 {
 	struct loop_device *lo = bdev->bd_disk->private_data;
-	int err;
+	int err = lock_loop_killable();
 
-	err = mutex_lock_killable_nested(&loop_ctl_mutex, 1);
 	if (err)
-		goto out_unlocked;
+		return err;
 
 	switch (cmd) {
 	case LOOP_SET_FD:
@@ -1423,10 +1460,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		err = loop_change_fd(lo, bdev, arg);
 		break;
 	case LOOP_CLR_FD:
-		/* loop_clr_fd would have unlocked loop_ctl_mutex on success */
 		err = loop_clr_fd(lo);
-		if (!err)
-			goto out_unlocked;
 		break;
 	case LOOP_SET_STATUS:
 		err = -EPERM;
@@ -1436,8 +1470,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		break;
 	case LOOP_GET_STATUS:
 		err = loop_get_status_old(lo, (struct loop_info __user *) arg);
-		/* loop_get_status() unlocks loop_ctl_mutex */
-		goto out_unlocked;
+		break;
 	case LOOP_SET_STATUS64:
 		err = -EPERM;
 		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
@@ -1446,8 +1479,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		break;
 	case LOOP_GET_STATUS64:
 		err = loop_get_status64(lo, (struct loop_info64 __user *) arg);
-		/* loop_get_status() unlocks loop_ctl_mutex */
-		goto out_unlocked;
+		break;
 	case LOOP_SET_CAPACITY:
 		err = -EPERM;
 		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
@@ -1466,9 +1498,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 	default:
 		err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
 	}
-	mutex_unlock(&loop_ctl_mutex);
-
-out_unlocked:
+	unlock_loop();
 	return err;
 }
 
@@ -1582,10 +1612,8 @@ struct compat_loop_info {
 	struct loop_info64 info64;
 	int err;
 
-	if (!arg) {
-		mutex_unlock(&loop_ctl_mutex);
+	if (!arg)
 		return -EINVAL;
-	}
 	err = loop_get_status(lo, &info64);
 	if (!err)
 		err = loop_info64_to_compat(&info64, arg);
@@ -1600,20 +1628,16 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 
 	switch(cmd) {
 	case LOOP_SET_STATUS:
-		err = mutex_lock_killable(&loop_ctl_mutex);
-		if (!err) {
+		err = lock_loop_killable();
+		if (!err)
 			err = loop_set_status_compat(lo,
 						     (const struct compat_loop_info __user *)arg);
-			mutex_unlock(&loop_ctl_mutex);
-		}
 		break;
 	case LOOP_GET_STATUS:
-		err = mutex_lock_killable(&loop_ctl_mutex);
-		if (!err) {
+		err = lock_loop_killable();
+		if (!err)
 			err = loop_get_status_compat(lo,
 						     (struct compat_loop_info __user *)arg);
-			/* loop_get_status() unlocks loop_ctl_mutex */
-		}
 		break;
 	case LOOP_SET_CAPACITY:
 	case LOOP_CLR_FD:
@@ -1630,6 +1654,7 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 		err = -ENOIOCTLCMD;
 		break;
 	}
+	unlock_loop();
 	return err;
 }
 #endif
@@ -1637,37 +1662,31 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 static int lo_open(struct block_device *bdev, fmode_t mode)
 {
 	struct loop_device *lo;
-	int err = 0;
+	int err = lock_loop_killable();
 
-	mutex_lock(&loop_index_mutex);
+	if (err)
+		return err;
 	lo = bdev->bd_disk->private_data;
-	if (!lo) {
+	if (!lo)
 		err = -ENXIO;
-		goto out;
-	}
-
-	atomic_inc(&lo->lo_refcnt);
-out:
-	mutex_unlock(&loop_index_mutex);
+	else
+		atomic_inc(&lo->lo_refcnt);
+	unlock_loop();
 	return err;
 }
 
 static void __lo_release(struct loop_device *lo)
 {
-	int err;
-
 	if (atomic_dec_return(&lo->lo_refcnt))
 		return;
 
-	mutex_lock(&loop_ctl_mutex);
+	lockdep_assert_held(&loop_mutex);
 	if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
 		/*
 		 * In autoclear mode, stop the loop thread
 		 * and remove configuration after last close.
 		 */
-		err = loop_clr_fd(lo);
-		if (!err)
-			return;
+		loop_clr_fd(lo);
 	} else if (lo->lo_state == Lo_bound) {
 		/*
 		 * Otherwise keep thread (if running) and config,
@@ -1676,15 +1695,13 @@ static void __lo_release(struct loop_device *lo)
 		blk_mq_freeze_queue(lo->lo_queue);
 		blk_mq_unfreeze_queue(lo->lo_queue);
 	}
-
-	mutex_unlock(&loop_ctl_mutex);
 }
 
 static void lo_release(struct gendisk *disk, fmode_t mode)
 {
-	mutex_lock(&loop_index_mutex);
+	lock_loop();
 	__lo_release(disk->private_data);
-	mutex_unlock(&loop_index_mutex);
+	unlock_loop();
 }
 
 static const struct block_device_operations lo_fops = {
@@ -1723,10 +1740,8 @@ static int unregister_transfer_cb(int id, void *ptr, void *data)
 	struct loop_device *lo = ptr;
 	struct loop_func_table *xfer = data;
 
-	mutex_lock(&loop_ctl_mutex);
 	if (lo->lo_encryption == xfer)
 		loop_release_xfer(lo);
-	mutex_unlock(&loop_ctl_mutex);
 	return 0;
 }
 
@@ -1738,8 +1753,14 @@ int loop_unregister_transfer(int number)
 	if (n == 0 || n >= MAX_LO_CRYPT || (xfer = xfer_funcs[n]) == NULL)
 		return -EINVAL;
 
+	/*
+	 * cleanup_cryptoloop() cannot handle errors because it is called
+	 * from module_exit(). Thus, don't give up upon SIGKILL here.
+	 */
+	lock_loop();
 	xfer_funcs[n] = NULL;
 	idr_for_each(&loop_index_idr, &unregister_transfer_cb, xfer);
+	unlock_loop();
 	return 0;
 }
 
@@ -1982,20 +2003,18 @@ static int loop_lookup(struct loop_device **l, int i)
 static struct kobject *loop_probe(dev_t dev, int *part, void *data)
 {
 	struct loop_device *lo;
-	struct kobject *kobj;
-	int err;
+	struct kobject *kobj = NULL;
+	int err = lock_loop_killable();
 
-	mutex_lock(&loop_index_mutex);
+	*part = 0;
+	if (err)
+		return NULL;
 	err = loop_lookup(&lo, MINOR(dev) >> part_shift);
 	if (err < 0)
 		err = loop_add(&lo, MINOR(dev) >> part_shift);
-	if (err < 0)
-		kobj = NULL;
-	else
+	if (err >= 0)
 		kobj = get_disk_and_module(lo->lo_disk);
-	mutex_unlock(&loop_index_mutex);
-
-	*part = 0;
+	unlock_loop();
 	return kobj;
 }
 
@@ -2003,9 +2022,11 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 			       unsigned long parm)
 {
 	struct loop_device *lo;
-	int ret = -ENOSYS;
+	int ret = lock_loop_killable();
 
-	mutex_lock(&loop_index_mutex);
+	if (ret)
+		return ret;
+	ret = -ENOSYS;
 	switch (cmd) {
 	case LOOP_CTL_ADD:
 		ret = loop_lookup(&lo, parm);
@@ -2019,21 +2040,15 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 		ret = loop_lookup(&lo, parm);
 		if (ret < 0)
 			break;
-		ret = mutex_lock_killable(&loop_ctl_mutex);
-		if (ret)
-			break;
 		if (lo->lo_state != Lo_unbound) {
 			ret = -EBUSY;
-			mutex_unlock(&loop_ctl_mutex);
 			break;
 		}
 		if (atomic_read(&lo->lo_refcnt) > 0) {
 			ret = -EBUSY;
-			mutex_unlock(&loop_ctl_mutex);
 			break;
 		}
 		lo->lo_disk->private_data = NULL;
-		mutex_unlock(&loop_ctl_mutex);
 		idr_remove(&loop_index_idr, lo->lo_number);
 		loop_remove(lo);
 		break;
@@ -2043,7 +2058,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 			break;
 		ret = loop_add(&lo, -1);
 	}
-	mutex_unlock(&loop_index_mutex);
+	unlock_loop();
 
 	return ret;
 }
@@ -2127,10 +2142,10 @@ static int __init loop_init(void)
 				  THIS_MODULE, loop_probe, NULL, NULL);
 
 	/* pre-create number of devices given by config or max_loop */
-	mutex_lock(&loop_index_mutex);
+	lock_loop();
 	for (i = 0; i < nr; i++)
 		loop_add(&lo, i);
-	mutex_unlock(&loop_index_mutex);
+	unlock_loop();
 
 	printk(KERN_INFO "loop: module loaded\n");
 	return 0;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation.
  2018-09-25 15:26 [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation Tetsuo Handa
                   ` (2 preceding siblings ...)
  2018-09-25 15:26 ` [PATCH 4/4] block/loop: Fix circular locking dependency at blkdev_reread_part() Tetsuo Handa
@ 2018-09-25 18:59 ` Omar Sandoval
  3 siblings, 0 replies; 8+ messages in thread
From: Omar Sandoval @ 2018-09-25 18:59 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: Jens Axboe, linux-block, Jan Kara

On Wed, Sep 26, 2018 at 12:26:46AM +0900, Tetsuo Handa wrote:
> vfs_getattr() needs "struct path" rather than "struct file".
> Let's use path_get()/path_put() rather than get_file()/fput().

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  drivers/block/loop.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index abad6d1..c2745e6 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1206,7 +1206,7 @@ static int loop_clr_fd(struct loop_device *lo)
>  static int
>  loop_get_status(struct loop_device *lo, struct loop_info64 *info)
>  {
> -	struct file *file;
> +	struct path path;
>  	struct kstat stat;
>  	int ret;
>  
> @@ -1231,16 +1231,16 @@ static int loop_clr_fd(struct loop_device *lo)
>  	}
>  
>  	/* Drop lo_ctl_mutex while we call into the filesystem. */
> -	file = get_file(lo->lo_backing_file);
> +	path = lo->lo_backing_file->f_path;
> +	path_get(&path);
>  	mutex_unlock(&lo->lo_ctl_mutex);
> -	ret = vfs_getattr(&file->f_path, &stat, STATX_INO,
> -			  AT_STATX_SYNC_AS_STAT);
> +	ret = vfs_getattr(&path, &stat, STATX_INO, AT_STATX_SYNC_AS_STAT);
>  	if (!ret) {
>  		info->lo_device = huge_encode_dev(stat.dev);
>  		info->lo_inode = stat.ino;
>  		info->lo_rdevice = huge_encode_dev(stat.rdev);
>  	}
> -	fput(file);
> +	path_put(&path);
>  	return ret;
>  }
>  
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] block/loop: Fix circular locking dependency at blkdev_reread_part().
  2018-09-25 15:26 ` [PATCH 4/4] block/loop: Fix circular locking dependency at blkdev_reread_part() Tetsuo Handa
@ 2018-09-27 11:27   ` Jan Kara
  2018-09-27 11:35     ` Tetsuo Handa
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Kara @ 2018-09-27 11:27 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: Jens Axboe, linux-block, Jan Kara, syzbot

Hi,

On Wed 26-09-18 00:26:49, Tetsuo Handa wrote:
> syzbot is reporting circular locking dependency between bdev->bd_mutex
> and lo->lo_ctl_mutex [1] which is caused by calling blkdev_reread_part()
> with lock held. We need to drop lo->lo_ctl_mutex in order to fix it.
> 
> This patch fixes it by combining loop_index_mutex and loop_ctl_mutex into
> loop_mutex, and releasing loop_mutex before calling blkdev_reread_part()
> or fput() or path_put() or leaving ioctl().
> 
> The rule is that current thread calls lock_loop() before accessing
> "struct loop_device", and current thread no longer accesses "struct
> loop_device" after unlock_loop() is called.
> 
> Since syzbot is reporting various bugs [2] where a race in the loop module
> is suspected, let's check whether this patch affects these bugs too.
> 
> [1] https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889
> [2] https://syzkaller.appspot.com/bug?id=b3c7e1440aa8ece16bf557dbac427fdff1dad9d6
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzbot+4684a000d5abdade83fac55b1e7d1f935ef1936e@syzkaller.appspotmail.com>
> ---
>  drivers/block/loop.c | 187 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 101 insertions(+), 86 deletions(-)

I still don't like this patch. I'll post a patch series showing what I have
in mind. Admittedly, it's a bit tedious but the locking is much saner
afterwards...

								Honza

> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 4b05a27..04389bb 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -84,12 +84,50 @@
>  #include <linux/uaccess.h>
>  
>  static DEFINE_IDR(loop_index_idr);
> -static DEFINE_MUTEX(loop_index_mutex);
> -static DEFINE_MUTEX(loop_ctl_mutex);
> +static DEFINE_MUTEX(loop_mutex);
> +static void *loop_mutex_owner; /* == __mutex_owner(&loop_mutex) */
>  
>  static int max_part;
>  static int part_shift;
>  
> +/*
> + * lock_loop - Lock loop_mutex.
> + */
> +static void lock_loop(void)
> +{
> +	mutex_lock(&loop_mutex);
> +	loop_mutex_owner = current;
> +}
> +
> +/*
> + * lock_loop_killable - Lock loop_mutex unless killed.
> + */
> +static int lock_loop_killable(void)
> +{
> +	int err = mutex_lock_killable(&loop_mutex);
> +
> +	if (err)
> +		return err;
> +	loop_mutex_owner = current;
> +	return 0;
> +}
> +
> +/*
> + * unlock_loop - Unlock loop_mutex as needed.
> + *
> + * Explicitly call this function before calling fput() or blkdev_reread_part()
> + * in order to avoid circular lock dependency. After this function is called,
> + * current thread is no longer allowed to access "struct loop_device" memory,
> + * for another thread would access that memory as soon as loop_mutex is held.
> + */
> +static void unlock_loop(void)
> +{
> +	if (loop_mutex_owner == current) {
> +		loop_mutex_owner = NULL;
> +		mutex_unlock(&loop_mutex);
> +	}
> +}
> +
>  static int transfer_xor(struct loop_device *lo, int cmd,
>  			struct page *raw_page, unsigned raw_off,
>  			struct page *loop_page, unsigned loop_off,
> @@ -637,6 +675,7 @@ static void loop_reread_partitions(struct loop_device *lo,
>  	const int count = atomic_read(&lo->lo_refcnt);
>  
>  	memcpy(filename, lo->lo_file_name, sizeof(filename));
> +	unlock_loop();
>  
>  	/*
>  	 * bd_mutex has been held already in release path, so don't
> @@ -699,6 +738,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>  	struct file	*file, *old_file;
>  	int		error;
>  
> +	lockdep_assert_held(&loop_mutex);
>  	error = -ENXIO;
>  	if (lo->lo_state != Lo_bound)
>  		goto out;
> @@ -737,10 +777,12 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>  
>  	if (lo->lo_flags & LO_FLAGS_PARTSCAN)
>  		loop_reread_partitions(lo, bdev);
> +	unlock_loop();
>  	fput(old_file);
>  	return 0;
>  
>   out_putf:
> +	unlock_loop();
>  	fput(file);
>   out:
>  	return error;
> @@ -918,6 +960,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	int		error;
>  	loff_t		size;
>  
> +	lockdep_assert_held(&loop_mutex);
>  	/* This is safe, since we have a reference from open(). */
>  	__module_get(THIS_MODULE);
>  
> @@ -991,6 +1034,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	return 0;
>  
>   out_putf:
> +	unlock_loop();
>  	fput(file);
>   out:
>  	/* This is safe: open() is still holding a reference. */
> @@ -1042,6 +1086,7 @@ static int loop_clr_fd(struct loop_device *lo)
>  	struct block_device *bdev = lo->lo_device;
>  	bool reread;
>  
> +	lockdep_assert_held(&loop_mutex);
>  	if (lo->lo_state != Lo_bound)
>  		return -ENXIO;
>  
> @@ -1057,7 +1102,6 @@ static int loop_clr_fd(struct loop_device *lo)
>  	 */
>  	if (atomic_read(&lo->lo_refcnt) > 1) {
>  		lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
> -		mutex_unlock(&loop_ctl_mutex);
>  		return 0;
>  	}
>  
> @@ -1111,13 +1155,7 @@ static int loop_clr_fd(struct loop_device *lo)
>  		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
>  	if (reread)
>  		loop_reread_partitions(lo, bdev);
> -	mutex_unlock(&loop_ctl_mutex);
> -	/*
> -	 * Need not hold loop_ctl_mutex to fput backing file.
> -	 * Calling fput holding loop_ctl_mutex triggers a circular
> -	 * lock dependency possibility warning as fput can take
> -	 * bd_mutex which is usually taken before loop_ctl_mutex.
> -	 */
> +	unlock_loop();
>  	fput(filp);
>  	return 0;
>  }
> @@ -1129,6 +1167,7 @@ static int loop_clr_fd(struct loop_device *lo)
>  	struct loop_func_table *xfer;
>  	kuid_t uid = current_uid();
>  
> +	lockdep_assert_held(&loop_mutex);
>  	if (lo->lo_encrypt_key_size &&
>  	    !uid_eq(lo->lo_key_owner, uid) &&
>  	    !capable(CAP_SYS_ADMIN))
> @@ -1220,10 +1259,9 @@ static int loop_clr_fd(struct loop_device *lo)
>  	struct kstat stat;
>  	int ret;
>  
> -	if (lo->lo_state != Lo_bound) {
> -		mutex_unlock(&loop_ctl_mutex);
> +	lockdep_assert_held(&loop_mutex);
> +	if (lo->lo_state != Lo_bound)
>  		return -ENXIO;
> -	}
>  
>  	memset(info, 0, sizeof(*info));
>  	info->lo_number = lo->lo_number;
> @@ -1240,10 +1278,10 @@ static int loop_clr_fd(struct loop_device *lo)
>  		       lo->lo_encrypt_key_size);
>  	}
>  
> -	/* Drop loop_ctl_mutex while we call into the filesystem. */
> +	/* Drop loop_mutex while we call into the filesystem. */
>  	path = lo->lo_backing_file->f_path;
>  	path_get(&path);
> -	mutex_unlock(&loop_ctl_mutex);
> +	unlock_loop();
>  	ret = vfs_getattr(&path, &stat, STATX_INO, AT_STATX_SYNC_AS_STAT);
>  	if (!ret) {
>  		info->lo_device = huge_encode_dev(stat.dev);
> @@ -1334,10 +1372,8 @@ static int loop_clr_fd(struct loop_device *lo)
>  	struct loop_info64 info64;
>  	int err;
>  
> -	if (!arg) {
> -		mutex_unlock(&loop_ctl_mutex);
> +	if (!arg)
>  		return -EINVAL;
> -	}
>  	err = loop_get_status(lo, &info64);
>  	if (!err)
>  		err = loop_info64_to_old(&info64, &info);
> @@ -1352,10 +1388,8 @@ static int loop_clr_fd(struct loop_device *lo)
>  	struct loop_info64 info64;
>  	int err;
>  
> -	if (!arg) {
> -		mutex_unlock(&loop_ctl_mutex);
> +	if (!arg)
>  		return -EINVAL;
> -	}
>  	err = loop_get_status(lo, &info64);
>  	if (!err && copy_to_user(arg, &info64, sizeof(info64)))
>  		err = -EFAULT;
> @@ -1365,6 +1399,7 @@ static int loop_clr_fd(struct loop_device *lo)
>  
>  static int loop_set_capacity(struct loop_device *lo)
>  {
> +	lockdep_assert_held(&loop_mutex);
>  	if (unlikely(lo->lo_state != Lo_bound))
>  		return -ENXIO;
>  
> @@ -1374,6 +1409,8 @@ static int loop_set_capacity(struct loop_device *lo)
>  static int loop_set_dio(struct loop_device *lo, unsigned long arg)
>  {
>  	int error = -ENXIO;
> +
> +	lockdep_assert_held(&loop_mutex);
>  	if (lo->lo_state != Lo_bound)
>  		goto out;
>  
> @@ -1387,6 +1424,7 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
>  
>  static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
>  {
> +	lockdep_assert_held(&loop_mutex);
>  	if (lo->lo_state != Lo_bound)
>  		return -ENXIO;
>  
> @@ -1409,11 +1447,10 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  	unsigned int cmd, unsigned long arg)
>  {
>  	struct loop_device *lo = bdev->bd_disk->private_data;
> -	int err;
> +	int err = lock_loop_killable();
>  
> -	err = mutex_lock_killable_nested(&loop_ctl_mutex, 1);
>  	if (err)
> -		goto out_unlocked;
> +		return err;
>  
>  	switch (cmd) {
>  	case LOOP_SET_FD:
> @@ -1423,10 +1460,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  		err = loop_change_fd(lo, bdev, arg);
>  		break;
>  	case LOOP_CLR_FD:
> -		/* loop_clr_fd would have unlocked loop_ctl_mutex on success */
>  		err = loop_clr_fd(lo);
> -		if (!err)
> -			goto out_unlocked;
>  		break;
>  	case LOOP_SET_STATUS:
>  		err = -EPERM;
> @@ -1436,8 +1470,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  		break;
>  	case LOOP_GET_STATUS:
>  		err = loop_get_status_old(lo, (struct loop_info __user *) arg);
> -		/* loop_get_status() unlocks loop_ctl_mutex */
> -		goto out_unlocked;
> +		break;
>  	case LOOP_SET_STATUS64:
>  		err = -EPERM;
>  		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> @@ -1446,8 +1479,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  		break;
>  	case LOOP_GET_STATUS64:
>  		err = loop_get_status64(lo, (struct loop_info64 __user *) arg);
> -		/* loop_get_status() unlocks loop_ctl_mutex */
> -		goto out_unlocked;
> +		break;
>  	case LOOP_SET_CAPACITY:
>  		err = -EPERM;
>  		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> @@ -1466,9 +1498,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  	default:
>  		err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
>  	}
> -	mutex_unlock(&loop_ctl_mutex);
> -
> -out_unlocked:
> +	unlock_loop();
>  	return err;
>  }
>  
> @@ -1582,10 +1612,8 @@ struct compat_loop_info {
>  	struct loop_info64 info64;
>  	int err;
>  
> -	if (!arg) {
> -		mutex_unlock(&loop_ctl_mutex);
> +	if (!arg)
>  		return -EINVAL;
> -	}
>  	err = loop_get_status(lo, &info64);
>  	if (!err)
>  		err = loop_info64_to_compat(&info64, arg);
> @@ -1600,20 +1628,16 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
>  
>  	switch(cmd) {
>  	case LOOP_SET_STATUS:
> -		err = mutex_lock_killable(&loop_ctl_mutex);
> -		if (!err) {
> +		err = lock_loop_killable();
> +		if (!err)
>  			err = loop_set_status_compat(lo,
>  						     (const struct compat_loop_info __user *)arg);
> -			mutex_unlock(&loop_ctl_mutex);
> -		}
>  		break;
>  	case LOOP_GET_STATUS:
> -		err = mutex_lock_killable(&loop_ctl_mutex);
> -		if (!err) {
> +		err = lock_loop_killable();
> +		if (!err)
>  			err = loop_get_status_compat(lo,
>  						     (struct compat_loop_info __user *)arg);
> -			/* loop_get_status() unlocks loop_ctl_mutex */
> -		}
>  		break;
>  	case LOOP_SET_CAPACITY:
>  	case LOOP_CLR_FD:
> @@ -1630,6 +1654,7 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
>  		err = -ENOIOCTLCMD;
>  		break;
>  	}
> +	unlock_loop();
>  	return err;
>  }
>  #endif
> @@ -1637,37 +1662,31 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
>  static int lo_open(struct block_device *bdev, fmode_t mode)
>  {
>  	struct loop_device *lo;
> -	int err = 0;
> +	int err = lock_loop_killable();
>  
> -	mutex_lock(&loop_index_mutex);
> +	if (err)
> +		return err;
>  	lo = bdev->bd_disk->private_data;
> -	if (!lo) {
> +	if (!lo)
>  		err = -ENXIO;
> -		goto out;
> -	}
> -
> -	atomic_inc(&lo->lo_refcnt);
> -out:
> -	mutex_unlock(&loop_index_mutex);
> +	else
> +		atomic_inc(&lo->lo_refcnt);
> +	unlock_loop();
>  	return err;
>  }
>  
>  static void __lo_release(struct loop_device *lo)
>  {
> -	int err;
> -
>  	if (atomic_dec_return(&lo->lo_refcnt))
>  		return;
>  
> -	mutex_lock(&loop_ctl_mutex);
> +	lockdep_assert_held(&loop_mutex);
>  	if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
>  		/*
>  		 * In autoclear mode, stop the loop thread
>  		 * and remove configuration after last close.
>  		 */
> -		err = loop_clr_fd(lo);
> -		if (!err)
> -			return;
> +		loop_clr_fd(lo);
>  	} else if (lo->lo_state == Lo_bound) {
>  		/*
>  		 * Otherwise keep thread (if running) and config,
> @@ -1676,15 +1695,13 @@ static void __lo_release(struct loop_device *lo)
>  		blk_mq_freeze_queue(lo->lo_queue);
>  		blk_mq_unfreeze_queue(lo->lo_queue);
>  	}
> -
> -	mutex_unlock(&loop_ctl_mutex);
>  }
>  
>  static void lo_release(struct gendisk *disk, fmode_t mode)
>  {
> -	mutex_lock(&loop_index_mutex);
> +	lock_loop();
>  	__lo_release(disk->private_data);
> -	mutex_unlock(&loop_index_mutex);
> +	unlock_loop();
>  }
>  
>  static const struct block_device_operations lo_fops = {
> @@ -1723,10 +1740,8 @@ static int unregister_transfer_cb(int id, void *ptr, void *data)
>  	struct loop_device *lo = ptr;
>  	struct loop_func_table *xfer = data;
>  
> -	mutex_lock(&loop_ctl_mutex);
>  	if (lo->lo_encryption == xfer)
>  		loop_release_xfer(lo);
> -	mutex_unlock(&loop_ctl_mutex);
>  	return 0;
>  }
>  
> @@ -1738,8 +1753,14 @@ int loop_unregister_transfer(int number)
>  	if (n == 0 || n >= MAX_LO_CRYPT || (xfer = xfer_funcs[n]) == NULL)
>  		return -EINVAL;
>  
> +	/*
> +	 * cleanup_cryptoloop() cannot handle errors because it is called
> +	 * from module_exit(). Thus, don't give up upon SIGKILL here.
> +	 */
> +	lock_loop();
>  	xfer_funcs[n] = NULL;
>  	idr_for_each(&loop_index_idr, &unregister_transfer_cb, xfer);
> +	unlock_loop();
>  	return 0;
>  }
>  
> @@ -1982,20 +2003,18 @@ static int loop_lookup(struct loop_device **l, int i)
>  static struct kobject *loop_probe(dev_t dev, int *part, void *data)
>  {
>  	struct loop_device *lo;
> -	struct kobject *kobj;
> -	int err;
> +	struct kobject *kobj = NULL;
> +	int err = lock_loop_killable();
>  
> -	mutex_lock(&loop_index_mutex);
> +	*part = 0;
> +	if (err)
> +		return NULL;
>  	err = loop_lookup(&lo, MINOR(dev) >> part_shift);
>  	if (err < 0)
>  		err = loop_add(&lo, MINOR(dev) >> part_shift);
> -	if (err < 0)
> -		kobj = NULL;
> -	else
> +	if (err >= 0)
>  		kobj = get_disk_and_module(lo->lo_disk);
> -	mutex_unlock(&loop_index_mutex);
> -
> -	*part = 0;
> +	unlock_loop();
>  	return kobj;
>  }
>  
> @@ -2003,9 +2022,11 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
>  			       unsigned long parm)
>  {
>  	struct loop_device *lo;
> -	int ret = -ENOSYS;
> +	int ret = lock_loop_killable();
>  
> -	mutex_lock(&loop_index_mutex);
> +	if (ret)
> +		return ret;
> +	ret = -ENOSYS;
>  	switch (cmd) {
>  	case LOOP_CTL_ADD:
>  		ret = loop_lookup(&lo, parm);
> @@ -2019,21 +2040,15 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
>  		ret = loop_lookup(&lo, parm);
>  		if (ret < 0)
>  			break;
> -		ret = mutex_lock_killable(&loop_ctl_mutex);
> -		if (ret)
> -			break;
>  		if (lo->lo_state != Lo_unbound) {
>  			ret = -EBUSY;
> -			mutex_unlock(&loop_ctl_mutex);
>  			break;
>  		}
>  		if (atomic_read(&lo->lo_refcnt) > 0) {
>  			ret = -EBUSY;
> -			mutex_unlock(&loop_ctl_mutex);
>  			break;
>  		}
>  		lo->lo_disk->private_data = NULL;
> -		mutex_unlock(&loop_ctl_mutex);
>  		idr_remove(&loop_index_idr, lo->lo_number);
>  		loop_remove(lo);
>  		break;
> @@ -2043,7 +2058,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
>  			break;
>  		ret = loop_add(&lo, -1);
>  	}
> -	mutex_unlock(&loop_index_mutex);
> +	unlock_loop();
>  
>  	return ret;
>  }
> @@ -2127,10 +2142,10 @@ static int __init loop_init(void)
>  				  THIS_MODULE, loop_probe, NULL, NULL);
>  
>  	/* pre-create number of devices given by config or max_loop */
> -	mutex_lock(&loop_index_mutex);
> +	lock_loop();
>  	for (i = 0; i < nr; i++)
>  		loop_add(&lo, i);
> -	mutex_unlock(&loop_index_mutex);
> +	unlock_loop();
>  
>  	printk(KERN_INFO "loop: module loaded\n");
>  	return 0;
> -- 
> 1.8.3.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] block/loop: Fix circular locking dependency at blkdev_reread_part().
  2018-09-27 11:27   ` Jan Kara
@ 2018-09-27 11:35     ` Tetsuo Handa
  2018-09-27 11:42       ` Jan Kara
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuo Handa @ 2018-09-27 11:35 UTC (permalink / raw)
  To: Jan Kara; +Cc: Jens Axboe, linux-block, syzbot

On 2018/09/27 20:27, Jan Kara wrote:
> Hi,
> 
> On Wed 26-09-18 00:26:49, Tetsuo Handa wrote:
>> syzbot is reporting circular locking dependency between bdev->bd_mutex
>> and lo->lo_ctl_mutex [1] which is caused by calling blkdev_reread_part()
>> with lock held. We need to drop lo->lo_ctl_mutex in order to fix it.
>>
>> This patch fixes it by combining loop_index_mutex and loop_ctl_mutex into
>> loop_mutex, and releasing loop_mutex before calling blkdev_reread_part()
>> or fput() or path_put() or leaving ioctl().
>>
>> The rule is that current thread calls lock_loop() before accessing
>> "struct loop_device", and current thread no longer accesses "struct
>> loop_device" after unlock_loop() is called.
>>
>> Since syzbot is reporting various bugs [2] where a race in the loop module
>> is suspected, let's check whether this patch affects these bugs too.
>>
>> [1] https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889
>> [2] https://syzkaller.appspot.com/bug?id=b3c7e1440aa8ece16bf557dbac427fdff1dad9d6
>>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Reported-by: syzbot <syzbot+4684a000d5abdade83fac55b1e7d1f935ef1936e@syzkaller.appspotmail.com>
>> ---
>>  drivers/block/loop.c | 187 ++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 101 insertions(+), 86 deletions(-)
> 
> I still don't like this patch. I'll post a patch series showing what I have
> in mind. Admittedly, it's a bit tedious but the locking is much saner
> afterwards...

Please be sure to Cc: me. I'm not subscribed to linux-block ML.

But if we have to release lo_ctl_mutex before calling blkdev_reread_part(),
what is nice with re-acquiring lo_ctl_mutex after blkdev_reread_part() ?

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] block/loop: Fix circular locking dependency at blkdev_reread_part().
  2018-09-27 11:35     ` Tetsuo Handa
@ 2018-09-27 11:42       ` Jan Kara
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Kara @ 2018-09-27 11:42 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: Jan Kara, Jens Axboe, linux-block, syzbot

On Thu 27-09-18 20:35:27, Tetsuo Handa wrote:
> On 2018/09/27 20:27, Jan Kara wrote:
> > Hi,
> > 
> > On Wed 26-09-18 00:26:49, Tetsuo Handa wrote:
> >> syzbot is reporting circular locking dependency between bdev->bd_mutex
> >> and lo->lo_ctl_mutex [1] which is caused by calling blkdev_reread_part()
> >> with lock held. We need to drop lo->lo_ctl_mutex in order to fix it.
> >>
> >> This patch fixes it by combining loop_index_mutex and loop_ctl_mutex into
> >> loop_mutex, and releasing loop_mutex before calling blkdev_reread_part()
> >> or fput() or path_put() or leaving ioctl().
> >>
> >> The rule is that current thread calls lock_loop() before accessing
> >> "struct loop_device", and current thread no longer accesses "struct
> >> loop_device" after unlock_loop() is called.
> >>
> >> Since syzbot is reporting various bugs [2] where a race in the loop module
> >> is suspected, let's check whether this patch affects these bugs too.
> >>
> >> [1] https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889
> >> [2] https://syzkaller.appspot.com/bug?id=b3c7e1440aa8ece16bf557dbac427fdff1dad9d6
> >>
> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >> Reported-by: syzbot <syzbot+4684a000d5abdade83fac55b1e7d1f935ef1936e@syzkaller.appspotmail.com>
> >> ---
> >>  drivers/block/loop.c | 187 ++++++++++++++++++++++++++++-----------------------
> >>  1 file changed, 101 insertions(+), 86 deletions(-)
> > 
> > I still don't like this patch. I'll post a patch series showing what I have
> > in mind. Admittedly, it's a bit tedious but the locking is much saner
> > afterwards...
> 
> Please be sure to Cc: me. I'm not subscribed to linux-block ML.

Yes, I've CCed you.

> But if we have to release lo_ctl_mutex before calling blkdev_reread_part(),
> what is nice with re-acquiring lo_ctl_mutex after blkdev_reread_part() ?

We don't reacquire the mutex after blkdev_reread_part(). Just the code
needed to be cleaned up so that loop_reread_part() does not need
lo_ctl_mutex for anything.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-09-27 11:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-25 15:26 [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation Tetsuo Handa
2018-09-25 15:26 ` [PATCH 2/4] block/loop: Use global lock for ioctl() operation Tetsuo Handa
2018-09-25 15:26 ` [PATCH 3/4] block/loop: Reorganize loop_reread_partitions() callers Tetsuo Handa
2018-09-25 15:26 ` [PATCH 4/4] block/loop: Fix circular locking dependency at blkdev_reread_part() Tetsuo Handa
2018-09-27 11:27   ` Jan Kara
2018-09-27 11:35     ` Tetsuo Handa
2018-09-27 11:42       ` Jan Kara
2018-09-25 18:59 ` [PATCH 1/4] block/loop: Don't grab "struct file" for vfs_getattr() operation Omar Sandoval

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.