Re: [PATCH] loop: fix LOOP_GET_STATUS lock imbalance
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
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
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
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
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, &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; -- 2.17.0