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

[Davide Italiano]

On Tue, Aug 27, 2013 at 10:00 AM, Andriy Gapon <avg at freebsd.org> wrote:
> 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

I've written a patch that attempts to fix the second problem.
http://people.freebsd.org/~davide/review/zfsrename_recycle.diff
The culprit is that we're dereferencing sdvp without the vnode lock
held, so data-stability is not guaranteed.
tdvp, instead, is locked at the entry point of VOP_RENAME() (at least
according to vop_rename_pre()) so it can be safely used.
As pointed out by Andriy ZFS has some different mechanisms wrt other
file systems that allow the vnode to not be recycled when it's
referenced (ZFS_ENTER), so once we get zfsvfs_t * from tdzp we can
call ZFS_ENTER and dereference sdvp in a safe manner.

Thanks,

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare