| Message ID | 20210329164907.2133175-1-mic@digikod.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] ovl: Fix leaked dentry | expand |
On 29/03/2021 18:49, Mickaël Salaün wrote: > From: Mickaël Salaün <mic@linux.microsoft.com> > > Since commit 6815f479ca90 ("ovl: use only uppermetacopy state in > ovl_lookup()"), overlayfs doesn't put temporary dentry when there is a > metacopy error, which leads to dentry leaks when shutting down the > related superblock: > > overlayfs: refusing to follow metacopy origin for (/file0) > ... > BUG: Dentry (____ptrval____){i=3f33,n=file3} still in use (1) [unmount of overlay overlay] > ... > WARNING: CPU: 1 PID: 432 at umount_check.cold+0x107/0x14d > CPU: 1 PID: 432 Comm: unmount-overlay Not tainted 5.12.0-rc5 #1 > ... > RIP: 0010:umount_check.cold+0x107/0x14d > ... > Call Trace: > d_walk+0x28c/0x950 > ? dentry_lru_isolate+0x2b0/0x2b0 > ? __kasan_slab_free+0x12/0x20 > do_one_tree+0x33/0x60 > shrink_dcache_for_umount+0x78/0x1d0 > generic_shutdown_super+0x70/0x440 > kill_anon_super+0x3e/0x70 > deactivate_locked_super+0xc4/0x160 > deactivate_super+0xfa/0x140 > cleanup_mnt+0x22e/0x370 > __cleanup_mnt+0x1a/0x30 > task_work_run+0x139/0x210 > do_exit+0xb0c/0x2820 > ? __kasan_check_read+0x1d/0x30 > ? find_held_lock+0x35/0x160 > ? lock_release+0x1b6/0x660 > ? mm_update_next_owner+0xa20/0xa20 > ? reacquire_held_locks+0x3f0/0x3f0 > ? __sanitizer_cov_trace_const_cmp4+0x22/0x30 > do_group_exit+0x135/0x380 > __do_sys_exit_group.isra.0+0x20/0x20 > __x64_sys_exit_group+0x3c/0x50 > do_syscall_64+0x45/0x70 > entry_SYSCALL_64_after_hwframe+0x44/0xae > ... > VFS: Busy inodes after unmount of overlay. Self-destruct in 5 seconds. Have a nice day... > > This fix has been tested with a syzkaller reproducer. > > Cc: Amir Goldstein <amir73il@gmail.com> > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Vivek Goyal <vgoyal@redhat.com> > Cc: <stable@vger.kernel.org> # v5.7+ The bogus commit 6815f479ca90 was applied on v5.7-rc2 but it is only available since v5.8+ . > Reported-by: syzbot <syzkaller@googlegroups.com> > Fixes: 6815f479ca90 ("ovl: use only uppermetacopy state in ovl_lookup()") > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > Link: https://lore.kernel.org/r/20210329164907.2133175-1-mic@digikod.net > --- > fs/overlayfs/namei.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index 3fe05fb5d145..424c594afd79 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -921,6 +921,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) { > err = -EPERM; > pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry); > + dput(this); > goto out_put; > } > > > base-commit: a5e13c6df0e41702d2b2c77c8ad41677ebb065b3 >
On Mon, Mar 29, 2021 at 06:49:07PM +0200, Mickaël Salaün wrote: > From: Mickaël Salaün <mic@linux.microsoft.com> > > Since commit 6815f479ca90 ("ovl: use only uppermetacopy state in > ovl_lookup()"), overlayfs doesn't put temporary dentry when there is a > metacopy error, which leads to dentry leaks when shutting down the > related superblock: > Hi, Thanks for finding and fixing this bug. Patch looks correct to me. We need to drop that reference to this. I am not able to trigger this warning on umount of overlayfs. I copied up a file with metacopy enabled and then remounted overlay again with metacopy disabled. That does hit this code and I see the warning. refusing to follow metacopy origin for (/foo.txt) This should have lead to leak of dentry pointed by "this". But after that I unmounted, overlay and that succeeds. Is there any additional step to be done to trigger this VFS warning. Vivek > overlayfs: refusing to follow metacopy origin for (/file0) > ... > BUG: Dentry (____ptrval____){i=3f33,n=file3} still in use (1) [unmount of overlay overlay] > ... > WARNING: CPU: 1 PID: 432 at umount_check.cold+0x107/0x14d > CPU: 1 PID: 432 Comm: unmount-overlay Not tainted 5.12.0-rc5 #1 > ... > RIP: 0010:umount_check.cold+0x107/0x14d > ... > Call Trace: > d_walk+0x28c/0x950 > ? dentry_lru_isolate+0x2b0/0x2b0 > ? __kasan_slab_free+0x12/0x20 > do_one_tree+0x33/0x60 > shrink_dcache_for_umount+0x78/0x1d0 > generic_shutdown_super+0x70/0x440 > kill_anon_super+0x3e/0x70 > deactivate_locked_super+0xc4/0x160 > deactivate_super+0xfa/0x140 > cleanup_mnt+0x22e/0x370 > __cleanup_mnt+0x1a/0x30 > task_work_run+0x139/0x210 > do_exit+0xb0c/0x2820 > ? __kasan_check_read+0x1d/0x30 > ? find_held_lock+0x35/0x160 > ? lock_release+0x1b6/0x660 > ? mm_update_next_owner+0xa20/0xa20 > ? reacquire_held_locks+0x3f0/0x3f0 > ? __sanitizer_cov_trace_const_cmp4+0x22/0x30 > do_group_exit+0x135/0x380 > __do_sys_exit_group.isra.0+0x20/0x20 > __x64_sys_exit_group+0x3c/0x50 > do_syscall_64+0x45/0x70 > entry_SYSCALL_64_after_hwframe+0x44/0xae > ... > VFS: Busy inodes after unmount of overlay. Self-destruct in 5 seconds. Have a nice day... > > This fix has been tested with a syzkaller reproducer. > > Cc: Amir Goldstein <amir73il@gmail.com> > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Vivek Goyal <vgoyal@redhat.com> > Cc: <stable@vger.kernel.org> # v5.7+ > Reported-by: syzbot <syzkaller@googlegroups.com> > Fixes: 6815f479ca90 ("ovl: use only uppermetacopy state in ovl_lookup()") > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > Link: https://lore.kernel.org/r/20210329164907.2133175-1-mic@digikod.net > --- > fs/overlayfs/namei.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index 3fe05fb5d145..424c594afd79 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -921,6 +921,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) { > err = -EPERM; > pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry); > + dput(this); > goto out_put; > } > > > base-commit: a5e13c6df0e41702d2b2c77c8ad41677ebb065b3 > -- > 2.30.2 >
On Mon, Mar 29, 2021 at 06:49:07PM +0200, Mickaël Salaün wrote: > From: Mickaël Salaün <mic@linux.microsoft.com> > > Since commit 6815f479ca90 ("ovl: use only uppermetacopy state in > ovl_lookup()"), overlayfs doesn't put temporary dentry when there is a > metacopy error, which leads to dentry leaks when shutting down the > related superblock: > > overlayfs: refusing to follow metacopy origin for (/file0) > ... > BUG: Dentry (____ptrval____){i=3f33,n=file3} still in use (1) [unmount of overlay overlay] > ... > WARNING: CPU: 1 PID: 432 at umount_check.cold+0x107/0x14d > CPU: 1 PID: 432 Comm: unmount-overlay Not tainted 5.12.0-rc5 #1 > ... > RIP: 0010:umount_check.cold+0x107/0x14d > ... > Call Trace: > d_walk+0x28c/0x950 > ? dentry_lru_isolate+0x2b0/0x2b0 > ? __kasan_slab_free+0x12/0x20 > do_one_tree+0x33/0x60 > shrink_dcache_for_umount+0x78/0x1d0 > generic_shutdown_super+0x70/0x440 > kill_anon_super+0x3e/0x70 > deactivate_locked_super+0xc4/0x160 > deactivate_super+0xfa/0x140 > cleanup_mnt+0x22e/0x370 > __cleanup_mnt+0x1a/0x30 > task_work_run+0x139/0x210 > do_exit+0xb0c/0x2820 > ? __kasan_check_read+0x1d/0x30 > ? find_held_lock+0x35/0x160 > ? lock_release+0x1b6/0x660 > ? mm_update_next_owner+0xa20/0xa20 > ? reacquire_held_locks+0x3f0/0x3f0 > ? __sanitizer_cov_trace_const_cmp4+0x22/0x30 > do_group_exit+0x135/0x380 > __do_sys_exit_group.isra.0+0x20/0x20 > __x64_sys_exit_group+0x3c/0x50 > do_syscall_64+0x45/0x70 > entry_SYSCALL_64_after_hwframe+0x44/0xae > ... > VFS: Busy inodes after unmount of overlay. Self-destruct in 5 seconds. Have a nice day... > > This fix has been tested with a syzkaller reproducer. > Looks good to me. I realized that dentry leak will happen on underlying filesystem so unmount of underlying filesystem will give this warning. I created nested overlayfs configuration and could reproduce this error and tested that this patch fixes it. Reviewed-by: Vivek Goyal <vgoyal@redhat.com> Vivek > Cc: Amir Goldstein <amir73il@gmail.com> > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Vivek Goyal <vgoyal@redhat.com> > Cc: <stable@vger.kernel.org> # v5.7+ > Reported-by: syzbot <syzkaller@googlegroups.com> > Fixes: 6815f479ca90 ("ovl: use only uppermetacopy state in ovl_lookup()") > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > Link: https://lore.kernel.org/r/20210329164907.2133175-1-mic@digikod.net > --- > fs/overlayfs/namei.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index 3fe05fb5d145..424c594afd79 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -921,6 +921,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) { > err = -EPERM; > pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry); > + dput(this); > goto out_put; > } > > > base-commit: a5e13c6df0e41702d2b2c77c8ad41677ebb065b3 > -- > 2.30.2 >
On Mon, Mar 29, 2021 at 6:48 PM Mickaël Salaün <mic@digikod.net> wrote: > > From: Mickaël Salaün <mic@linux.microsoft.com> > > Since commit 6815f479ca90 ("ovl: use only uppermetacopy state in > ovl_lookup()"), overlayfs doesn't put temporary dentry when there is a > metacopy error, which leads to dentry leaks when shutting down the > related superblock: > > overlayfs: refusing to follow metacopy origin for (/file0) > ... > BUG: Dentry (____ptrval____){i=3f33,n=file3} still in use (1) [unmount of overlay overlay] > ... > WARNING: CPU: 1 PID: 432 at umount_check.cold+0x107/0x14d > CPU: 1 PID: 432 Comm: unmount-overlay Not tainted 5.12.0-rc5 #1 > ... > RIP: 0010:umount_check.cold+0x107/0x14d > ... > Call Trace: > d_walk+0x28c/0x950 > ? dentry_lru_isolate+0x2b0/0x2b0 > ? __kasan_slab_free+0x12/0x20 > do_one_tree+0x33/0x60 > shrink_dcache_for_umount+0x78/0x1d0 > generic_shutdown_super+0x70/0x440 > kill_anon_super+0x3e/0x70 > deactivate_locked_super+0xc4/0x160 > deactivate_super+0xfa/0x140 > cleanup_mnt+0x22e/0x370 > __cleanup_mnt+0x1a/0x30 > task_work_run+0x139/0x210 > do_exit+0xb0c/0x2820 > ? __kasan_check_read+0x1d/0x30 > ? find_held_lock+0x35/0x160 > ? lock_release+0x1b6/0x660 > ? mm_update_next_owner+0xa20/0xa20 > ? reacquire_held_locks+0x3f0/0x3f0 > ? __sanitizer_cov_trace_const_cmp4+0x22/0x30 > do_group_exit+0x135/0x380 > __do_sys_exit_group.isra.0+0x20/0x20 > __x64_sys_exit_group+0x3c/0x50 > do_syscall_64+0x45/0x70 > entry_SYSCALL_64_after_hwframe+0x44/0xae > ... > VFS: Busy inodes after unmount of overlay. Self-destruct in 5 seconds. Have a nice day... > > This fix has been tested with a syzkaller reproducer. > > Cc: Amir Goldstein <amir73il@gmail.com> > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Vivek Goyal <vgoyal@redhat.com> > Cc: <stable@vger.kernel.org> # v5.7+ > Reported-by: syzbot <syzkaller@googlegroups.com> > Fixes: 6815f479ca90 ("ovl: use only uppermetacopy state in ovl_lookup()") > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > Link: https://lore.kernel.org/r/20210329164907.2133175-1-mic@digikod.net Thanks, applied. Miklos
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 3fe05fb5d145..424c594afd79 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -921,6 +921,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) { err = -EPERM; pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry); + dput(this); goto out_put; }