INFO: task hung in lo_ioctl

Commit Message

Tetsuo Handa April 6, 2018, 12:04 p.m. UTC

On 2018/04/05 0:23, Tetsuo Handa wrote:
> This seems to be an AB-BA deadlock where the lockdep cannot report (due to use of nested lock?).
> What is happening here?
> 

This patch does not directly fix the bug syzbot is reporting, but could be relevant.
Maybe try replacing mutex_lock_killable_nested() with mutex_lock_killable() and
check what the lockdep will say?



From 0b006d536b2e439f347eb857e482fc304e84fd1d Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <[e-mail]>
Date: Fri, 6 Apr 2018 20:52:10 +0900
Subject: [PATCH] block/loop: fix lock imbalance bug at lo_ioctl

Commit 2d1d4c1e591fd40b ("loop: don't call into filesystem while holding
lo_ctl_mutex") introduced mutex lock imbalance bug where syzbot would
trigger. The caller of loop_get_status64() assumes that mutex_unlock() is
already called by loop_get_status() but loop_get_status64() does not
always call loop_get_status().

Commit f028f3b2f987ebc6 ("loop: fix circular locking in loop_clr_fd()")
also dropped the mutex for deadlock avoidance reason. But we should
recheck whether we have to drop the mutex. Dropping the mutex at the
middle of an ioctl() request is not nice, but syzbot is reporting a
deadlock inside loop_reread_partitions() which is called by loop_set_fd()
and loop_change_fd().

Signed-off-by: Tetsuo Handa <[e-mail]>
Fixes: 2d1d4c1e591fd40b ("loop: don't call into filesystem while holding lo_ctl_mutex")
Cc: Omar Sandoval <[e-mail]>
Cc: Nikanth Karthikesan <[e-mail]>
Cc: Jens Axboe <[e-mail]>
---
 drivers/block/loop.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

Comments

Peter Zijlstra April 6, 2018, 12:14 p.m. UTC | #1

On Fri, Apr 06, 2018 at 09:04:18PM +0900, Tetsuo Handa wrote:
> +	/* Temporary hack for handling lock imbalance. */
> +	if (__mutex_owner(&lo->lo_ctl_mutex) == current)
> +		mutex_unlock(&lo->lo_ctl_mutex);

ARGGH.. you didn't read the comment we put on that?

Tetsuo Handa April 6, 2018, 1:55 p.m. UTC | #2

Peter Zijlstra wrote:
> On Fri, Apr 06, 2018 at 09:04:18PM +0900, Tetsuo Handa wrote:
> > +	/* Temporary hack for handling lock imbalance. */
> > +	if (__mutex_owner(&lo->lo_ctl_mutex) == current)
> > +		mutex_unlock(&lo->lo_ctl_mutex);
> 
> ARGGH.. you didn't read the comment we put on that?
> 

Commit 5b52330bbfe63b33 ("audit: fix auditd/kernel connection state tracking")
is using __mutex_owner(). ;-)

Of course, regarding loop module, we will be able to add a flag variable
associated with lo->lo_ctl_mutex. But that will be done after we solved
the deadlock problem. I think whether we need to drop lo->lo_ctl_mutex is
not clear. Maybe

-	err = mutex_lock_killable_nested(&lo->lo_ctl_mutex, 1);
+	err = mutex_lock_killable(&lo->lo_ctl_mutex);

on top of this patch and listen to the lockdep?

Commit f028f3b2f987ebc6 ("loop: fix circular locking in loop_clr_fd()") says

  A simple way to silence lockdep could be to mark the lo_ctl_mutex
  in ioctl to be a sub class, but this might mask some other real bugs.

and we are currently hitting a deadlock problem.

Peter Zijlstra April 6, 2018, 3:43 p.m. UTC | #3

On Fri, Apr 06, 2018 at 10:55:03PM +0900, Tetsuo Handa wrote:
> Peter Zijlstra wrote:
> > On Fri, Apr 06, 2018 at 09:04:18PM +0900, Tetsuo Handa wrote:
> > > +	/* Temporary hack for handling lock imbalance. */
> > > +	if (__mutex_owner(&lo->lo_ctl_mutex) == current)
> > > +		mutex_unlock(&lo->lo_ctl_mutex);
> > 
> > ARGGH.. you didn't read the comment we put on that?
> > 
> 
> Commit 5b52330bbfe63b33 ("audit: fix auditd/kernel connection state tracking")
> is using __mutex_owner(). ;-)

That got removed and the warning added.

Patch

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 264abaa..64033e7 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1362,7 +1362,7 @@  static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 
 	err = mutex_lock_killable_nested(&lo->lo_ctl_mutex, 1);
 	if (err)
-		goto out_unlocked;
+		return err;
 
 	switch (cmd) {
 	case LOOP_SET_FD:
@@ -1372,10 +1372,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;
@@ -1385,8 +1382,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))
@@ -1395,8 +1391,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))
@@ -1415,9 +1410,9 @@  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:
+	/* Temporary hack for handling lock imbalance. */
+	if (__mutex_owner(&lo->lo_ctl_mutex) == current)
+		mutex_unlock(&lo->lo_ctl_mutex);
 	return err;
 }