Date: Tue, 27 Aug 2013 11:00:44 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: d@delphij.net Cc: src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, Xin LI <delphij@FreeBSD.org>, svn-src-head@FreeBSD.org, Konstantin Belousov <kib@FreeBSD.org>, Xin Li <delphij@delphij.net> Subject: Re: svn commit: r254585 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs Message-ID: <521C5CAC.2060400@FreeBSD.org> In-Reply-To: <521BDEAC.9080909@delphij.net> References: <201308202231.r7KMVERi068300@svn.freebsd.org> <20130825221517.GM24767@caravan.chchile.org> <521B75CE.70103@FreeBSD.org> <521BDEAC.9080909@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?521C5CAC.2060400>