| Message ID | 20210520154654.1791183-2-groug@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofs: propagate sync() to file server | expand |
On Thu, May 20, 2021 at 05:46:50PM +0200, Greg Kurz wrote:
> Some rollback was forgotten during the addition of crossmounts.
Have you actually tested that? Because I strongly suspect that
by that point the ownership of fc and fm is with sb and those
should be taken care of by deactivate_locked_super().
On Thu, 20 May 2021 at 21:45, Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Thu, May 20, 2021 at 05:46:50PM +0200, Greg Kurz wrote: > > Some rollback was forgotten during the addition of crossmounts. > > Have you actually tested that? Because I strongly suspect that > by that point the ownership of fc and fm is with sb and those > should be taken care of by deactivate_locked_super(). Not quite. Patch looks correct because destruction of fm is done in fuse_put_super(), which only gets called if the sb initialization gets as far as setting up sb->s_root, which only happens after the successful fuse_fill_super_submount() call in this case. Doing the destruction from the various ->kill_sb() instances instead of from ->put_super() would also fix this, but I'm not quite sure that that would be any cleaner. Thanks, Miklos
On Thu, 20 May 2021 19:45:13 +0000 Al Viro <viro@zeniv.linux.org.uk> wrote: > On Thu, May 20, 2021 at 05:46:50PM +0200, Greg Kurz wrote: > > Some rollback was forgotten during the addition of crossmounts. > > Have you actually tested that? Because I strongly suspect that > by that point the ownership of fc and fm is with sb and those > should be taken care of by deactivate_locked_super(). My bad, I didn't test but now I did and the issue is actually worse than just a memory leak. This error path crashes upstream without this patch: [ 26.206673] BUG: kernel NULL pointer dereference, address: 0000000000000000 [ 26.209560] #PF: supervisor read access in kernel mode [ 26.211699] #PF: error_code(0x0000) - not-present page [ 26.214574] PGD 0 P4D 0 [ 26.216016] Oops: 0000 [#1] SMP PTI [ 26.217451] CPU: 0 PID: 3380 Comm: ls Kdump: loaded Not tainted 5.13.0-virtio-fs-sync+ #30 [ 26.220839] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014 [ 26.226362] RIP: 0010:__list_del_entry_valid+0x25/0x90 [ 26.228449] Code: c3 0f 1f 40 00 48 8b 17 4c 8b 47 08 48 b8 00 01 00 00 00 00 ad de 48 39 c2 74 26 48 b8 22 01 00 00 00 00 ad de 49 39 c0 74 2b <49> 8b 30 48 39 fe 75 3a 48 8b 52 08 48 39 f2 75 48 b8 01 00 00 00 [ 26.234256] RSP: 0018:ffffaa37006cbb18 EFLAGS: 00010217 [ 26.235473] RAX: dead000000000122 RBX: ffff8f6844098200 RCX: 0000000000000000 [ 26.236922] RDX: 0000000000000000 RSI: ffffffff99264e92 RDI: ffff8f6844098210 [ 26.238401] RBP: ffff8f68420b3c00 R08: 0000000000000000 R09: 000000000000002a [ 26.239852] R10: 0000000000000000 R11: ffff8f6840402480 R12: ffff8f6844098210 [ 26.241160] R13: ffff8f68420b3da8 R14: ffff8f6844098200 R15: 0000000000000000 [ 26.242398] FS: 00007f547b93f200(0000) GS:ffff8f687bc00000(0000) knlGS:0000000000000000 [ 26.243698] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 26.244693] CR2: 0000000000000000 CR3: 0000000104e50000 CR4: 00000000000006f0 [ 26.245936] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 26.246961] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 26.247938] Call Trace: [ 26.248300] fuse_mount_remove+0x2c/0x70 [fuse] [ 26.248892] virtio_kill_sb+0x22/0x160 [virtiofs] [ 26.249487] deactivate_locked_super+0x36/0xa0 [ 26.250077] fuse_dentry_automount+0x178/0x1a0 [fuse] The crash happens because we're assuming fm was already added to fc->mounts... bool fuse_mount_remove(struct fuse_mount *fm) { struct fuse_conn *fc = fm->fc; bool last = false; down_write(&fc->killsb); list_del_init(&fm->fc_entry); <=== HERE if (list_empty(&fc->mounts)) last = true; up_write(&fc->killsb); return last; } but fm is added to fc->mounts much later after the superblock is fully configured. Looking again at what is done for the root mount in virtio_fs_get_tree(), I now realize sb->s_fs_info is used as a flag to decide whether fuse_mount_remove() should be called: static int virtio_fs_get_tree(struct fs_context *fsc) { ... if (!sb->s_root) { err = virtio_fs_fill_super(sb, fsc); if (err) { fuse_conn_put(fc); kfree(fm); CLEARED HERE => sb->s_fs_info = NULL; deactivate_locked_super(sb); return err; } sb->s_flags |= SB_ACTIVE; } ... } static void virtio_kill_sb(struct super_block *sb) { struct fuse_mount *fm = get_fuse_mount_super(sb); I.E. sb->s_fs_info bool last; /* If mount failed, we can still be called without any fc */ if (fm) { TESTED HERE ^^ last = fuse_mount_remove(fm); if (last) virtio_fs_conn_destroy(fm); } kill_anon_super(sb); } The natural fix is to do the same in the automount case : take back the ownership on fm by clearing sb->s_fs_info, which thus implies to do the freeing.
On Fri, 21 May 2021 09:54:19 +0200 Miklos Szeredi <miklos@szeredi.hu> wrote: > On Thu, 20 May 2021 at 21:45, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > On Thu, May 20, 2021 at 05:46:50PM +0200, Greg Kurz wrote: > > > Some rollback was forgotten during the addition of crossmounts. > > > > Have you actually tested that? Because I strongly suspect that > > by that point the ownership of fc and fm is with sb and those > > should be taken care of by deactivate_locked_super(). > > Not quite. Patch looks correct because destruction of fm is done in > fuse_put_super(), which only gets called if the sb initialization gets > as far as setting up sb->s_root, which only happens after the > successful fuse_fill_super_submount() call in this case. > > Doing the destruction from the various ->kill_sb() instances instead > of from ->put_super() would also fix this, but I'm not quite sure that > that would be any cleaner. > As saying in the answer I've just posted, a failure in fuse_fill_super_submount() causes an actual crash because fuse_mount_remove() logically assumes fm to already be in fc->mounts, which isn't the case at this point. In the root mount case, this is handled by taking back the ownership on fm, i.e. do the rollback *and* clear sb->s_fs_info. It seems that the same should be done for submounts. > Thanks, > Miklos
On Fri, 21 May 2021 at 10:15, Greg Kurz <groug@kaod.org> wrote: > > On Fri, 21 May 2021 09:54:19 +0200 > Miklos Szeredi <miklos@szeredi.hu> wrote: > > > On Thu, 20 May 2021 at 21:45, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > > > On Thu, May 20, 2021 at 05:46:50PM +0200, Greg Kurz wrote: > > > > Some rollback was forgotten during the addition of crossmounts. > > > > > > Have you actually tested that? Because I strongly suspect that > > > by that point the ownership of fc and fm is with sb and those > > > should be taken care of by deactivate_locked_super(). > > > > Not quite. Patch looks correct because destruction of fm is done in > > fuse_put_super(), which only gets called if the sb initialization gets > > as far as setting up sb->s_root, which only happens after the > > successful fuse_fill_super_submount() call in this case. > > > > Doing the destruction from the various ->kill_sb() instances instead > > of from ->put_super() would also fix this, but I'm not quite sure that > > that would be any cleaner. > > > > As saying in the answer I've just posted, a failure in > fuse_fill_super_submount() causes an actual crash because > fuse_mount_remove() logically assumes fm to already be in > fc->mounts, which isn't the case at this point. > > In the root mount case, this is handled by taking back > the ownership on fm, i.e. do the rollback *and* clear > sb->s_fs_info. It seems that the same should be done > for submounts. Agreed. Thanks for verifying. Miklos
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 1b6c001a7dd1..fb2af70596c3 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -339,8 +339,11 @@ static struct vfsmount *fuse_dentry_automount(struct path *path) /* Initialize superblock, making @mp_fi its root */ err = fuse_fill_super_submount(sb, mp_fi); - if (err) + if (err) { + fuse_conn_put(fc); + kfree(fm); goto out_put_sb; + } sb->s_flags |= SB_ACTIVE; fsc->root = dget(sb->s_root);
Some rollback was forgotten during the addition of crossmounts. Fixes: bf109c64040f ("fuse: implement crossmounts") Cc: mreitz@redhat.com Signed-off-by: Greg Kurz <groug@kaod.org> --- fs/fuse/dir.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)