| Message ID | 20180406155905.GA11697@vader (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 6, 2018 at 5:59 PM Omar Sandoval <osandov@osandov.com> wrote: > > On Fri, Apr 06, 2018 at 05:43:43PM +0200, Peter Zijlstra wrote: > > 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. > > Seems easy enough to fix without resorting to __mutex_owner() (untested): > > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 264abaaff662..cee258d12a1e 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -1300,12 +1300,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, &info64); > + if (!arg) { > + mutex_unlock(&lo->lo_ctl_mutex); > + return -EINVAL; > + } > + err = loop_get_status(lo, &info64); > if (!err && copy_to_user(arg, &info64, sizeof(info64))) > err = -EFAULT; > > > I'll test it and send it up when I get into the office. Was this ever submitted? Or some other fix for this? The bug is still open, but last happened 289 days ago: https://syzkaller.appspot.com/bug?id=608144371e7fc2cb6285b9ed871fb1eb817a61ce But it also has 10 duplicates, some of which happened much more recently. If a fix was submitted, but Reported-by tag wasn't added this open bug can now mask lots of other new bugs.
On 2019/01/20 3:56, Dmitry Vyukov wrote: >> Seems easy enough to fix without resorting to __mutex_owner() (untested): >> >> >> diff --git a/drivers/block/loop.c b/drivers/block/loop.c >> index 264abaaff662..cee258d12a1e 100644 >> --- a/drivers/block/loop.c >> +++ b/drivers/block/loop.c >> @@ -1300,12 +1300,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, &info64); >> + if (!arg) { >> + mutex_unlock(&lo->lo_ctl_mutex); >> + return -EINVAL; >> + } >> + err = loop_get_status(lo, &info64); >> if (!err && copy_to_user(arg, &info64, sizeof(info64))) >> err = -EFAULT; >> >> >> I'll test it and send it up when I get into the office. > > > Was this ever submitted? Or some other fix for this? > > The bug is still open, but last happened 289 days ago: > https://syzkaller.appspot.com/bug?id=608144371e7fc2cb6285b9ed871fb1eb817a61ce > > But it also has 10 duplicates, some of which happened much more recently. > If a fix was submitted, but Reported-by tag wasn't added this open bug > can now mask lots of other new bugs. > The commit for this specific patch is bdac616db9bbadb9 ("loop: fix LOOP_GET_STATUS lock imbalance"). But the root cause of these hung tasks would be fixed by a series containing commit 1dded9acf6dc9a34 ("Avoid circular locking dependency between loop_ctl_mutex and bd_mutex") or commit 04906b2f542c2362 ("blockdev: Fix livelocks on loop device"). We were not aware of these bugs when you marked these reports as duplicates on 2017/12/12. You can undup them and fix them if you want.
On Sun, Jan 20, 2019 at 3:36 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2019/01/20 3:56, Dmitry Vyukov wrote: > >> Seems easy enough to fix without resorting to __mutex_owner() (untested): > >> > >> > >> diff --git a/drivers/block/loop.c b/drivers/block/loop.c > >> index 264abaaff662..cee258d12a1e 100644 > >> --- a/drivers/block/loop.c > >> +++ b/drivers/block/loop.c > >> @@ -1300,12 +1300,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, &info64); > >> + if (!arg) { > >> + mutex_unlock(&lo->lo_ctl_mutex); > >> + return -EINVAL; > >> + } > >> + err = loop_get_status(lo, &info64); > >> if (!err && copy_to_user(arg, &info64, sizeof(info64))) > >> err = -EFAULT; > >> > >> > >> I'll test it and send it up when I get into the office. > > > > > > Was this ever submitted? Or some other fix for this? > > > > The bug is still open, but last happened 289 days ago: > > https://syzkaller.appspot.com/bug?id=608144371e7fc2cb6285b9ed871fb1eb817a61ce > > > > But it also has 10 duplicates, some of which happened much more recently. > > If a fix was submitted, but Reported-by tag wasn't added this open bug > > can now mask lots of other new bugs. > > > > The commit for this specific patch is bdac616db9bbadb9 ("loop: fix LOOP_GET_STATUS > lock imbalance"). But the root cause of these hung tasks would be fixed by a series > containing commit 1dded9acf6dc9a34 ("Avoid circular locking dependency between > loop_ctl_mutex and bd_mutex") or commit 04906b2f542c2362 ("blockdev: Fix livelocks > on loop device"). We were not aware of these bugs when you marked these reports as > duplicates on 2017/12/12. You can undup them and fix them if you want. OK, let's just do then: #syz fix: blockdev: Fix livelocks on loop device
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 264abaaff662..cee258d12a1e 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1300,12 +1300,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, &info64); + if (!arg) { + mutex_unlock(&lo->lo_ctl_mutex); + return -EINVAL; + } + err = loop_get_status(lo, &info64); if (!err && copy_to_user(arg, &info64, sizeof(info64))) err = -EFAULT;