svn commit: r254585 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs

[Xin Li]

-----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 <delphij at 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-----
-------------- next part --------------
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