Re: [PATCH] loop: fix LOOP_GET_STATUS lock imbalance

[Dmitry Vyukov]

On Sat, Apr 7, 2018 at 9:27 AM, Tetsuo Handa
 wrote:
> Omar Sandoval wrote:
>> From: Omar Sandoval 
>>
>> Commit 2d1d4c1e591f made loop_get_status() drop lo_ctx_mutex before
>> returning, but the loop_get_status_old(), loop_get_status64(), and
>> loop_get_status_compat() wrappers don't call loop_get_status() if the
>> passed argument is NULL. The callers expect that the lock is dropped, so
>> make sure we drop it in that case, too.
>>
>> Reported-by: syzbot+31e8daa8b3fc129e7...@syzkaller.appspotmail.com
>
> Well, it is me who reported this bug before syzbot reports it. ;-)

Robots are taking our jobs! :)

We could notify syzbot with just "#syz fix" tag in the report email,
rather than putting it into Reported-by.


>> Fixes: 2d1d4c1e591f ("loop: don't call into filesystem while holding 
>> lo_ctl_mutex")
>
> But I feel we should revert 2d1d4c1e591f rather than applying this patch.
>
>> Signed-off-by: Omar Sandoval 
>
> If the reason of dropping the lock is to avoid deadlock caused by recursing
> into the same lock from vfs_getattr(), it is correct thing to drop the lock.
>
> But when the reason is that vfs_getattr() cannot return when NFS server is
> dead, there is no point with dropping the lock. Anybody who tries to call
> loop_get_status() will get stuck. It is commit 3148ffbdb9162baa ("loop:
> use killable lock in ioctls") which actually helps minimizing number of
> stuck processes when NFS server is dead if we didn't drop the lock.
> If we drop the lock before calling vfs_getattr(), all threads who called
> loop_get_status() will reach vfs_getattr() and get stuck, won't it?

Re: [PATCH] loop: fix LOOP_GET_STATUS lock imbalance

[Tetsuo Handa]

Omar Sandoval wrote:
> From: Omar Sandoval 
> 
> Commit 2d1d4c1e591f made loop_get_status() drop lo_ctx_mutex before
> returning, but the loop_get_status_old(), loop_get_status64(), and
> loop_get_status_compat() wrappers don't call loop_get_status() if the
> passed argument is NULL. The callers expect that the lock is dropped, so
> make sure we drop it in that case, too.
> 
> Reported-by: syzbot+31e8daa8b3fc129e7...@syzkaller.appspotmail.com

Well, it is me who reported this bug before syzbot reports it. ;-)

> Fixes: 2d1d4c1e591f ("loop: don't call into filesystem while holding 
> lo_ctl_mutex")

But I feel we should revert 2d1d4c1e591f rather than applying this patch.

> Signed-off-by: Omar Sandoval 

If the reason of dropping the lock is to avoid deadlock caused by recursing
into the same lock from vfs_getattr(), it is correct thing to drop the lock.

But when the reason is that vfs_getattr() cannot return when NFS server is
dead, there is no point with dropping the lock. Anybody who tries to call
loop_get_status() will get stuck. It is commit 3148ffbdb9162baa ("loop:
use killable lock in ioctls") which actually helps minimizing number of
stuck processes when NFS server is dead if we didn't drop the lock.
If we drop the lock before calling vfs_getattr(), all threads who called
loop_get_status() will reach vfs_getattr() and get stuck, won't it?

Re: [PATCH] loop: fix LOOP_GET_STATUS lock imbalance

[Jens Axboe]

On 4/6/18 10:57 AM, Omar Sandoval wrote:
> From: Omar Sandoval 
> 
> Commit 2d1d4c1e591f made loop_get_status() drop lo_ctx_mutex before
> returning, but the loop_get_status_old(), loop_get_status64(), and
> loop_get_status_compat() wrappers don't call loop_get_status() if the
> passed argument is NULL. The callers expect that the lock is dropped, so
> make sure we drop it in that case, too.

Applied, thanks Omar.

-- 
Jens Axboe

Re: [PATCH] loop: fix LOOP_GET_STATUS lock imbalance

[Omar Sandoval]

On Fri, Apr 06, 2018 at 09:57:03AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval 
> 
> Commit 2d1d4c1e591f made loop_get_status() drop lo_ctx_mutex before
> returning, but the loop_get_status_old(), loop_get_status64(), and
> loop_get_status_compat() wrappers don't call loop_get_status() if the
> passed argument is NULL. The callers expect that the lock is dropped, so
> make sure we drop it in that case, too.
> 
> Reported-by: syzbot+31e8daa8b3fc129e7...@syzkaller.appspotmail.com
> Fixes: 2d1d4c1e591f ("loop: don't call into filesystem while holding 
> lo_ctl_mutex")
> Signed-off-by: Omar Sandoval 
> ---
> Based on Linus' tree.
> 
>  drivers/block/loop.c | 33 ++---
>  1 file changed, 18 insertions(+), 15 deletions(-)

Also just pushed a regression test to blktests:

140ee15de9f3 ("loop: add ioctl lock imbalance regression test")

[PATCH] loop: fix LOOP_GET_STATUS lock imbalance

[Omar Sandoval]

From: Omar Sandoval 

Commit 2d1d4c1e591f made loop_get_status() drop lo_ctx_mutex before
returning, but the loop_get_status_old(), loop_get_status64(), and
loop_get_status_compat() wrappers don't call loop_get_status() if the
passed argument is NULL. The callers expect that the lock is dropped, so
make sure we drop it in that case, too.

Reported-by: syzbot+31e8daa8b3fc129e7...@syzkaller.appspotmail.com
Fixes: 2d1d4c1e591f ("loop: don't call into filesystem while holding 
lo_ctl_mutex")
Signed-off-by: Omar Sandoval 
---
Based on Linus' tree.

 drivers/block/loop.c | 33 ++---
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 264abaaff662..9b476fd2bc41 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1283,12 +1283,13 @@ static int
 loop_get_status_old(struct loop_device *lo, struct loop_info __user *arg) {
struct loop_info info;
struct loop_info64 info64;
-   int err = 0;
+   int err;
 
-   if (!arg)
-   err = -EINVAL;
-   if (!err)
-   err = loop_get_status(lo, );
+   if (!arg) {
+   mutex_unlock(>lo_ctl_mutex);
+   return -EINVAL;
+   }
+   err = loop_get_status(lo, );
if (!err)
err = loop_info64_to_old(, );
if (!err && copy_to_user(arg, , sizeof(info)))
@@ -1300,12 +1301,13 @@ loop_get_status_old(struct loop_device *lo, struct 
loop_info __user *arg) {
 static int
 loop_get_status64(struct loop_device *lo, struct loop_info64 __user *arg) {
struct loop_info64 info64;
-   int err = 0;
+   int err;
 
-   if (!arg)
-   err = -EINVAL;
-   if (!err)
-   err = loop_get_status(lo, );
+   if (!arg) {
+   mutex_unlock(>lo_ctl_mutex);
+   return -EINVAL;
+   }
+   err = loop_get_status(lo, );
if (!err && copy_to_user(arg, , sizeof(info64)))
err = -EFAULT;
 
@@ -1529,12 +1531,13 @@ loop_get_status_compat(struct loop_device *lo,
   struct compat_loop_info __user *arg)
 {
struct loop_info64 info64;
-   int err = 0;
+   int err;
 
-   if (!arg)
-   err = -EINVAL;
-   if (!err)
-   err = loop_get_status(lo, );
+   if (!arg) {
+   mutex_unlock(>lo_ctl_mutex);
+   return -EINVAL;
+   }
+   err = loop_get_status(lo, );
if (!err)
err = loop_info64_to_compat(, arg);
return err;
-- 
2.17.0