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

[Andriy Gapon]

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
- VOP_REALVP is a glorified nop on FreeBSD

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 suspect this leads to a panic on my -CURRENT machine:
> 
> 
> #1  0xffffffff8033a664 in db_fncall_generic (addr=-2138557936,
>     rv=0xfffffe00e5a30c90, nargs=0, args=0xfffffe00e5a30ca0)
>     at /usr/src/sys/ddb/db_command.c:578
> #2  0xffffffff8033a34a in db_fncall (dummy1=-2138210939, dummy2=0, dummy3=-1,
>     dummy4=0xfffffe00e5a30d80 "") at /usr/src/sys/ddb/db_command.c:630
> #3  0xffffffff80339fe1 in db_command (last_cmdp=0xffffffff812b7850,
>     cmd_table=0x0, dopager=0) at /usr/src/sys/ddb/db_command.c:449
> #4  0xffffffff8033a0c2 in db_command_script (
>     command=0xffffffff812b8754 "call doadump")
>     at /usr/src/sys/ddb/db_command.c:520
> #5  0xffffffff80340b09 in db_script_exec (
>     scriptname=0xfffffe00e5a30f50 "kdb.enter.panic", warnifnotfound=0)
>     at /usr/src/sys/ddb/db_script.c:302
> #6  0xffffffff8034096c in db_script_kdbenter (
>     eventname=0xffffffff80f7cd55 "panic") at /usr/src/sys/ddb/db_script.c:324
> #7  0xffffffff8033dee1 in db_trap (type=3, code=0)
>     at /usr/src/sys/ddb/db_main.c:230
> #8  0xffffffff808d84b6 in kdb_trap (type=3, code=0, tf=0xfffffe00e5a31390)
>     at /usr/src/sys/kern/subr_kdb.c:654
> #9  0xffffffff80dbc22a in trap (frame=0xfffffe00e5a31390)
>     at /usr/src/sys/amd64/amd64/trap.c:579
> #10 0xffffffff80d99282 in calltrap ()
>     at /usr/src/sys/amd64/amd64/exception.S:232
> #11 0xffffffff808d7d85 in breakpoint () at cpufunc.h:63
> #12 0xffffffff808d79eb in kdb_enter (why=0xffffffff80f7cd55 "panic",
>     msg=0xffffffff80f7cd55 "panic") at /usr/src/sys/kern/subr_kdb.c:445
> #13 0xffffffff808838b3 in vpanic (
>     fmt=0xffffffff81b05389 "solaris assert: %s, file: %s, line: %d",
>     ap=0xfffffe00e5a31530) at /usr/src/sys/kern/kern_shutdown.c:747
> #14 0xffffffff80883960 in panic (
>     fmt=0xffffffff81b05389 "solaris assert: %s, file: %s, line: %d")
>     at /usr/src/sys/kern/kern_shutdown.c:683
> #15 0xffffffff81b0443c in assfail (
>     a=0xffffffff819ab3cc "zp == NULL || zp->z_vnode == NULL || zp->z_vnode == vp",
>     f=0xffffffff819ab403 "/usr/src/sys/modules/zfs/../../cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_znode.h", l=248)
>     at /usr/src/sys/modules/opensolaris/../../cddl/compat/opensolaris/kern/opensolaris_cmn_err.c:81
> #16 0xffffffff8191b111 in VTOZ (vp=0xfffff800967f5b10) at zfs_znode.h:248
> #17 0xffffffff8192372c in zfs_rename (sdvp=0xfffff800911bf3b0,
>     snm=0xfffff80004a9d806 "patchoHq2mGI", tdvp=0xfffff800967f5b10,
>     tnm=0xfffff8000451780f "periodic.conf.5", cr=0xfffff80011d4ce00, ct=0x0,
>     flags=0)
>     at /usr/src/sys/modules/zfs/../../cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c:3731
> #18 0xffffffff8191ec06 in zfs_freebsd_rename (ap=0xfffffe00e5a317a8)
>     at /usr/src/sys/modules/zfs/../../cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c:6253
> #19 0xffffffff80eb67be in VOP_RENAME_APV (vop=0xffffffff819b5f80,
>     a=0xfffffe00e5a317a8) at vnode_if.c:1546
> #20 0xffffffff8099db19 in VOP_RENAME (fdvp=0xfffff800911bf3b0, 
>     fvp=0xfffff800910831d8, fcnp=0xfffffe00e5a31990, tdvp=0xfffff800967f5b10, 
>     tvp=0x0, tcnp=0xfffffe00e5a318d0) at vnode_if.h:636
> #21 0xffffffff8099d99e in kern_renameat (td=0xfffff8001c0db920, oldfd=-100, 
>     old=0x800c4a040 <Address 0x800c4a040 out of bounds>, newfd=-100, 
>     new=0x800c4a0c0 <Address 0x800c4a0c0 out of bounds>, 
>     pathseg=UIO_USERSPACE) at /usr/src/sys/kern/vfs_syscalls.c:3558
> #22 0xffffffff8099d533 in kern_rename (td=0xfffff8001c0db920, 
>     from=0x800c4a040 <Address 0x800c4a040 out of bounds>, 
>     to=0x800c4a0c0 <Address 0x800c4a0c0 out of bounds>, pathseg=UIO_USERSPACE)
>     at /usr/src/sys/kern/vfs_syscalls.c:3465
> #23 0xffffffff8099d4fa in sys_rename (td=0xfffff8001c0db920, 
>     uap=0xfffffe00e5a31b98) at /usr/src/sys/kern/vfs_syscalls.c:3442
> #24 0xffffffff80dbddce in syscallenter (td=0xfffff8001c0db920, 
>     sa=0xfffffe00e5a31b88) at subr_syscall.c:134
> #25 0xffffffff80dbd78f in amd64_syscall (td=0xfffff8001c0db920, traced=0)
>     at /usr/src/sys/amd64/amd64/trap.c:974
> #26 0xffffffff80d9956b in Xfast_syscall ()
>     at /usr/src/sys/amd64/amd64/exception.S:391
> #27 0x00000008008826ac in ?? ()
> 
> 


-- 
Andriy Gapon