-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 08/26/13 08:35, Andriy Gapon wrote: > on 26/08/2013 01:15 Jeremie Le Hen said the following: >> Hi Xin, >> >> On Tue, Aug 20, 2013 at 10:31:14PM +0000, Xin LI wrote: >>> Author: delphij Date: Tue Aug 20 22:31:13 2013 New Revision: >>> 254585 URL: http://svnweb.freebsd.org/changeset/base/254585 >>> >>> Log: MFV r254220: >>> >>> Illumos ZFS issues: 4039 zfs_rename()/zfs_link() needs stronger >>> test for XDEV >>> >>> Modified: >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c >>> >>> Directory Properties: >>> head/sys/cddl/contrib/opensolaris/ (props changed) >>> >>> Modified: >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c >>> >>> ============================================================================== >>> --- >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c >>> Tue Aug 20 21:47:07 2013 (r254584) +++ >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c >>> Tue Aug 20 22:31:13 2013 (r254585) @@ -21,6 +21,7 @@ /* * >>> Copyright (c) 2005, 2010, Oracle and/or its affiliates. All >>> rights reserved. * Copyright (c) 2013 by Delphix. All rights >>> reserved. + * Copyright 2013 Nexenta Systems, Inc. All rights >>> reserved. */ >>> >>> /* Portions Copyright 2007 Jeremy Teo */ @@ -3727,13 +3728,18 >>> @@ zfs_rename(vnode_t *sdvp, char *snm, vno if >>> (VOP_REALVP(tdvp, &realvp, ct) == 0) tdvp = realvp; >>> >>> - if (tdvp->v_vfsp != sdvp->v_vfsp || zfsctl_is_node(tdvp)) { + >>> tdzp = VTOZ(tdvp); > > The problem with this change, at least on FreeBSD, is that tdvp may > not belong to ZFS. In that case VTOZ(tdvp) does not make any sense > and would produce garbage or trigger an assert. Previously > tdvp->v_vfsp != sdvp->v_vfsp check would catch that situations. Two > side notes: - v_vfsp is actually v_mount on FreeBSD
Ah that's good point. Any objection in putting a change to their _freebsd_ counterpart (zfs_freebsd_rename and zfs_freebsd_link) as attached? > - VOP_REALVP is a glorified nop on FreeBSD It's not clear to me what was the intention for having a macro instead of just ifdef'ing the code out -- maybe to prevent unwanted conflict with upstream? These two VOP's are the only consumers of VOP_REALVP in tree. > Another unrelated problem that existed before this change and has > been noted by Davide Italiano is that sdvp is not locked and so it > can potentially be recycled before ZFS_ENTER() enter and for that > reason sdzp and zfsvfs could be invalid. Because sdvp is > referenced, this problem can currently occur only if a forced > unmount runs concurrently to zfs_rename. tdvp should be locked and > thus could be used instead of sdvp as a source for zfsvfs. I think this would need more work, I'll take a look after we have this regression fixed. Cheers, - -- Xin LI <delp...@delphij.net> https://www.delphij.net/ FreeBSD - The Power to Serve! Live free or die -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.21 (FreeBSD) iQEcBAEBCgAGBQJSG96rAAoJEG80Jeu8UPuzQG4IAK/Qw1McLNoy0egEzelYcsar iBRwoGDXfJuufCy04TEXD5rEz78VdqOl+g0tFqhSMbKHzQj+qEa6P6DIKptEnSsW AtQOQABs0gHY4SZ3MUdvdlEmFlWtyYPTqw471k2jIjRMNEM3wyslVn/SHvfymmwT s9VTI40jkoHWCUMW217jvER5co/niQDU4QL9ZNPb8vzRT02obqiq7ugZ7eqgklAI zqzB46Trn6Oplab+vNt/dWgSK/cuPwDaeTNeRBiw2YQ/uQMsOEdNPB2JqLUA5XgF WezHnotyFT/vdiQCe6dHjatOaR5ui7qWTUKTAwcq4gUrLJQx9FYYV3Im9xmesSM= =FjK7 -----END PGP SIGNATURE-----
Index: sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c =================================================================== --- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c (revision 254924) +++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c (working copy) @@ -6250,6 +6250,9 @@ zfs_freebsd_rename(ap) ASSERT(ap->a_fcnp->cn_flags & (SAVENAME|SAVESTART)); ASSERT(ap->a_tcnp->cn_flags & (SAVENAME|SAVESTART)); + if (fdvp->v_mount != tdvp->v_mount) + return (EXDEV); + error = zfs_rename(fdvp, ap->a_fcnp->cn_nameptr, tdvp, ap->a_tcnp->cn_nameptr, ap->a_fcnp->cn_cred, NULL, 0); @@ -6308,10 +6311,15 @@ zfs_freebsd_link(ap) } */ *ap; { struct componentname *cnp = ap->a_cnp; + vnode_t *vp = ap->a_vp; + vnode_t *tdvp = ap->a_tdvp; + if (tdvp->v_mount != vp->v_mount) + return (EXDEV); + ASSERT(cnp->cn_flags & SAVENAME); - return (zfs_link(ap->a_tdvp, ap->a_vp, cnp->cn_nameptr, cnp->cn_cred, NULL, 0)); + return (zfs_link(tdvp, vp, cnp->cn_nameptr, cnp->cn_cred, NULL, 0)); } static int
_______________________________________________ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"