Re: possible deadlock in blkdev_reread_part

[Dmitry Vyukov]

/\/\/\/\/\/\On Fri, Apr 20, 2018 at 4:06 PM, Tetsuo Handa
<penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
> Tetsuo Handa wrote:
>> Eric Biggers wrote:
>> > It seems that ->bd_mutex is held while opening and closing block devices, which
>> > should rank it above both ->lo_ctl_mutex and loop_index_mutex (see lo_open() and
>> > lo_release()).
>> >
>> > But blkdev_reread_part(), which takes ->bd_mutex, is called from some of the
>> > ioctls while ->lo_ctl_mutex is held.
>> >
>> > Perhaps we should call blkdev_reread_part() at the end of the ioctls, after
>> > ->lo_ctl_mutex has been dropped?  But it looks like that can do I/O to the
>> > device, which probably could race with loop_clr_fd()...
>> >
>> > Or perhaps we should just take both locks for the ioctls, in the order
>> > ->bd_mutex, then ->lo_ctl_mutex -- and then use __blkdev_reread_part()?
>>
>> But do we really need ->lo_ctl_mutex ? What is the purpose of ->lo_ctl_mutex ?
>> If the purpose of ->lo_ctl_mutex is to serialize ioctl() request against each
>> loop device when multiple threads opened the same loop device, I feel that
>>
>>   There is no need to use ->lo_ctl_mutex which the lockdep will check and
>>   complain, provided that ioctl() request cannot recurse into ioctl() request.
>>   Instead, we can use simple flag variable whether an ioctl() is in progress.
>>
>>   There is no need to use ->lo_ctl_mutex from __lo_release(), for it is
>>   guaranteed that there is no in-flight ioctl() request when __lo_release()
>>   is called, and loop_index_mutex is blocking further open() requests.
>>
>> and simplify like below.
>>
> Can we test this patch?
>
> >From 0ca0694b74ca4b02e9d2e3f46c9d960ba167adec Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Date: Fri, 20 Apr 2018 22:54:42 +0900
> Subject: [PATCH] block/loop: Serialize ioctl operations.
>
> syzbot is reporting various bugs which involve /dev/loopX.
> Two of them
>
>   INFO: rcu detected stall in lo_ioctl
>   https://syzkaller.appspot.com/bug?id=7b49fb610af9cca78c24e9f796f2e8b0d5573997
>
>   general protection fault in lo_ioctl (2)
>   https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3

/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\

Now there is a repro for this one. I've pushed it to kernel mailing lists:

https://groups.google.com/d/msg/syzkaller-bugs/c8KUcTAzTvA/3o_7g6-tAwAJ

