From b26cc897197f1e7a355569a2a6890de4fe451877 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 12 Oct 2022 15:11:52 +0000 Subject: [PATCH] zfs_domount: fix double-disown of dataset / double-free of zfsvfs_t Before this patch, in zfs_domount, if zfs_root or d_make_root fails, we leave zfsvfs != NULL. This will lead to execution of the error handling `if` statement at the `out` label, and hence to a call to dmu_objset_disown and zfsvfs_free. However, zfs_umount, which we call upon failure of zfs_root and d_make_root already does dmu_objset_disown and zfsvfs_free. I suppose this patch rather adds to the brittleness of this part of the code base, but I don't want to invest more time in this right now. To add a regression test, we'd need some kind of fault injection facitility for zfs_root or d_make_root, which doesn't exist right now. And even then, I think that regression test would be too closely tied to the implementation. To repro the double-disown / double-free, do the following: 1. patch zfs_root to always return an error 2. mount a ZFS filesystem Here's the stack trace you would see then: VERIFY3(ds->ds_owner == tag) failed (0000000000000000 == ffff9142361e8000) PANIC at dsl_dataset.c:1003:dsl_dataset_disown() Showing stack for process 28332 CPU: 2 PID: 28332 Comm: zpool Tainted: G O 5.10.103-1.nutanix.el7.x86_64 #1 Call Trace: dump_stack+0x74/0x92 spl_dumpstack+0x29/0x2b [spl] spl_panic+0xd4/0xfc [spl] ? find_next_bit+0x14/0x20 ? __free_pages+0x77/0x80 ? kfree+0x3a6/0x450 ? percpu_counter_destroy+0x6a/0x80 dsl_dataset_disown+0xe9/0x150 [zfs] dmu_objset_disown+0xd6/0x150 [zfs] zfs_domount+0x17b/0x4b0 [zfs] zpl_mount+0x174/0x220 [zfs] legacy_get_tree+0x2b/0x50 vfs_get_tree+0x2a/0xc0 ? ns_capable+0x10/0x20 path_mount+0x2fa/0xa70 do_mount+0x7c/0xa0 __x64_sys_mount+0x8b/0xe0 do_syscall_64+0x38/0x50 entry_SYSCALL_64_after_hwframe+0x44/0xa9 Signed-off-by: Christian Schwarz --- module/os/linux/zfs/zfs_vfsops.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/module/os/linux/zfs/zfs_vfsops.c b/module/os/linux/zfs/zfs_vfsops.c index 64d6b4616e1c..cf3e3ef8ffcf 100644 --- a/module/os/linux/zfs/zfs_vfsops.c +++ b/module/os/linux/zfs/zfs_vfsops.c @@ -1555,14 +1555,18 @@ zfs_domount(struct super_block *sb, zfs_mnt_t *zm, int silent) /* Allocate a root inode for the filesystem. */ error = zfs_root(zfsvfs, &root_inode); if (error) { + VERIFY3P(zfsvfs->z_os, !=, NULL); (void) zfs_umount(sb); + zfsvfs = NULL; /* avoid double-free; first in zfs_unmount */ goto out; } /* Allocate a root dentry for the filesystem */ sb->s_root = d_make_root(root_inode); if (sb->s_root == NULL) { + VERIFY3P(zfsvfs->z_os, !=, NULL); (void) zfs_umount(sb); + zfsvfs = NULL; /* avoid double-free; first in zfs_unmount */ error = SET_ERROR(ENOMEM); goto out; }