From: Simon Wilkinson <sxw@inf.ed.ac.uk>
Date: Mon, 26 Oct 2009 17:06:20 +0000 (+0000)
Subject: Fix locking in afs_buffer.c
X-Git-Tag: openafs-devel-1_5_67~104
X-Git-Url: https://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=b21b209f1cb2bafe916543c1ac8f232d6fc84847

Fix locking in afs_buffer.c

Back in 2002, 0eb68f307aac84472a13523a0ce8b7a865f01ac7 was committed
to fix locking problems in dir/buffer.c.

Sadly, similar changes were never made to afs/afs_buffer.c, so the
same problems remain in the cache manager.

The issue here is with two processes racing in afs_newslot. Calls to
afs_newslot protect buffers with a zero reference count using
afs_bufferLock. If we release afs_bufferLock, before we increase the
reference count of the vcache, then we can end up with newslot
picking the same buffer for two different purposes.

The GLOCK actually protects us from the worst of this, but this fix
is necessary both for correctness, and for symmetry with the file
server buffer code.

Change-Id: I7f1c7d6571559c5f1784926c39d4889b77677231
Reviewed-on: http://gerrit.openafs.org/737
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: Derrick Brashear <shadow@dementia.org>
---

diff --git a/src/afs/afs_buffer.c b/src/afs/afs_buffer.c
index bdae5f0..220b496 100644
--- a/src/afs/afs_buffer.c
+++ b/src/afs/afs_buffer.c
@@ -76,9 +76,23 @@ extern struct buf *geteblk();
 #ifdef AFS_FBSD_ENV
 #define timecounter afs_timecounter
 #endif
-/* The locks for individual buffer entries are now sometimes obtained while holding the
- * afs_bufferLock. Thus we now have a locking hierarchy: afs_bufferLock -> Buffers[].lock.
+
+/* A note on locking in 'struct buffer'
+ *
+ * afs_bufferLock protects the hash chain, and the 'lockers' field where that
+ * has a zero value. It must be held whenever lockers is incremented from zero.
+ *
+ * The individual buffer lock protects the contents of the structure, including
+ * the lockers field.
+ *
+ * For safety: afs_bufferLock and the individual buffer lock must be held
+ * when obtaining a reference on a structure. Only the individual buffer lock
+ * need be held when releasing a reference.
+ *
+ * The locking hierarchy is afs_bufferLock-> buffer.lock
+ *
  */
+
 static afs_lock_t afs_bufferLock;
 static struct buffer *phTable[PHSIZE];	/* page hash table */
 static int nbuffers;
@@ -170,8 +184,8 @@ DRead(register struct dcache *adc, register int page)
     if ((tb = phTable[pHash(adc->index, page)])) {
 	if (bufmatch(tb)) {
 	    MObtainWriteLock(&tb->lock, 257);
-	    ReleaseWriteLock(&afs_bufferLock);
 	    tb->lockers++;
+	    ReleaseWriteLock(&afs_bufferLock);
 	    tb->accesstime = timecounter++;
 	    AFS_STATS(afs_stats_cmperf.bufHits++);
 	    MReleaseWriteLock(&tb->lock);
@@ -183,8 +197,8 @@ DRead(register struct dcache *adc, register int page)
 		if (bufmatch(tb2)) {
 		    buf_Front(bufhead, tb, tb2);
 		    MObtainWriteLock(&tb2->lock, 258);
-		    ReleaseWriteLock(&afs_bufferLock);
 		    tb2->lockers++;
+		    ReleaseWriteLock(&afs_bufferLock);
 		    tb2->accesstime = timecounter++;
 		    AFS_STATS(afs_stats_cmperf.bufHits++);
 		    MReleaseWriteLock(&tb2->lock);
@@ -194,8 +208,8 @@ DRead(register struct dcache *adc, register int page)
 		    if (bufmatch(tb)) {
 			buf_Front(bufhead, tb2, tb);
 			MObtainWriteLock(&tb->lock, 259);
-			ReleaseWriteLock(&afs_bufferLock);
 			tb->lockers++;
+			ReleaseWriteLock(&afs_bufferLock);
 			tb->accesstime = timecounter++;
 			AFS_STATS(afs_stats_cmperf.bufHits++);
 			MReleaseWriteLock(&tb->lock);
@@ -220,8 +234,8 @@ DRead(register struct dcache *adc, register int page)
 	return NULL;
     }
     MObtainWriteLock(&tb->lock, 260);
-    MReleaseWriteLock(&afs_bufferLock);
     tb->lockers++;
+    MReleaseWriteLock(&afs_bufferLock);
     if (page * AFS_BUFFER_PAGESIZE >= adc->f.chunkBytes) {
 	tb->fid = NULLIDX;
 	afs_reset_inode(&tb->inode);
@@ -555,8 +569,8 @@ DNew(register struct dcache *adc, register int page)
 	afs_WriteDCache(adc, 1);
     }
     MObtainWriteLock(&tb->lock, 265);
-    MReleaseWriteLock(&afs_bufferLock);
     tb->lockers++;
+    MReleaseWriteLock(&afs_bufferLock);
     MReleaseWriteLock(&tb->lock);
     return tb->data;
 }