Re: possible deadlock in blkdev_reread_part
On Thu, Sep 13, 2018 at 3:43 PM, Tetsuo Handa wrote: > On 2018/09/13 21:58, Dmitry Vyukov wrote: >> On Wed, May 2, 2018 at 1:23 PM, Dmitry Vyukov wrote: >>> On Wed, May 2, 2018 at 12:30 PM, Tetsuo Handa >>> wrote: Dmitry Vyukov wrote: >> 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 OK, thanks. But among loop related reports, this will be a dup of "INFO: rcu detected stall in blkdev_ioctl" which already has C reproducer. Should we merge them? >>> >>> Yes, sure, I will take care of it. >> >> 1. I forgot to take care of it. >> >> 2. "INFO: rcu detected stall in blkdev_ioctl" was fixed 3 months ago: >> https://syzkaller.appspot.com/bug?id=1f7b710f4110f225aed1f4263ec2b98b8dbd472e >> >> but this bug still happens up until now: >> https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889 >> >> So this is a different bug. > > I'm not sure what you are talking about. > RCU stall and lockdep warning are obviously different bugs. Ah, OK, I misinterpreted your "But among loop related reports, this will be a dup of "INFO: rcu detected stall in blkdev_ioctl"". So this is just an independent bug that still happens. > Regarding lockdep warning on loop module, we are still waiting for Jens to > come up a better alternative than > http://lkml.kernel.org/r/1527297408-4428-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp > . > Since no alternative was proposed, I think we should start testing my patch. >
Re: possible deadlock in blkdev_reread_part
On 2018/09/13 21:58, Dmitry Vyukov wrote: > On Wed, May 2, 2018 at 1:23 PM, Dmitry Vyukov wrote: >> On Wed, May 2, 2018 at 12:30 PM, Tetsuo Handa >> wrote: >>> Dmitry Vyukov wrote: > 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 >>> >>> OK, thanks. But among loop related reports, this will be a dup of >>> "INFO: rcu detected stall in blkdev_ioctl" which already has C reproducer. >>> Should we merge them? >> >> Yes, sure, I will take care of it. > > 1. I forgot to take care of it. > > 2. "INFO: rcu detected stall in blkdev_ioctl" was fixed 3 months ago: > https://syzkaller.appspot.com/bug?id=1f7b710f4110f225aed1f4263ec2b98b8dbd472e > > but this bug still happens up until now: > https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889 > > So this is a different bug. I'm not sure what you are talking about. RCU stall and lockdep warning are obviously different bugs. Regarding lockdep warning on loop module, we are still waiting for Jens to come up a better alternative than http://lkml.kernel.org/r/1527297408-4428-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp . Since no alternative was proposed, I think we should start testing my patch.
Re: possible deadlock in blkdev_reread_part
On Wed, May 2, 2018 at 1:23 PM, Dmitry Vyukov wrote: > On Wed, May 2, 2018 at 12:30 PM, Tetsuo Handa > wrote: >> Dmitry Vyukov wrote: >>> > 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 >> >> OK, thanks. But among loop related reports, this will be a dup of >> "INFO: rcu detected stall in blkdev_ioctl" which already has C reproducer. >> Should we merge them? > > Yes, sure, I will take care of it. 1. I forgot to take care of it. 2. "INFO: rcu detected stall in blkdev_ioctl" was fixed 3 months ago: https://syzkaller.appspot.com/bug?id=1f7b710f4110f225aed1f4263ec2b98b8dbd472e but this bug still happens up until now: https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889 So this is a different bug. >> INFO: rcu detected stall in blkdev_ioctl >> >> https://syzkaller.appspot.com/bug?id=1f7b710f4110f225aed1f4263ec2b98b8dbd472e >> >> general protection fault in lo_ioctl (2) >> >> https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3 >> #syz dup: INFO: rcu detected stall in blkdev_ioctl >> >> INFO: rcu detected stall in lo_compat_ioctl >> >> https://syzkaller.appspot.com/bug?id=6299555c4e252b53f7a2ae2b8216cc9456c56ac0 >> #syz dup: INFO: rcu detected stall in blkdev_ioctl >> >> INFO: rcu detected stall in lo_ioctl >> >> https://syzkaller.appspot.com/bug?id=7b49fb610af9cca78c24e9f796f2e8b0d5573997 >> #syz dup: INFO: rcu detected stall in blkdev_ioctl >> >> INFO: task hung in lo_ioctl >> >> https://syzkaller.appspot.com/bug?id=608144371e7fc2cb6285b9ed871fb1eb817a61ce >> >> INFO: task hung in lo_open (2) >> >> https://syzkaller.appspot.com/bug?id=1f93b57f496d969efb9fb24167f6f9de5ee068fd >> >> possible deadlock in blkdev_reread_part >> >> https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889 >> >> INFO: task hung in loop_control_ioctl >> >> https://syzkaller.appspot.com/bug?id=61fe32c77ea00412c5149bd34649a65b7f672b5e >> >> WARNING in sysfs_remove_group >> >> https://syzkaller.appspot.com/bug?id=3f86c0edf75c86d2633aeb9dd69eccc70bc7e90b >> >>> >>> > 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.
Re: possible deadlock in blkdev_reread_part
On Wed, May 2, 2018 at 12:30 PM, Tetsuo Handa wrote: > Dmitry Vyukov wrote: >> > 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 > > OK, thanks. But among loop related reports, this will be a dup of > "INFO: rcu detected stall in blkdev_ioctl" which already has C reproducer. > Should we merge them? Yes, sure, I will take care of it. > INFO: rcu detected stall in blkdev_ioctl > > https://syzkaller.appspot.com/bug?id=1f7b710f4110f225aed1f4263ec2b98b8dbd472e > > general protection fault in lo_ioctl (2) > > https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3 > #syz dup: INFO: rcu detected stall in blkdev_ioctl > > INFO: rcu detected stall in lo_compat_ioctl > > https://syzkaller.appspot.com/bug?id=6299555c4e252b53f7a2ae2b8216cc9456c56ac0 > #syz dup: INFO: rcu detected stall in blkdev_ioctl > > INFO: rcu detected stall in lo_ioctl > > https://syzkaller.appspot.com/bug?id=7b49fb610af9cca78c24e9f796f2e8b0d5573997 > #syz dup: INFO: rcu detected stall in blkdev_ioctl > > INFO: task hung in lo_ioctl > > https://syzkaller.appspot.com/bug?id=608144371e7fc2cb6285b9ed871fb1eb817a61ce > > INFO: task hung in lo_open (2) > > https://syzkaller.appspot.com/bug?id=1f93b57f496d969efb9fb24167f6f9de5ee068fd > > possible deadlock in blkdev_reread_part > > https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889 > > INFO: task hung in loop_control_ioctl > > https://syzkaller.appspot.com/bug?id=61fe32c77ea00412c5149bd34649a65b7f672b5e > > WARNING in sysfs_remove_group > > https://syzkaller.appspot.com/bug?id=3f86c0edf75c86d2633aeb9dd69eccc70bc7e90b > >> >> > 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.
Re: possible deadlock in blkdev_reread_part
Dmitry Vyukov wrote: > > 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 OK, thanks. But among loop related reports, this will be a dup of "INFO: rcu detected stall in blkdev_ioctl" which already has C reproducer. Should we merge them? INFO: rcu detected stall in blkdev_ioctl https://syzkaller.appspot.com/bug?id=1f7b710f4110f225aed1f4263ec2b98b8dbd472e general protection fault in lo_ioctl (2) https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3 #syz dup: INFO: rcu detected stall in blkdev_ioctl INFO: rcu detected stall in lo_compat_ioctl https://syzkaller.appspot.com/bug?id=6299555c4e252b53f7a2ae2b8216cc9456c56ac0 #syz dup: INFO: rcu detected stall in blkdev_ioctl INFO: rcu detected stall in lo_ioctl https://syzkaller.appspot.com/bug?id=7b49fb610af9cca78c24e9f796f2e8b0d5573997 #syz dup: INFO: rcu detected stall in blkdev_ioctl INFO: task hung in lo_ioctl https://syzkaller.appspot.com/bug?id=608144371e7fc2cb6285b9ed871fb1eb817a61ce INFO: task hung in lo_open (2) https://syzkaller.appspot.com/bug?id=1f93b57f496d969efb9fb24167f6f9de5ee068fd possible deadlock in blkdev_reread_part https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889 INFO: task hung in loop_control_ioctl https://syzkaller.appspot.com/bug?id=61fe32c77ea00412c5149bd34649a65b7f672b5e WARNING in sysfs_remove_group https://syzkaller.appspot.com/bug?id=3f86c0edf75c86d2633aeb9dd69eccc70bc7e90b > > > 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.
Re: possible deadlock in blkdev_reread_part
/\/\/\/\/\/\On Fri, Apr 20, 2018 at 4:06 PM, Tetsuo Handa
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
> 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
> 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 c9d0449..59716d1 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(&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.
> + */
> +s
Re: possible deadlock in blkdev_reread_part
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
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
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
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 c9d0449..59716d1 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(&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 "s
Re: possible deadlock in blkdev_reread_part
Eric Biggers wrote:
> Here's a simplified reproducer:
>
> #include
> #include
> #include
> #include
>
> int main()
> {
> int loopfd, fd;
> struct loop_info info = { .lo_flags = LO_FLAGS_PARTSCAN };
>
> loopfd = open("/dev/loop0", O_RDWR);
>
> fd = open("/bin/ls", O_RDONLY);
>
> ioctl(loopfd, LOOP_SET_FD, fd);
>
> ioctl(loopfd, LOOP_SET_STATUS, &info);
> }
>
> It still needs to be compiled with -m32. The reason is that lo_ioctl() has:
>
> mutex_lock_nested(&lo->lo_ctl_mutex, 1);
>
> but lo_compat_ioctl() has:
>
> mutex_lock(&lo->lo_ctl_mutex);
>
> But ->lo_ctl_mutex isn't actually being nested under itself, so I don't think
> the "nested" annotation is actually appropriate.
Yes, I think we should try
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c9d0449..70d6736 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1366,7 +1366,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(&lo->lo_ctl_mutex);
if (err)
goto out_unlocked;
and check what the lockdep says. Use of "_nested" version could be the cause of
various hung up without lockdep warning.
>
> 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.
Also, what is wrong with not using LO_FLAGS_AUTOCLEAR when ->lo_refcnt > 1?
As long as each ioctl() are serialized, I feel there should be no problem.
I think it is strange to fail below case just because somebody else (not
limited to blkid command) opened the same device.
#include
#include
#include
#include
int main(int argc, char *argv[])
{
int fd = open("/dev/loop0", O_RDWR);
int fd2 = open("/bin/ls", O_RDONLY);
int fd3 = open("/bin/cat", O_RDONLY);
ioctl(fd, LOOP_SET_FD, fd2);
ioctl(fd, LOOP_CLR_FD, fd2);
ioctl(fd, LOOP_SET_FD, fd3); /* <= 0 or EBUSY depending on whether somebody
else is opening /dev/loop0 */
ioctl(fd, LOOP_CLR_FD, fd3);
return 0;
}
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c9d0449..3910e5b 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -993,6 +993,38 @@ static int loop_set_fd(struct loop_device *lo, fmode_t
mode,
return err;
}
+static int loop_lock_for_ioctl(struct loop_device *lo)
+{
+ while (1) {
+ int err = mutex_lock_killable(&lo->lo_ctl_mutex);
+ if (err)
+ return err;
+ if (!lo->ioctl_in_progress) {
+ lo->ioctl_in_progress = true;
+ mutex_unlock(&lo->lo_ctl_mutex);
+ return 0;
+ }
+ mutex_unlock(&lo->lo_ctl_mutex);
+ schedule_timeout_killable(1);
+ }
+}
+
+static void loop_lock_wait_for_ioctl_completion(struct loop_device *lo)
+{
+ while (1) {
+ mutex_lock(&lo->lo_ctl_mutex);
+ if (!lo->ioctl_in_progress)
+ return;
+ mutex_unlock(&lo->lo_ctl_mutex);
+ schedule_timeout_uninterruptible(1);
+ }
+}
+
+static void loop_unlock_for_ioctl(struct loop_device *lo)
+{
+ lo->ioctl_in_progress = false;
+}
+
static int loop_clr_fd(struct loop_device *lo)
{
struct file *filp = lo->lo_backing_file;
@@ -1002,22 +1034,6 @@ static int loop_clr_fd(struct loop_device *lo)
if (lo->lo_state != Lo_bound)
return -ENXIO;
- /*
-* If we've
Re: possible deadlock in blkdev_reread_part
On Wed, Nov 01, 2017 at 10:02:44PM +0300, 'Dmitry Vyukov' via syzkaller-bugs
wrote:
>
> Still happens on linux-next 36ef71cae353f88fd6e095e2aaa3e5953af1685d (Oct 20).
> Note repro needs to be compiled with -m32
>
> [ 243.819514] ==
> [ 243.820949] WARNING: possible circular locking dependency detected
> [ 243.822417] 4.14.0-rc5-next-20171018 #15 Not tainted
> [ 243.823592] --
> [ 243.825012] a.out/11871 is trying to acquire lock:
> [ 243.826182] (&bdev->bd_mutex){+.+.}, at: []
> blkdev_reread_part+0x1e/0x40
> [ 243.828317]
> [ 243.828317] but task is already holding lock:
> [ 243.829669] (&lo->lo_ctl_mutex#2){+.+.}, at: []
> lo_compat_ioctl+0x119/0x150
> [ 243.831728]
> [ 243.831728] which lock already depends on the new lock.
> [ 243.831728]
> [ 243.833373]
Here's a simplified reproducer:
#include
#include
#include
#include
int main()
{
int loopfd, fd;
struct loop_info info = { .lo_flags = LO_FLAGS_PARTSCAN };
loopfd = open("/dev/loop0", O_RDWR);
fd = open("/bin/ls", O_RDONLY);
ioctl(loopfd, LOOP_SET_FD, fd);
ioctl(loopfd, LOOP_SET_STATUS, &info);
}
It still needs to be compiled with -m32. The reason is that lo_ioctl() has:
mutex_lock_nested(&lo->lo_ctl_mutex, 1);
but lo_compat_ioctl() has:
mutex_lock(&lo->lo_ctl_mutex);
But ->lo_ctl_mutex isn't actually being nested under itself, so I don't think
the "nested" annotation is actually appropriate.
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()?
Eric
Re: possible deadlock in blkdev_reread_part
On Wed, Nov 1, 2017 at 10:01 PM, syzbot
wrote:
> Hello,
>
> syzkaller hit the following crash on
> e19b205be43d11bff638cad4487008c48d21c103
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers
>
>
> ==
> WARNING: possible circular locking dependency detected
> 4.14.0-rc2+ #10 Not tainted
> --
> syzkaller821047/2981 is trying to acquire lock:
> (&bdev->bd_mutex){+.+.}, at: []
> blkdev_reread_part+0x1e/0x40 block/ioctl.c:192
>
> but task is already holding lock:
> (&lo->lo_ctl_mutex#2){+.+.}, at: []
> lo_compat_ioctl+0x109/0x140 drivers/block/loop.c:1533
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&lo->lo_ctl_mutex#2){+.+.}:
>check_prevs_add kernel/locking/lockdep.c:2020 [inline]
>validate_chain kernel/locking/lockdep.c:2469 [inline]
>__lock_acquire+0x328f/0x4620 kernel/locking/lockdep.c:3498
>lock_acquire+0x1d5/0x580 kernel/locking/lockdep.c:4002
>__mutex_lock_common kernel/locking/mutex.c:756 [inline]
>__mutex_lock+0x16f/0x19d0 kernel/locking/mutex.c:893
>mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:908
>lo_release+0x6b/0x180 drivers/block/loop.c:1587
>__blkdev_put+0x602/0x7c0 fs/block_dev.c:1780
>blkdev_put+0x85/0x4f0 fs/block_dev.c:1845
>blkdev_close+0x91/0xc0 fs/block_dev.c:1852
>__fput+0x333/0x7f0 fs/file_table.c:210
>fput+0x15/0x20 fs/file_table.c:244
>task_work_run+0x199/0x270 kernel/task_work.c:112
>tracehook_notify_resume include/linux/tracehook.h:191 [inline]
>exit_to_usermode_loop+0x296/0x310 arch/x86/entry/common.c:162
>prepare_exit_to_usermode arch/x86/entry/common.c:197 [inline]
>syscall_return_slowpath+0x42f/0x510 arch/x86/entry/common.c:266
>entry_SYSCALL_64_fastpath+0xbc/0xbe
>
> -> #0 (&bdev->bd_mutex){+.+.}:
>check_prev_add+0x865/0x1520 kernel/locking/lockdep.c:1894
>check_prevs_add kernel/locking/lockdep.c:2020 [inline]
>validate_chain kernel/locking/lockdep.c:2469 [inline]
>__lock_acquire+0x328f/0x4620 kernel/locking/lockdep.c:3498
>lock_acquire+0x1d5/0x580 kernel/locking/lockdep.c:4002
>__mutex_lock_common kernel/locking/mutex.c:756 [inline]
>__mutex_lock+0x16f/0x19d0 kernel/locking/mutex.c:893
>mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:908
>blkdev_reread_part+0x1e/0x40 block/ioctl.c:192
>loop_reread_partitions+0x12f/0x1a0 drivers/block/loop.c:614
>loop_set_status+0x9ba/0xf60 drivers/block/loop.c:1156
>loop_set_status_compat+0x92/0xf0 drivers/block/loop.c:1506
>lo_compat_ioctl+0x114/0x140 drivers/block/loop.c:1534
>compat_blkdev_ioctl+0x3ba/0x1850 block/compat_ioctl.c:405
>C_SYSC_ioctl fs/compat_ioctl.c:1593 [inline]
>compat_SyS_ioctl+0x1d7/0x3290 fs/compat_ioctl.c:1540
>do_syscall_32_irqs_on arch/x86/entry/common.c:329 [inline]
>do_fast_syscall_32+0x3f2/0xf05 arch/x86/entry/common.c:391
>entry_SYSENTER_compat+0x51/0x60 arch/x86/entry/entry_64_compat.S:124
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
>CPU0CPU1
>
> lock(&lo->lo_ctl_mutex#2);
>lock(&bdev->bd_mutex);
>lock(&lo->lo_ctl_mutex#2);
> lock(&bdev->bd_mutex);
>
> *** DEADLOCK ***
>
> 1 lock held by syzkaller821047/2981:
> #0: (&lo->lo_ctl_mutex#2){+.+.}, at: []
> lo_compat_ioctl+0x109/0x140 drivers/block/loop.c:1533
>
> stack backtrace:
> CPU: 0 PID: 2981 Comm: syzkaller821047 Not tainted 4.14.0-rc2+ #10
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:16 [inline]
> dump_stack+0x194/0x257 lib/dump_stack.c:52
> print_circular_bug+0x503/0x710 kernel/locking/lockdep.c:1259
> check_prev_add+0x865/0x1520 kernel/locking/lockdep.c:1894
> check_prevs_add kernel/locking/lockdep.c:2020 [inline]
> validate_chain kernel/locking/lockdep.c:2469 [inline]
> __lock_acquire+0x328f/0x4620 kernel/locking/lockdep.c:3498
> lock_acquire+0x1d5/0x580 kernel/locking/lockdep.c:4002
> __mutex_lock_common kernel/locking/mutex.c:756 [inline]
> __mutex_lock+0x16f/0x19d0 kernel/locking/mutex.c:893
> mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:908
> blkdev_reread_part+0x1e/0x40 block/ioctl.c:192
> loop_reread_partitions+0x12f/0x1a0 drivers/block/loop.c:614
> loop_set_status+0x9ba/0xf60 drivers/block/loop.c:1156
> loop_set_status_compat+0x
