mirror of
https://github.com/freebsd/freebsd-src
synced 2024-07-08 20:26:31 +00:00
unionfs_rename: fix numerous locking issues
There are a few places in which unionfs_rename() accesses fvp's private data without holding the necessary lock/interlock. Moreover, the implementation completely fails to handle the case in which fdvp is not the same as tdvp; in this case it simply fails to lock fdvp at all. Finally, it locks fvp while potentially already holding tvp's lock, but makes no attempt to deal with possible LOR there. Fix this by optimistically using the vnode interlock to protect the short accesses to fdvp and fvp private data, sequentially. If a file copy or shadow directory creation is required to prepare the upper FS for the rename operation, the interlock must be dropped and fdvp/fvp locked as necessary. Additionally, use ERELOOKUP (as suggested by kib@) to simplify the locking logic and eliminate unionfs_relookup() calls for file-copy/ shadow-directory cases that require tdvp's lock to be dropped. Reviewed by: kib (earlier version), olce Tested by: pho Differential Revision: https://reviews.freebsd.org/D44788
This commit is contained in:
Jason A. Harmening
parent
993d1fad5b
commit
05e8ab627b
|
|
@ -1167,7 +1167,6 @@ unionfs_rename(struct vop_rename_args *ap)
|
|||
struct unionfs_mount *ump;
|
||||
struct unionfs_node *unp;
|
||||
int error;
|
||||
int needrelookup;
|
||||
|
||||
UNIONFS_INTERNAL_DEBUG("unionfs_rename: enter\n");
|
||||
|
||||
|
|
@ -1185,7 +1184,6 @@ unionfs_rename(struct vop_rename_args *ap)
|
|||
rfvp = fvp;
|
||||
rtdvp = tdvp;
|
||||
rtvp = tvp;
|
||||
needrelookup = 0;
|
||||
|
||||
/* check for cross device rename */
|
||||
if (fvp->v_mount != tdvp->v_mount ||
|
||||
|
|
@ -1201,58 +1199,114 @@ unionfs_rename(struct vop_rename_args *ap)
|
|||
if (fvp == tvp)
|
||||
goto unionfs_rename_abort;
|
||||
|
||||
/*
|
||||
* from/to vnode is unionfs node.
|
||||
*/
|
||||
|
||||
KASSERT_UNIONFS_VNODE(fdvp);
|
||||
KASSERT_UNIONFS_VNODE(fvp);
|
||||
KASSERT_UNIONFS_VNODE(tdvp);
|
||||
if (tvp != NULLVP)
|
||||
KASSERT_UNIONFS_VNODE(tvp);
|
||||
|
||||
if (fdvp != tdvp)
|
||||
VI_LOCK(fdvp);
|
||||
unp = VTOUNIONFS(fdvp);
|
||||
if (unp == NULL) {
|
||||
if (fdvp != tdvp)
|
||||
VI_UNLOCK(fdvp);
|
||||
error = ENOENT;
|
||||
goto unionfs_rename_abort;
|
||||
}
|
||||
#ifdef UNIONFS_IDBG_RENAME
|
||||
UNIONFS_INTERNAL_DEBUG("fdvp=%p, ufdvp=%p, lfdvp=%p\n",
|
||||
fdvp, unp->un_uppervp, unp->un_lowervp);
|
||||
#endif
|
||||
if (unp->un_uppervp == NULLVP) {
|
||||
error = ENODEV;
|
||||
} else {
|
||||
rfdvp = unp->un_uppervp;
|
||||
vref(rfdvp);
|
||||
}
|
||||
if (fdvp != tdvp)
|
||||
VI_UNLOCK(fdvp);
|
||||
if (error != 0)
|
||||
goto unionfs_rename_abort;
|
||||
|
||||
VI_LOCK(fvp);
|
||||
unp = VTOUNIONFS(fvp);
|
||||
if (unp == NULL) {
|
||||
VI_UNLOCK(fvp);
|
||||
error = ENOENT;
|
||||
goto unionfs_rename_abort;
|
||||
}
|
||||
rfdvp = unp->un_uppervp;
|
||||
vref(rfdvp);
|
||||
|
||||
unp = VTOUNIONFS(fvp);
|
||||
#ifdef UNIONFS_IDBG_RENAME
|
||||
UNIONFS_INTERNAL_DEBUG("fvp=%p, ufvp=%p, lfvp=%p\n",
|
||||
fvp, unp->un_uppervp, unp->un_lowervp);
|
||||
#endif
|
||||
ump = MOUNTTOUNIONFSMOUNT(fvp->v_mount);
|
||||
/*
|
||||
* If we only have a lower vnode, copy the source file to the upper
|
||||
* FS so that the rename operation can be issued against the upper FS.
|
||||
*/
|
||||
if (unp->un_uppervp == NULLVP) {
|
||||
switch (fvp->v_type) {
|
||||
case VREG:
|
||||
if ((error = vn_lock(fvp, LK_EXCLUSIVE)) != 0)
|
||||
goto unionfs_rename_abort;
|
||||
error = unionfs_copyfile(unp, 1, fcnp->cn_cred, td);
|
||||
VOP_UNLOCK(fvp);
|
||||
if (error != 0)
|
||||
goto unionfs_rename_abort;
|
||||
break;
|
||||
case VDIR:
|
||||
if ((error = vn_lock(fvp, LK_EXCLUSIVE)) != 0)
|
||||
goto unionfs_rename_abort;
|
||||
error = unionfs_mkshadowdir(ump, rfdvp, unp, fcnp, td);
|
||||
VOP_UNLOCK(fvp);
|
||||
if (error != 0)
|
||||
goto unionfs_rename_abort;
|
||||
break;
|
||||
default:
|
||||
error = ENODEV;
|
||||
goto unionfs_rename_abort;
|
||||
bool unlock_fdvp = false, relock_tdvp = false;
|
||||
VI_UNLOCK(fvp);
|
||||
if (tvp != NULLVP)
|
||||
VOP_UNLOCK(tvp);
|
||||
if (fvp->v_type == VREG) {
|
||||
/*
|
||||
* For regular files, unionfs_copyfile() will expect
|
||||
* fdvp's upper parent directory vnode to be unlocked
|
||||
* and will temporarily lock it. If fdvp == tdvp, we
|
||||
* should unlock tdvp to avoid recursion on tdvp's
|
||||
* lock. If fdvp != tdvp, we should also unlock tdvp
|
||||
* to avoid potential deadlock due to holding tdvp's
|
||||
* lock while locking unrelated vnodes associated with
|
||||
* fdvp/fvp.
|
||||
*/
|
||||
VOP_UNLOCK(tdvp);
|
||||
relock_tdvp = true;
|
||||
} else if (fvp->v_type == VDIR && tdvp != fdvp) {
|
||||
/*
|
||||
* For directories, unionfs_mkshadowdir() will expect
|
||||
* fdvp's upper parent directory vnode to be locked
|
||||
* and will temporarily unlock it. If fdvp == tdvp,
|
||||
* we can therefore leave tdvp locked. If fdvp !=
|
||||
* tdvp, we should exchange the lock on tdvp for a
|
||||
* lock on fdvp.
|
||||
*/
|
||||
VOP_UNLOCK(tdvp);
|
||||
unlock_fdvp = true;
|
||||
relock_tdvp = true;
|
||||
vn_lock(fdvp, LK_EXCLUSIVE | LK_RETRY);
|
||||
}
|
||||
|
||||
needrelookup = 1;
|
||||
vn_lock(fvp, LK_EXCLUSIVE | LK_RETRY);
|
||||
unp = VTOUNIONFS(fvp);
|
||||
if (unp == NULL)
|
||||
error = ENOENT;
|
||||
else if (unp->un_uppervp == NULLVP) {
|
||||
switch (fvp->v_type) {
|
||||
case VREG:
|
||||
error = unionfs_copyfile(unp, 1, fcnp->cn_cred, td);
|
||||
break;
|
||||
case VDIR:
|
||||
error = unionfs_mkshadowdir(ump, rfdvp, unp, fcnp, td);
|
||||
break;
|
||||
default:
|
||||
error = ENODEV;
|
||||
break;
|
||||
}
|
||||
}
|
||||
VOP_UNLOCK(fvp);
|
||||
if (unlock_fdvp)
|
||||
VOP_UNLOCK(fdvp);
|
||||
if (relock_tdvp)
|
||||
vn_lock(tdvp, LK_EXCLUSIVE | LK_RETRY);
|
||||
if (tvp != NULLVP)
|
||||
vn_lock(tvp, LK_EXCLUSIVE | LK_RETRY);
|
||||
/*
|
||||
* Since we've dropped tdvp's lock at some point in the copy
|
||||
* sequence above, force the caller to re-drive the lookup
|
||||
* in case the relationship between tdvp and tvp has changed.
|
||||
*/
|
||||
if (error == 0)
|
||||
error = ERELOOKUP;
|
||||
goto unionfs_rename_abort;
|
||||
}
|
||||
|
||||
if (unp->un_lowervp != NULLVP)
|
||||
|
|
@ -1260,7 +1314,10 @@ unionfs_rename(struct vop_rename_args *ap)
|
|||
rfvp = unp->un_uppervp;
|
||||
vref(rfvp);
|
||||
|
||||
VI_UNLOCK(fvp);
|
||||
|
||||
unp = VTOUNIONFS(tdvp);
|
||||
|
||||
#ifdef UNIONFS_IDBG_RENAME
|
||||
UNIONFS_INTERNAL_DEBUG("tdvp=%p, utdvp=%p, ltdvp=%p\n",
|
||||
tdvp, unp->un_uppervp, unp->un_lowervp);
|
||||
|
|
@ -1273,11 +1330,12 @@ unionfs_rename(struct vop_rename_args *ap)
|
|||
ltdvp = unp->un_lowervp;
|
||||
vref(rtdvp);
|
||||
|
||||
if (tdvp == tvp) {
|
||||
rtvp = rtdvp;
|
||||
vref(rtvp);
|
||||
} else if (tvp != NULLVP) {
|
||||
if (tvp != NULLVP) {
|
||||
unp = VTOUNIONFS(tvp);
|
||||
if (unp == NULL) {
|
||||
error = ENOENT;
|
||||
goto unionfs_rename_abort;
|
||||
}
|
||||
#ifdef UNIONFS_IDBG_RENAME
|
||||
UNIONFS_INTERNAL_DEBUG("tvp=%p, utvp=%p, ltvp=%p\n",
|
||||
tvp, unp->un_uppervp, unp->un_lowervp);
|
||||
|
|
@ -1298,24 +1356,6 @@ unionfs_rename(struct vop_rename_args *ap)
|
|||
if (rfvp == rtvp)
|
||||
goto unionfs_rename_abort;
|
||||
|
||||
if (needrelookup != 0) {
|
||||
if ((error = vn_lock(fdvp, LK_EXCLUSIVE)) != 0)
|
||||
goto unionfs_rename_abort;
|
||||
error = unionfs_relookup_for_delete(fdvp, fcnp, td);
|
||||
VOP_UNLOCK(fdvp);
|
||||
if (error != 0)
|
||||
goto unionfs_rename_abort;
|
||||
|
||||
/* Lock of tvp is canceled in order to avoid recursive lock. */
|
||||
if (tvp != NULLVP && tvp != tdvp)
|
||||
VOP_UNLOCK(tvp);
|
||||
error = unionfs_relookup_for_rename(tdvp, tcnp, td);
|
||||
if (tvp != NULLVP && tvp != tdvp)
|
||||
vn_lock(tvp, LK_EXCLUSIVE | LK_RETRY);
|
||||
if (error != 0)
|
||||
goto unionfs_rename_abort;
|
||||
}
|
||||
|
||||
error = VOP_RENAME(rfdvp, rfvp, fcnp, rtdvp, rtvp, tcnp);
|
||||
|
||||
if (error == 0) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user