Skip to content

Commit

Permalink
unionfs_rename: fix numerous locking issues
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Jason A. Harmening authored and Jason A. Harmening committed Apr 29, 2024
1 parent 993d1fa commit 05e8ab6
Showing 1 changed file with 96 additions and 56 deletions.
152 changes: 96 additions & 56 deletions sys/fs/unionfs/union_vnops.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand All @@ -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 ||
Expand All @@ -1201,66 +1199,125 @@ 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;
goto unionfs_rename_abort;
} else {
rfdvp = unp->un_uppervp;
vref(rfdvp);
}
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;
}

#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)
fcnp->cn_flags |= DOWHITEOUT;
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);
Expand All @@ -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);
Expand All @@ -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) {
Expand Down

0 comments on commit 05e8ab6

Please sign in to comment.