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

[Andriy Gapon]

on 27/08/2013 02:03 Xin Li said the following:
> 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:
[snip]
>>>> @@ 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?

I think that at least the change to zfs_freebsd_rename as it is now would break
locking and reference counting semantics for the vnodes involved -- vreles and
vputs have to be done even in the error case.

Also, look at the check at the start of ufs_rename, it also considers tvp when
it's not NULL.  I am not sure if it is possible to have a situation where fvp
and tdvp are from the same fs, but tvp is from a different one.

I've been also tempted to place the check in the common code in kern_renameat.
That should cover the system calls.  But could be insufficient for direct calls
to VOP_RENAME in other places.

diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index a7cb87a..cfa4d93 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -3608,6 +3608,14 @@ kern_renameat(struct thread *td, int oldfd, char *old,
int newfd, char *new,
 		error = EINVAL;
 		goto out;
 	}
+
+	/* Check for cross-device rename. */
+	if ((fvp->v_mount != tdvp->v_mount) ||
+	    (tvp && (fvp->v_mount != tvp->v_mount))) {
+		error = EXDEV;
+		goto out;
+	}
+
 	/*
 	 * If the source is the same as the destination (that is, if they
 	 * are links to the same vnode), then there is nothing to do.

>> - 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.

Yes.  Personally I would just ifdef out the calls.

>> 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.

Thank you.

-- 
Andriy Gapon