From: Ben Kaduk <kaduk@mit.edu>
Date: Sat, 6 Nov 2010 04:02:31 +0000 (-0400)
Subject: FBSD: close race in afs_root
X-Git-Tag: upstream/1.6.0.pre2^2~163
X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=72e2b1119136315a5845c7af00aa73456985d6fa;p=packages%2Fo%2Fopenafs.git

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: I4733489d50d3459172ee2eb021190d013f68018a
Reviewed-on: http://gerrit.openafs.org/3275
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Reviewed-on: http://gerrit.openafs.org/3631
Tested-by: Derrick Brashear <shadow@dementia.org>
---

diff --git a/src/afs/FBSD/osi_vfsops.c b/src/afs/FBSD/osi_vfsops.c
index 32a8feb43..b956b8090 100644
--- a/src/afs/FBSD/osi_vfsops.c
+++ b/src/afs/FBSD/osi_vfsops.c
@@ -242,6 +242,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
@@ -250,22 +251,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;
 	}
     }
@@ -274,16 +279,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;