Aren
/
NetBSD
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix a problem noticed by Reinoud: the fs would try to release an 

unlocked vnode when trying to rename a directory.  The fix was to
shuffle some bits around and #pray.

The rename routine actually needs a very very major wide-angle whopping:
 * it takes locks out-of-order
 * it deals with references from SAVESTART lookups in interesting ways
 * I doubt there is any guarantee for correct operation if there
   are multiple concurrent accesses
 * the error branches might just as well call panic() directly
This commit is contained in:
pooka 2007-11-14 19:16:29 +00:00
parent 276f1a3cde
commit 4a0a4d4f30
1 changed files with 44 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
sys/fs/msdosfs/msdosfs_vnops.c
View File

@ -1,4 +1,4 @@
/* $NetBSD: msdosfs_vnops.c,v 1.42 2007/10/08 18:04:04 ad Exp $ */
/* $NetBSD: msdosfs_vnops.c,v 1.43 2007/11/14 19:16:29 pooka Exp $ */
/*-
* Copyright (C) 1994, 1995, 1997 Wolfgang Solfrank.
@ -48,7 +48,7 @@
*/
#include <sys/cdefs.h>
__KERNEL_RCSID(0, "$NetBSD: msdosfs_vnops.c,v 1.42 2007/10/08 18:04:04 ad Exp $");
__KERNEL_RCSID(0, "$NetBSD: msdosfs_vnops.c,v 1.43 2007/11/14 19:16:29 pooka Exp $");
#include <sys/param.h>
#include <sys/systm.h>
@ -834,6 +834,9 @@ msdosfs_link(v)
* I'm not sure how the memory containing the pathnames pointed at by the
* componentname structures is freed, there may be some memory bleeding
* for each rename done.
*
* --More-- Notes:
* This routine needs help. badly.
*/
int
msdosfs_rename(v)
@ -902,7 +905,11 @@ abortit:
goto abortit;
}
/* */
/*
* XXX: This can deadlock since we hold tdvp/tvp locked.
* But I'm not going to fix it now. If lockmgr detected the
* deadlock, this might actually work... sorta.
*/
if ((error = vn_lock(fvp, LK_EXCLUSIVE)) != 0)
goto abortit;
dp = VTODE(fdvp);
@ -958,12 +965,23 @@ abortit:
VOP_UNLOCK(fvp, 0);
if (VTODE(fdvp)->de_StartCluster != VTODE(tdvp)->de_StartCluster)
newparent = 1;
/*
* XXX: We can do this here because rename uses SAVEFART and
* therefore fdvp has at least two references (one doesn't
* belong to us, though, and that's evil). We'll get
* another "extra" reference when we do relookup(), so we
* need to compensate. We should *NOT* be doing this, but
* it works, so whatever.
*/
vrele(fdvp);
if (doingdirectory && newparent) {
if (error) /* write access check above */
goto bad;
goto tdvpbad;
if (xp != NULL)
vput(tvp);
tvp = NULL;
/*
* doscheckpath() vput()'s dp,
* so we have to do a relookup afterwards
@ -972,8 +990,15 @@ abortit:
goto out;
if ((tcnp->cn_flags & SAVESTART) == 0)
panic("msdosfs_rename: lost to startdir");
if ((error = relookup(tdvp, &tvp, tcnp)) != 0)
vn_lock(tdvp, LK_EXCLUSIVE | LK_RETRY);
if ((error = relookup(tdvp, &tvp, tcnp)) != 0) {
VOP_UNLOCK(tdvp, 0);
goto out;
}
/*
* XXX: SAVESTART causes us to get a reference, but
* that's released already above in doscheckpath()
*/
dp = VTODE(tdvp);
xp = tvp ? VTODE(tvp) : NULL;
}
@ -987,22 +1012,23 @@ abortit:
if (xp->de_Attributes & ATTR_DIRECTORY) {
if (!dosdirempty(xp)) {
error = ENOTEMPTY;
goto bad;
goto tdvpbad;
}
if (!doingdirectory) {
error = ENOTDIR;
goto bad;
goto tdvpbad;
}
} else if (doingdirectory) {
error = EISDIR;
goto bad;
goto tdvpbad;
}
if ((error = removede(dp, xp)) != 0)
goto bad;
goto tdvpbad;
VN_KNOTE(tdvp, NOTE_WRITE);
VN_KNOTE(tvp, NOTE_DELETE);
cache_purge(tvp);
vput(tvp);
tvp = NULL;
xp = NULL;
}
@ -1022,11 +1048,10 @@ abortit:
fcnp->cn_flags |= LOCKPARENT | LOCKLEAF;
if ((fcnp->cn_flags & SAVESTART) == 0)
panic("msdosfs_rename: lost from startdir");
if (!newparent)
VOP_UNLOCK(tdvp, 0);
VOP_UNLOCK(tdvp, 0);
vn_lock(fdvp, LK_EXCLUSIVE | LK_RETRY);
if ((error = relookup(fdvp, &fvp, fcnp))) {
vput(fdvp);
VOP_UNLOCK(fdvp, 0);
vrele(ap->a_fvp);
vrele(tdvp);
return (error);
@ -1043,6 +1068,7 @@ abortit:
return 0;
}
fdvp_dorele = 1;
VOP_UNLOCK(fdvp, 0);
xp = VTODE(fvp);
zp = VTODE(fdvp);
from_diroffset = zp->de_fndoffset;
@ -1060,8 +1086,6 @@ abortit:
panic("rename: lost dir entry");
vrele(ap->a_fvp);
VOP_UNLOCK(fvp, 0);
if (newparent)
VOP_UNLOCK(fdvp, 0);
xp = NULL;
} else {
vrele(fvp);
@ -1082,8 +1106,6 @@ abortit:
error = createde(ip, dp, (struct denode **)0, tcnp);
if (error) {
memcpy(ip->de_Name, oldname, 11);
if (newparent)
VOP_UNLOCK(fdvp, 0);
VOP_UNLOCK(fvp, 0);
goto bad;
}
@ -1091,8 +1113,6 @@ abortit:
zp->de_fndoffset = from_diroffset;
if ((error = removede(zp, ip)) != 0) {
/* XXX should really panic here, fs is corrupt */
if (newparent)
VOP_UNLOCK(fdvp, 0);
VOP_UNLOCK(fvp, 0);
goto bad;
}
@ -1102,8 +1122,6 @@ abortit:
&ip->de_dirclust, 0);
if (error) {
/* XXX should really panic here, fs is corrupt */
if (newparent)
VOP_UNLOCK(fdvp, 0);
VOP_UNLOCK(fvp, 0);
goto bad;
}
@ -1112,8 +1130,6 @@ abortit:
ip->de_diroffset &= pmp->pm_crbomask;
}
reinsert(ip);
if (newparent)
VOP_UNLOCK(fdvp, 0);
}
/*
@ -1153,9 +1169,9 @@ abortit:
VN_KNOTE(fvp, NOTE_RENAME);
VOP_UNLOCK(fvp, 0);
bad:
if (xp)
if (tvp)
vput(tvp);
vput(tdvp);
vrele(tdvp);
out:
ip->de_flag &= ~DE_RENAME;
if (fdvp_dorele)
@ -1163,6 +1179,10 @@ out:
vrele(fvp);
return (error);
/* XXX: uuuh */
tdvpbad:
VOP_UNLOCK(tdvp, 0);
goto bad;
}
static const struct {