Date: Tue, 22 Nov 2016 11:47:27 +0000 (UTC) From: Andriy Gapon <avg@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-vendor@freebsd.org Subject: svn commit: r308987 - in vendor-sys/illumos/dist/uts/common/fs/zfs: . sys Message-ID: <201611221147.uAMBlRI0012310@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: avg Date: Tue Nov 22 11:47:27 2016 New Revision: 308987 URL: https://svnweb.freebsd.org/changeset/base/308987 Log: 7180 potential race between zfs_suspend_fs+zfs_resume_fs and zfs_ioc_rename illumos/illumos-gate@690041b9caf801816f2d0bac90bc7cecefb73523 https://github.com/illumos/illumos-gate/commit/690041b9caf801816f2d0bac90bc7cecefb73523 https://www.illumos.org/issues/7180 If a filesystem is not unmounted while the rename is being performed, then, for example, a concurrect zfs rollback may call zfs_suspend_fs followed by zfs_resume_fs on the same filesystem. The latter takes the filesystem's name as an argument. If the filesystem name changes as a result of the rename, then dmu_objset_hold(osname, zfsvfs, &os) call in zfs_resume_fs would fail resulting in a kernel panic. So far I have been able to reproduce this problem on FreeBSD where zfs rename has -u option that skips the unmounting before doing the renaming. But I think that in theory the same problem can occur on illumos as well, because the unmounting is done in userland before invoking the rename ioctl and there could be a race with, e.g., zfs mount. panic: solaris assert: dmu_objset_hold(osname, zfsvfs, &zfsvfs->z_os) == 0 (0x2 == 0x0), file: /usr/devel/svn/head/sys/cddl/contrib/opensolaris/uts/common/fs/ zfs/zfs_vfsops.c, line: 2210 KDB: stack backtrace: db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe004df30710 vpanic() at vpanic+0x182/frame 0xfffffe004df30790 panic() at panic+0x43/frame 0xfffffe004df307f0 assfail3() at assfail3+0x2c/frame 0xfffffe004df30810 zfs_resume_fs() at zfs_resume_fs+0xb9/frame 0xfffffe004df30860 zfs_ioc_rollback() at zfs_ioc_rollback+0x61/frame 0xfffffe004df308a0 zfsdev_ioctl() at zfsdev_ioctl+0x65c/frame 0xfffffe004df30940 devfs_ioctl_f() at devfs_ioctl_f+0x156/frame 0xfffffe004df309a0 kern_ioctl() at kern_ioctl+0x246/frame 0xfffffe004df30a00 sys_ioctl() at sys_ioctl+0x171/frame 0xfffffe004df30ae0 amd64_syscall() at amd64_syscall+0x2db/frame 0xfffffe004df30bf0 Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe004df30bf0 Reviewed by: Matt Ahrens <mahrens@delphix.com> Reviewed by: Pavel Zakharov <pavel.zakharov@delphix.com> Approved by: Richard Lowe <richlowe@richlowe.net> Author: Andriy Gapon <andriy.gapon@clusterhq.com> Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_vfsops.h vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_ioctl.c vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_vfsops.c Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_vfsops.h ============================================================================== --- vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_vfsops.h Tue Nov 22 11:46:22 2016 (r308986) +++ vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_vfsops.h Tue Nov 22 11:47:27 2016 (r308987) @@ -137,7 +137,7 @@ typedef struct zfid_long { extern uint_t zfs_fsyncer_key; extern int zfs_suspend_fs(zfsvfs_t *zfsvfs); -extern int zfs_resume_fs(zfsvfs_t *zfsvfs, const char *osname); +extern int zfs_resume_fs(zfsvfs_t *zfsvfs, struct dsl_dataset *ds); extern int zfs_userspace_one(zfsvfs_t *zfsvfs, zfs_userquota_prop_t type, const char *domain, uint64_t rid, uint64_t *valuep); extern int zfs_userspace_many(zfsvfs_t *zfsvfs, zfs_userquota_prop_t type, Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_ioctl.c ============================================================================== --- vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_ioctl.c Tue Nov 22 11:46:22 2016 (r308986) +++ vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_ioctl.c Tue Nov 22 11:47:27 2016 (r308987) @@ -3645,12 +3645,15 @@ zfs_ioc_rollback(const char *fsname, nvl int error; if (getzfsvfs(fsname, &zfsvfs) == 0) { + dsl_dataset_t *ds; + + ds = dmu_objset_ds(zfsvfs->z_os); error = zfs_suspend_fs(zfsvfs); if (error == 0) { int resume_err; error = dsl_dataset_rollback(fsname, zfsvfs, outnvl); - resume_err = zfs_resume_fs(zfsvfs, fsname); + resume_err = zfs_resume_fs(zfsvfs, ds); error = error ? error : resume_err; } VFS_RELE(zfsvfs->z_vfs); @@ -4275,8 +4278,10 @@ zfs_ioc_recv(zfs_cmd_t *zc) if (getzfsvfs(tofs, &zfsvfs) == 0) { /* online recv */ + dsl_dataset_t *ds; int end_err; + ds = dmu_objset_ds(zfsvfs->z_os); error = zfs_suspend_fs(zfsvfs); /* * If the suspend fails, then the recv_end will @@ -4284,7 +4289,7 @@ zfs_ioc_recv(zfs_cmd_t *zc) */ end_err = dmu_recv_end(&drc, zfsvfs); if (error == 0) - error = zfs_resume_fs(zfsvfs, tofs); + error = zfs_resume_fs(zfsvfs, ds); error = error ? error : end_err; VFS_RELE(zfsvfs->z_vfs); } else { @@ -4808,11 +4813,14 @@ zfs_ioc_userspace_upgrade(zfs_cmd_t *zc) * objset needs to be closed & reopened (to grow the * objset_phys_t). Suspend/resume the fs will do that. */ + dsl_dataset_t *ds; + + ds = dmu_objset_ds(zfsvfs->z_os); error = zfs_suspend_fs(zfsvfs); if (error == 0) { dmu_objset_refresh_ownership(zfsvfs->z_os, zfsvfs); - error = zfs_resume_fs(zfsvfs, zc->zc_name); + error = zfs_resume_fs(zfsvfs, ds); } } if (error == 0) Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_vfsops.c ============================================================================== --- vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_vfsops.c Tue Nov 22 11:46:22 2016 (r308986) +++ vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_vfsops.c Tue Nov 22 11:47:27 2016 (r308987) @@ -2020,7 +2020,7 @@ zfs_suspend_fs(zfsvfs_t *zfsvfs) * zfsvfs, held, and long held on entry. */ int -zfs_resume_fs(zfsvfs_t *zfsvfs, const char *osname) +zfs_resume_fs(zfsvfs_t *zfsvfs, dsl_dataset_t *ds) { int err; znode_t *zp; @@ -2029,14 +2029,13 @@ zfs_resume_fs(zfsvfs_t *zfsvfs, const ch ASSERT(RW_WRITE_HELD(&zfsvfs->z_teardown_inactive_lock)); /* - * We already own this, so just hold and rele it to update the - * objset_t, as the one we had before may have been evicted. + * We already own this, so just update the objset_t, as the one we + * had before may have been evicted. */ objset_t *os; - VERIFY0(dmu_objset_hold(osname, zfsvfs, &os)); - VERIFY3P(os->os_dsl_dataset->ds_owner, ==, zfsvfs); - VERIFY(dsl_dataset_long_held(os->os_dsl_dataset)); - dmu_objset_rele(os, zfsvfs); + VERIFY3P(ds->ds_owner, ==, zfsvfs); + VERIFY(dsl_dataset_long_held(ds)); + VERIFY0(dmu_objset_from_ds(ds, &os)); err = zfsvfs_init(zfsvfs, os); if (err != 0)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201611221147.uAMBlRI0012310>