> suggest that loop module is not thread safe. The former suggests that
> l->lo_backing_file is forming circular loop and the latter suggests that
> l->lo_backing_file became NULL.
>
> In fact, recursion avoidance check in loop_set_fd() is traversing loop
> devices without holding each lo->lo_ctl_mutex lock. lo_state can become
> Lo_rundown and lo_backing_file can become NULL if raced with loop_clr_fd().
>
>   /* Avoid recursion */
>   f = file;
>   while (is_loop_device(f)) {
>     struct loop_device *l;
>
>     if (f->f_mapping->host->i_bdev == bdev)
>       goto out_putf;
>
>     l = f->f_mapping->host->i_bdev->bd_disk->private_data;
>     if (l->lo_state == Lo_unbound) {
>       error = -EINVAL;
>       goto out_putf;
>     }
>     f = l->lo_backing_file;
>   }
>
> Since ioctl() request on loop devices is not frequent operation, we don't
> need fine grained locking. Let's use global lock rather than dancing with
> locking order inside this recursion avoidance check.
>
> Strategy is that the global lock is held upon entry of ioctl() request,
> and release it before either starting operations which might recurse 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.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Reported-by: syzbot <syzbot+e5e426c6a47c26ada908348fe84aff916a26d337@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Reported-by: syzbot <syzbot+116e7de10c6752bf5e1e@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Cc: Jens Axboe <axboe@xxxxxxxxx>
> ---
>  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 c9d0449..59716d1 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -81,11 +81,50 @@
>  #include <linux/uaccess.h>
>
>  static DEFINE_IDR(loop_index_idr);
> -static DEFINE_MUTEX(loop_index_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,
> @@ -604,7 +643,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.
> @@ -619,7 +663,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);
>  }
>
>  /*
> @@ -637,6 +681,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>         struct inode    *inode;
>         int             error;
>
> +       lockdep_assert_held(&loop_mutex);
>         error = -ENXIO;
>         if (lo->lo_state != Lo_bound)
>                 goto out;
> @@ -673,12 +718,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;
> @@ -862,6 +909,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);
>
> @@ -936,19 +984,21 @@ 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);
> +               loop_reread_partitions(lo, bdev); /* calls unlock_loop() */
>         return 0;
>
>   out_putf:
> +       unlock_loop();
>         fput(file);
>   out:
>         /* This is safe: open() is still holding a reference. */
> @@ -998,7 +1048,9 @@ 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;
>
> +       lockdep_assert_held(&loop_mutex);
>         if (lo->lo_state != Lo_bound)
>                 return -ENXIO;
>
> @@ -1014,7 +1066,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(&lo->lo_ctl_mutex);
>                 return 0;
>         }
>
> @@ -1059,20 +1110,14 @@ static int loop_clr_fd(struct loop_device *lo)
>         /* This is safe: open() is still holding a reference. */
>         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;
>         lo->lo_flags = 0;
>         if (!part_shift)
>                 lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
>         loop_unprepare_queue(lo);
> -       mutex_unlock(&lo->lo_ctl_mutex);
> -       /*
> -        * Need not hold lo_ctl_mutex to fput backing file.
> -        * Calling fput holding lo_ctl_mutex triggers a circular
> -        * lock dependency possibility warning as fput can take
> -        * bd_mutex which is usually taken before lo_ctl_mutex.
> -        */
> +       if (reread)
> +               loop_reread_partitions(lo, bdev); /* calls unlock_loop() */
> +       unlock_loop();
>         fput(filp);
>         return 0;
>  }
> @@ -1162,7 +1207,7 @@ static int loop_clr_fd(struct loop_device *lo)
>              !(lo->lo_flags & LO_FLAGS_PARTSCAN)) {
>                 lo->lo_flags |= LO_FLAGS_PARTSCAN;
>                 lo->lo_disk->flags &= ~GENHD_FL_NO_PART_SCAN;
> -               loop_reread_partitions(lo, lo->lo_device);
> +               loop_reread_partitions(lo, lo->lo_device); /* calls unlock_loop() */
>         }
>
>         return err;
> @@ -1171,14 +1216,13 @@ 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;
>
> -       if (lo->lo_state != Lo_bound) {
> -               mutex_unlock(&lo->lo_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;
> @@ -1195,17 +1239,17 @@ static int loop_clr_fd(struct loop_device *lo)
>                        lo->lo_encrypt_key_size);
>         }
>
> -       /* Drop lo_ctl_mutex while we call into the filesystem. */
> -       file = get_file(lo->lo_backing_file);
> -       mutex_unlock(&lo->lo_ctl_mutex);
> -       ret = vfs_getattr(&file->f_path, &stat, STATX_INO,
> -                         AT_STATX_SYNC_AS_STAT);
> +       /* Drop loop_mutex while we call into the filesystem. */
> +       path = lo->lo_backing_file->f_path;
> +       path_get(&path);
> +       unlock_loop();
> +       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;
>  }
>
> @@ -1267,6 +1311,7 @@ static int loop_clr_fd(struct loop_device *lo)
>         struct loop_info info;
>         struct loop_info64 info64;
>
> +       lockdep_assert_held(&loop_mutex);
>         if (copy_from_user(&info, arg, sizeof (struct loop_info)))
>                 return -EFAULT;
>         loop_info64_from_old(&info, &info64);
> @@ -1278,6 +1323,7 @@ static int loop_clr_fd(struct loop_device *lo)
>  {
>         struct loop_info64 info64;
>
> +       lockdep_assert_held(&loop_mutex);
>         if (copy_from_user(&info64, arg, sizeof (struct loop_info64)))
>                 return -EFAULT;
>         return loop_set_status(lo, &info64);
> @@ -1289,10 +1335,8 @@ static int loop_clr_fd(struct loop_device *lo)
>         struct loop_info64 info64;
>         int err;
>
> -       if (!arg) {
> -               mutex_unlock(&lo->lo_ctl_mutex);
> +       if (!arg)
>                 return -EINVAL;
> -       }
>         err = loop_get_status(lo, &info64);
>         if (!err)
>                 err = loop_info64_to_old(&info64, &info);
> @@ -1307,10 +1351,8 @@ static int loop_clr_fd(struct loop_device *lo)
>         struct loop_info64 info64;
>         int err;
>
> -       if (!arg) {
> -               mutex_unlock(&lo->lo_ctl_mutex);
> +       if (!arg)
>                 return -EINVAL;
> -       }
>         err = loop_get_status(lo, &info64);
>         if (!err && copy_to_user(arg, &info64, sizeof(info64)))
>                 err = -EFAULT;
> @@ -1320,6 +1362,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;
>
> @@ -1329,6 +1372,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;
>
> @@ -1342,6 +1387,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;
>
> @@ -1364,12 +1410,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(&lo->lo_ctl_mutex, 1);
>         if (err)
> -               goto out_unlocked;
> -
> +               return err;
>         switch (cmd) {
>         case LOOP_SET_FD:
>                 err = loop_set_fd(lo, mode, bdev, arg);
> @@ -1378,10 +1422,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 */
>                 err = loop_clr_fd(lo);
> -               if (!err)
> -                       goto out_unlocked;
>                 break;
>         case LOOP_SET_STATUS:
>                 err = -EPERM;
> @@ -1391,8 +1432,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 */
> -               goto out_unlocked;
> +               break;
>         case LOOP_SET_STATUS64:
>                 err = -EPERM;
>                 if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> @@ -1401,8 +1441,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 */
> -               goto out_unlocked;
> +               break;
>         case LOOP_SET_CAPACITY:
>                 err = -EPERM;
>                 if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> @@ -1421,9 +1460,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);
> -
> -out_unlocked:
> +       unlock_loop();
>         return err;
>  }
>
> @@ -1524,6 +1561,7 @@ struct compat_loop_info {
>         struct loop_info64 info64;
>         int ret;
>
> +       lockdep_assert_held(&loop_mutex);
>         ret = loop_info64_from_compat(arg, &info64);
>         if (ret < 0)
>                 return ret;
> @@ -1537,10 +1575,9 @@ struct compat_loop_info {
>         struct loop_info64 info64;
>         int err;
>
> -       if (!arg) {
> -               mutex_unlock(&lo->lo_ctl_mutex);
> +       lockdep_assert_held(&loop_mutex);
> +       if (!arg)
>                 return -EINVAL;
> -       }
>         err = loop_get_status(lo, &info64);
>         if (!err)
>                 err = loop_info64_to_compat(&info64, arg);
> @@ -1555,20 +1592,16 @@ 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);
> -               if (!err) {
> +               err = lock_loop_killable();
> +               if (!err)
>                         err = loop_set_status_compat(lo,
>                                                      (const struct compat_loop_info __user *)arg);
> -                       mutex_unlock(&lo->lo_ctl_mutex);
> -               }
>                 break;
>         case LOOP_GET_STATUS:
> -               err = mutex_lock_killable(&lo->lo_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 lo_ctl_mutex */
> -               }
>                 break;
>         case LOOP_SET_CAPACITY:
>         case LOOP_CLR_FD:
> @@ -1583,6 +1616,7 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
>                 err = -ENOIOCTLCMD;
>                 break;
>         }
> +       unlock_loop();
>         return err;
>  }
>  #endif
> @@ -1590,37 +1624,30 @@ 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(&lo->lo_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,
> @@ -1629,15 +1656,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(&lo->lo_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 = {
> @@ -1676,10 +1701,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(&lo->lo_ctl_mutex);
>         if (lo->lo_encryption == xfer)
>                 loop_release_xfer(lo);
> -       mutex_unlock(&lo->lo_ctl_mutex);
>         return 0;
>  }
>
> @@ -1691,8 +1714,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;
>  }
>
> @@ -1860,7 +1889,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);
> @@ -1936,20 +1964,19 @@ 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);
> +       unlock_loop();
>
> -       *part = 0;
>         return kobj;
>  }
>
> @@ -1957,9 +1984,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);
> @@ -1973,21 +2002,17 @@ 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);
>                 if (ret)
>                         break;
>                 if (lo->lo_state != Lo_unbound) {
>                         ret = -EBUSY;
> -                       mutex_unlock(&lo->lo_ctl_mutex);
>                         break;
>                 }
>                 if (atomic_read(&lo->lo_refcnt) > 0) {
>                         ret = -EBUSY;
> -                       mutex_unlock(&lo->lo_ctl_mutex);
>                         break;
>                 }
>                 lo->lo_disk->private_data = NULL;
> -               mutex_unlock(&lo->lo_ctl_mutex);
>                 idr_remove(&loop_index_idr, lo->lo_number);
>                 loop_remove(lo);
>                 break;
> @@ -1997,7 +2022,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;
>  }
> @@ -2081,10 +2106,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;
> diff --git a/drivers/block/loop.h b/drivers/block/loop.h
> index 0f45416..61c6421 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