| Message ID | 6bee3eddc24ef5525ca12efb023e66b2503cb178.1523033157.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 06, 2018 at 09:57:03AM -0700, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > 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+31e8daa8b3fc129e75f2@syzkaller.appspotmail.com > Fixes: 2d1d4c1e591f ("loop: don't call into filesystem while holding lo_ctl_mutex") > Signed-off-by: Omar Sandoval <osandov@fb.com> > --- > 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")
On 4/6/18 10:57 AM, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > 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.
Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > 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+31e8daa8b3fc129e75f2@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 <osandov@fb.com> 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?
On Sat, Apr 7, 2018 at 9:27 AM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > Omar Sandoval wrote: >> From: Omar Sandoval <osandov@fb.com> >> >> 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+31e8daa8b3fc129e75f2@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 <osandov@fb.com> > > 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?
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, &info64); + if (!arg) { + mutex_unlock(&lo->lo_ctl_mutex); + return -EINVAL; + } + err = loop_get_status(lo, &info64); if (!err) err = loop_info64_to_old(&info64, &info); if (!err && copy_to_user(arg, &info, 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, &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; @@ -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, &info64); + if (!arg) { + mutex_unlock(&lo->lo_ctl_mutex); + return -EINVAL; + } + err = loop_get_status(lo, &info64); if (!err) err = loop_info64_to_compat(&info64, arg); return err;