From 76de77c7f9a6f58e7d7e85554decaa2ac959db07 Mon Sep 17 00:00:00 2001 From: Ben Kaduk Date: Sat, 6 Nov 2010 00:02:31 -0400 Subject: [PATCH] FBSD: close race in afs_root Previously, we called afs_PutVCache(afs_globalVp) directly. This is unsafe because PutVCache acquires locks which can sleep, losing the serialization of the GLOCK. In rare circumstances, this can result in two threads simultaneously making that call, and the second one would panic in vputx() with a negative refcount. Close the race by using a local variable for the afs_PutVCache() calls, applying the change to afs_globalVp before dropping the GLOCK. While here, fix up other race conditions. Change-Id: Id62f27d4469a96c9becd9b031125bd6c910c3e9b Reviewed-on: http://gerrit.openafs.org/3275 Tested-by: BuildBot Reviewed-by: Derrick Brashear --- src/afs/FBSD/osi_vfsops.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/afs/FBSD/osi_vfsops.c b/src/afs/FBSD/osi_vfsops.c index d3e6c57..c0673f0 100644 --- a/src/afs/FBSD/osi_vfsops.c +++ b/src/afs/FBSD/osi_vfsops.c @@ -228,6 +228,7 @@ afs_root(struct mount *mp, struct vnode **vpp) int error; struct vrequest treq; struct vcache *tvp = 0; + struct vcache *gvp; #if !defined(AFS_FBSD53_ENV) || defined(AFS_FBSD80_ENV) struct thread *td = curthread; #endif @@ -236,22 +237,26 @@ afs_root(struct mount *mp, struct vnode **vpp) AFS_GLOCK(); AFS_STATCNT(afs_root); crhold(cr); +tryagain: if (afs_globalVp && (afs_globalVp->f.states & CStatd)) { tvp = afs_globalVp; error = 0; } else { -tryagain: - if (afs_globalVp) { - afs_PutVCache(afs_globalVp); - /* vrele() needed here or not? */ - afs_globalVp = NULL; - } if (!(error = afs_InitReq(&treq, cr)) && !(error = afs_CheckInit())) { tvp = afs_GetVCache(&afs_rootFid, &treq, NULL, NULL); /* we really want this to stay around */ - if (tvp) + if (tvp) { + gvp = afs_globalVp; afs_globalVp = tvp; - else + if (gvp) { + afs_PutVCache(gvp); + if (tvp != afs_globalVp) { + /* someone raced us and won */ + afs_PutVCache(tvp); + goto tryagain; + } + } + } else error = ENOENT; } } @@ -260,16 +265,23 @@ tryagain: ASSERT_VI_UNLOCKED(vp, "afs_root"); AFS_GUNLOCK(); + error = vget(vp, LK_EXCLUSIVE | LK_RETRY, td); + AFS_GLOCK(); + /* we dropped the glock, so re-check everything it had serialized */ + if (!afs_globalVp || !(afs_globalVp->f.states & CStatd) || + tvp != afs_globalVp) { + vput(vp); + afs_PutVCache(tvp); + goto tryagain; + } + if (error != 0) + goto tryagain; /* * I'm uncomfortable about this. Shouldn't this happen at a * higher level, and shouldn't we busy the top-level directory * to prevent recycling? */ - error = vget(vp, LK_EXCLUSIVE | LK_RETRY, td); vp->v_vflag |= VV_ROOT; - AFS_GLOCK(); - if (error != 0) - goto tryagain; afs_globalVFS = mp; *vpp = vp; -- 1.9.4