[PATCH v3] block/loop: Serialize ioctl operations.

[Tetsuo Handa]

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 without holding corresponding locks.

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.

Since ioctl() request on loop devices is not frequent operation, we don't
need fine grained locking. Let's use global lock and simplify the locking
order.

Strategy is that the global lock is held upon entry of ioctl() request,
and release it before either starting operations which might deadlock or
leaving ioctl() request. After the global lock is released, current thread
no longer uses "struct loop_device" memory because it might be modified
by next ioctl() request which was waiting for current ioctl() request.

In order to enforce this strategy, this patch inversed
loop_reread_partitions() and loop_unprepare_queue() in loop_clr_fd().
I don't know whether it breaks something, but I don't have testcases.

Since this patch serializes using global lock, race bugs should no longer
exist. Thus, it will be easy to test whether this patch broke something.

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

Signed-off-by: Tetsuo Handa 
Reported-by: syzbot 
Reported-by: syzbot 

Cc: Jens Axboe 
---
 drivers/block/loop.c | 231 ---
 drivers/block/loop.h |   1 -
 2 files changed, 128 insertions(+), 104 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 55cf554..feb9fa7 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -81,11 +81,50 @@
 #include 
 
 static DEFINE_IDR(loop_index_idr);
-static DEFINE_MUTEX(loop_index_mutex);
+static DEFINE_MUTEX(loop_mutex);
+static void *loop_mutex_owner; /* == __mutex_owner(_mutex) */
 
 static int max_part;
 static int part_shift;
 
+/*
+ * lock_loop - Lock loop_mutex.
+ */
+static void lock_loop(void)
+{
+   mutex_lock(_mutex);
+   loop_mutex_owner = current;
+}
+
+/*
+ * lock_loop_killable - Lock loop_mutex unless killed.
+ */
+static int lock_loop_killable(void)
+{
+   int err = mutex_lock_killable(_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(_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,
@@ -626,7 +665,12 @@ static void loop_reread_partitions(struct loop_device *lo,
   struct block_device *bdev)
 {
int rc;
+   /* Save variables which might change after unlock_loop() is called. */
+   char filename[sizeof(lo->lo_file_name)];
+   const int num = lo->lo_number;
 
+   memmove(filename, lo->lo_file_name, sizeof(filename));
+   unlock_loop();
/*
 * bd_mutex has been held already in release path, so don't
 * acquire it if this function is called in such case.
@@ -641,7 +685,7 @@ static void loop_reread_partitions(struct loop_device *lo,
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);
 }
 
 /*
@@ -659,6 +703,7 @@ static int loop_change_fd(struct loop_device *lo, struct 
block_device *bdev,
struct inode*inode;
int error;
 
+   lockdep_assert_held(_mutex);
error = -ENXIO;
if (lo->lo_state != Lo_bound)
goto out;
@@ -695,12 +740,14 @@ 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);
+   loop_reread_partitions(lo, bdev); /* calls unlock_loop() */
+ 

[PATCH v3] block/loop: Serialize ioctl operations.

[Tetsuo Handa]

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 without holding corresponding locks.

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.

Since ioctl() request on loop devices is not frequent operation, we don't
need fine grained locking. Let's use global lock and simplify the locking
order.

Strategy is that the global lock is held upon entry of ioctl() request,
and release it before either starting operations which might deadlock or
leaving ioctl() request. After the global lock is released, current thread
no longer uses "struct loop_device" memory because it might be modified
by next ioctl() request which was waiting for current ioctl() request.

In order to enforce this strategy, this patch inversed
loop_reread_partitions() and loop_unprepare_queue() in loop_clr_fd().
I don't know whether it breaks something, but I don't have testcases.

Since this patch serializes using global lock, race bugs should no longer
exist. Thus, it will be easy to test whether this patch broke something.

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

Signed-off-by: Tetsuo Handa 
Reported-by: syzbot 
Reported-by: syzbot 

Cc: Jens Axboe 
---
 drivers/block/loop.c | 231 ---
 drivers/block/loop.h |   1 -
 2 files changed, 128 insertions(+), 104 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 55cf554..feb9fa7 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -81,11 +81,50 @@
 #include 
 
 static DEFINE_IDR(loop_index_idr);
-static DEFINE_MUTEX(loop_index_mutex);
+static DEFINE_MUTEX(loop_mutex);
+static void *loop_mutex_owner; /* == __mutex_owner(_mutex) */
 
 static int max_part;
 static int part_shift;
 
+/*
+ * lock_loop - Lock loop_mutex.
+ */
+static void lock_loop(void)
+{
+   mutex_lock(_mutex);
+   loop_mutex_owner = current;
+}
+
+/*
+ * lock_loop_killable - Lock loop_mutex unless killed.
+ */
+static int lock_loop_killable(void)
+{
+   int err = mutex_lock_killable(_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(_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,
@@ -626,7 +665,12 @@ static void loop_reread_partitions(struct loop_device *lo,
   struct block_device *bdev)
 {
int rc;
+   /* Save variables which might change after unlock_loop() is called. */
+   char filename[sizeof(lo->lo_file_name)];
+   const int num = lo->lo_number;
 
+   memmove(filename, lo->lo_file_name, sizeof(filename));
+   unlock_loop();
/*
 * bd_mutex has been held already in release path, so don't
 * acquire it if this function is called in such case.
@@ -641,7 +685,7 @@ static void loop_reread_partitions(struct loop_device *lo,
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);
 }
 
 /*
@@ -659,6 +703,7 @@ static int loop_change_fd(struct loop_device *lo, struct 
block_device *bdev,
struct inode*inode;
int error;
 
+   lockdep_assert_held(_mutex);
error = -ENXIO;
if (lo->lo_state != Lo_bound)
goto out;
@@ -695,12 +740,14 @@ 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);
+   loop_reread_partitions(lo, bdev); /* calls unlock_loop() */
+   unlock_loop();
+   fput(old_file);
return 0;
 
  out_putf:
+   unlock_loop();
fput(file);
  out:
return error;
@@ -884,6 +931,7 @@ static int