* [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 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
* 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
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.