diff options
author | jeff <jeff@FreeBSD.org> | 2005-04-09 11:53:16 +0000 |
---|---|---|
committer | jeff <jeff@FreeBSD.org> | 2005-04-09 11:53:16 +0000 |
commit | 46c812596a58be0ea67662a6d56bc9af43ea08b6 (patch) | |
tree | eecf3f40c62a4d34291511e4ba17c079496c451d | |
parent | 1a889a3a93f7c1d0666bf18dacc7bccf9f2b068e (diff) | |
download | FreeBSD-src-46c812596a58be0ea67662a6d56bc9af43ea08b6.zip FreeBSD-src-46c812596a58be0ea67662a6d56bc9af43ea08b6.tar.gz |
- If we vrele() a dvp while the child is locked we can potentially deadlock
when vrele() acquires the directory lock in the wrong order. Fix this
via the following changes:
- Keep the directory locked after VOP_LOOKUP() until we've determined
what we're going to do with the child. This allows us to remove the
complicated post LOOKUP code which determins whether we should lock or
unlock the parent. This means we may have to vput() in the appropriate
cases later, rather than doing an unsafe vrele.
- in NDFREE() keep two flags to indicate whether we need to unlock vp or
dvp. This allows us to vput rather than vrele in the appropriate
cases without rechecking the flags. Move the code to handle dvp after
we handle vp.
- Remove some dead code from namei() that was the result of changes to
VFS_LOCK_GIANT().
Sponsored by: Isilon Systems, Inc.
-rw-r--r-- | sys/kern/vfs_lookup.c | 111 |
1 files changed, 60 insertions, 51 deletions
diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c index f17bb5e..ff8ba2d 100644 --- a/sys/kern/vfs_lookup.c +++ b/sys/kern/vfs_lookup.c @@ -118,7 +118,6 @@ namei(ndp) struct componentname *cnp = &ndp->ni_cnd; struct thread *td = cnp->cn_thread; struct proc *p = td->td_proc; - struct mount *mp; int vfslocked; ndp->ni_cnd.cn_cred = ndp->ni_cnd.cn_thread->td_ucred; @@ -229,8 +228,6 @@ namei(ndp) ndp->ni_cnd.cn_flags |= GIANTHELD; return (0); } - if ((cnp->cn_flags & LOCKPARENT) && ndp->ni_pathlen == 1) - VOP_UNLOCK(ndp->ni_dvp, 0, td); if (ndp->ni_loopcnt++ >= MAXSYMLINKS) { error = ELOOP; break; @@ -290,11 +287,10 @@ namei(ndp) cnp->cn_pnbuf = NULL; cnp->cn_nameptr = NULL; #endif - vrele(ndp->ni_dvp); - mp = ndp->ni_vp->v_mount; vput(ndp->ni_vp); - VFS_UNLOCK_GIANT(vfslocked); ndp->ni_vp = NULL; + vrele(ndp->ni_dvp); + VFS_UNLOCK_GIANT(vfslocked); return (error); } @@ -498,12 +494,12 @@ dirloop: goto bad; } tdp = dp; - tvfslocked = vfslocked; dp = dp->v_mount->mnt_vnodecovered; - vput(tdp); + tvfslocked = vfslocked; vfslocked = VFS_LOCK_GIANT(dp->v_mount); - VFS_UNLOCK_GIANT(tvfslocked); VREF(dp); + vput(tdp); + VFS_UNLOCK_GIANT(tvfslocked); vn_lock(dp, cnp->cn_lkflags | LK_RETRY, td); } } @@ -548,12 +544,12 @@ unionlookup: (dp->v_vflag & VV_ROOT) && (dp->v_mount != NULL) && (dp->v_mount->mnt_flag & MNT_UNION)) { tdp = dp; - tvfslocked = vfslocked; dp = dp->v_mount->mnt_vnodecovered; - vput(tdp); + tvfslocked = vfslocked; vfslocked = VFS_LOCK_GIANT(dp->v_mount); - VFS_UNLOCK_GIANT(tvfslocked); VREF(dp); + vput(tdp); + VFS_UNLOCK_GIANT(tvfslocked); vn_lock(dp, cnp->cn_lkflags | LK_RETRY, td); goto unionlookup; } @@ -596,22 +592,11 @@ unionlookup: printf("found\n"); #endif /* - * In the DOTDOT case dp is unlocked, we may have to relock it if - * this is the parent of the last component. Otherwise, we have to - * unlock if this is not the last component, or if it is and - * LOCKPARENT is set. + * In the DOTDOT case dp is unlocked, we relock it here even if we + * may not need it to simplify the code below. */ - if (cnp->cn_flags & ISDOTDOT) { - if ((cnp->cn_flags & (ISLASTCN | LOCKPARENT)) == - (ISLASTCN | LOCKPARENT)) - vn_lock(dp, LK_EXCLUSIVE | LK_RETRY, td); - } else if (dp != ndp->ni_vp) { - if ((cnp->cn_flags & (ISLASTCN | LOCKPARENT)) == ISLASTCN) - VOP_UNLOCK(dp, 0, td); - else if ((cnp->cn_flags & ISLASTCN) == 0) - VOP_UNLOCK(dp, 0, td); - } - + if (cnp->cn_flags & ISDOTDOT) + vn_lock(dp, LK_EXCLUSIVE | LK_RETRY, td); /* * Take into account any additional components consumed by * the underlying filesystem. @@ -631,21 +616,20 @@ unionlookup: */ while (dp->v_type == VDIR && (mp = dp->v_mountedhere) && (cnp->cn_flags & NOCROSSMOUNT) == 0) { + KASSERT(dp != ndp->ni_dvp, ("XXX")); if (vfs_busy(mp, 0, 0, td)) continue; - VOP_UNLOCK(dp, 0, td); + vput(dp); tvfslocked = VFS_LOCK_GIANT(mp); + VFS_UNLOCK_GIANT(vfslocked); + vfslocked = tvfslocked; error = VFS_ROOT(mp, cnp->cn_lkflags, &tdp, td); vfs_unbusy(mp, td); if (error) { - VFS_UNLOCK_GIANT(tvfslocked); dpunlocked = 1; goto bad2; } - vrele(dp); - VFS_UNLOCK_GIANT(vfslocked); ndp->ni_vp = dp = tdp; - vfslocked = tvfslocked; } /* @@ -665,6 +649,11 @@ unionlookup: error = EACCES; goto bad2; } + /* + * Symlink code always expects an unlocked dvp. + */ + if (ndp->ni_dvp != ndp->ni_vp) + VOP_UNLOCK(ndp->ni_dvp, 0, td); goto success; } @@ -689,9 +678,10 @@ nextname: cnp->cn_nameptr++; ndp->ni_pathlen--; } - if (ndp->ni_dvp != ndp->ni_vp) - ASSERT_VOP_UNLOCKED(ndp->ni_dvp, "lookup"); - vrele(ndp->ni_dvp); + if (ndp->ni_dvp != dp) + vput(ndp->ni_dvp); + else + vrele(ndp->ni_dvp); goto dirloop; } /* @@ -706,8 +696,13 @@ nextname: ndp->ni_startdir = ndp->ni_dvp; VREF(ndp->ni_startdir); } - if (!wantparent) - vrele(ndp->ni_dvp); + if (!wantparent) { + if (ndp->ni_dvp != dp) + vput(ndp->ni_dvp); + else + vrele(ndp->ni_dvp); + } else if ((cnp->cn_flags & LOCKPARENT) == 0 && ndp->ni_dvp != dp) + VOP_UNLOCK(ndp->ni_dvp, 0, td); if ((cnp->cn_flags & LOCKLEAF) == 0) VOP_UNLOCK(dp, 0, td); @@ -717,14 +712,12 @@ success: return (0); bad2: - if ((cnp->cn_flags & LOCKPARENT) && *ndp->ni_next == '\0') + if (dp != ndp->ni_dvp) vput(ndp->ni_dvp); else vrele(ndp->ni_dvp); bad: - if (dpunlocked) - vrele(dp); - else + if (!dpunlocked) vput(dp); VFS_UNLOCK_GIANT(vfslocked); ndp->ni_cnd.cn_flags &= ~GIANTHELD; @@ -887,29 +880,45 @@ NDFREE(ndp, flags) struct nameidata *ndp; const u_int flags; { + int unlock_dvp; + int unlock_vp; + + unlock_dvp = 0; + unlock_vp = 0; if (!(flags & NDF_NO_FREE_PNBUF) && (ndp->ni_cnd.cn_flags & HASBUF)) { uma_zfree(namei_zone, ndp->ni_cnd.cn_pnbuf); ndp->ni_cnd.cn_flags &= ~HASBUF; } + if (!(flags & NDF_NO_VP_UNLOCK) && + (ndp->ni_cnd.cn_flags & LOCKLEAF) && ndp->ni_vp) + unlock_vp = 1; + if (!(flags & NDF_NO_VP_RELE) && ndp->ni_vp) { + if (unlock_vp) { + vput(ndp->ni_vp); + unlock_vp = 0; + } else + vrele(ndp->ni_vp); + ndp->ni_vp = NULL; + } + if (unlock_vp) + VOP_UNLOCK(ndp->ni_vp, 0, ndp->ni_cnd.cn_thread); if (!(flags & NDF_NO_DVP_UNLOCK) && (ndp->ni_cnd.cn_flags & LOCKPARENT) && ndp->ni_dvp != ndp->ni_vp) - VOP_UNLOCK(ndp->ni_dvp, 0, ndp->ni_cnd.cn_thread); + unlock_dvp = 1; if (!(flags & NDF_NO_DVP_RELE) && (ndp->ni_cnd.cn_flags & (LOCKPARENT|WANTPARENT))) { - vrele(ndp->ni_dvp); + if (unlock_dvp) { + vput(ndp->ni_dvp); + unlock_dvp = 0; + } else + vrele(ndp->ni_dvp); ndp->ni_dvp = NULL; } - if (!(flags & NDF_NO_VP_UNLOCK) && - (ndp->ni_cnd.cn_flags & LOCKLEAF) && ndp->ni_vp) - VOP_UNLOCK(ndp->ni_vp, 0, ndp->ni_cnd.cn_thread); - if (!(flags & NDF_NO_VP_RELE) && - ndp->ni_vp) { - vrele(ndp->ni_vp); - ndp->ni_vp = NULL; - } + if (unlock_dvp) + VOP_UNLOCK(ndp->ni_dvp, 0, ndp->ni_cnd.cn_thread); if (!(flags & NDF_NO_STARTDIR_RELE) && (ndp->ni_cnd.cn_flags & SAVESTART)) { vrele(ndp->ni_startdir); |