DEVEL15-windows-lock-corrections-20080808

LICENSE MIT

Derrick helped identify a few locations where rw or mx locks where
not properly being tracked.  As a result there were some locations
in which an assertion could be thrown due to releasing the wrong
type of lock.

Also added lock_AssertXXX calls to some locations to ensure that
the correct lock type is being held when the calls are made.  volume
location updates, cm_SyncOp, cm_SyncOpDone.

(cherry picked from commit 423cdb708f21dcbc28f6563c7c49069f6a6ec155)

diff --git a/src/WINNT/afsd/cm_daemon.c b/src/WINNT/afsd/cm_daemon.c
index a49c289..b930a94 100644 (file)
--- a/src/WINNT/afsd/cm_daemon.c
+++ b/src/WINNT/afsd/cm_daemon.c
@@ -132,22 +132,14 @@ void cm_BkgDaemon(void * parm)
 #ifdef DEBUG_REFCOUNT                
        osi_Log2(afsd_logp,"cm_BkgDaemon (after) scp 0x%x ref %d",rp->scp, rp->scp->refCount);
 #endif
-       if (code == 0) {
-           cm_ReleaseUser(rp->userp);
-           cm_ReleaseSCache(rp->scp);
-           free(rp);
-       }
-
-        lock_ObtainWrite(&cm_daemonLock);
 
         /* 
-        * Keep the following list synchronized with the
-        * error code list in cm_BkgStore
-        */
+         * Keep the following list synchronized with the
+         * error code list in cm_BkgStore.  
+         * cm_SyncOpDone(CM_SCACHESYNC_ASYNCSTORE) will be called there unless
+         * one of these errors has occurred.
+         */
        switch ( code ) {
-       case 0: /* success */
-           osi_Log1(afsd_logp,"cm_BkgDaemon SUCCESS: request 0x%p", rp);
-           break;
        case CM_ERROR_TIMEDOUT: /* or server restarting */
        case CM_ERROR_RETRY:
        case CM_ERROR_WOULDBLOCK:
@@ -157,18 +149,26 @@ void cm_BkgDaemon(void * parm)
        case CM_ERROR_PARTIALWRITE:
            osi_Log2(afsd_logp,"cm_BkgDaemon re-queueing failed request 0x%p code 0x%x",
                     rp, code);
+            lock_ObtainWrite(&cm_daemonLock);
            cm_bkgQueueCount++;
            osi_QAddT((osi_queue_t **) &cm_bkgListp, (osi_queue_t **)&cm_bkgListEndp, &rp->q);
            break;
-       default:
-           osi_Log2(afsd_logp,"cm_BkgDaemon FAILED: request dropped 0x%p code 0x%x",
+       case 0:  /* success */
+       default: /* other error */
+           if (code == 0)
+                osi_Log1(afsd_logp,"cm_BkgDaemon SUCCESS: request 0x%p", rp);
+            else
+                osi_Log2(afsd_logp,"cm_BkgDaemon FAILED: request dropped 0x%p code 0x%x",
                     rp, code);
+           cm_ReleaseUser(rp->userp);
+           cm_ReleaseSCache(rp->scp);
+           free(rp);
+            lock_ObtainWrite(&cm_daemonLock);
        }
     }
     lock_ReleaseWrite(&cm_daemonLock);
 
     thrd_SetEvent(cm_BkgDaemon_ShutdownEvent[daemonID]);
-
 }
 
 void cm_QueueBKGRequest(cm_scache_t *scp, cm_bkgProc_t *procp, afs_uint32 p1, afs_uint32 p2, afs_uint32 p3, afs_uint32 p4,

diff --git a/src/WINNT/afsd/cm_scache.c b/src/WINNT/afsd/cm_scache.c
index 8b582c7..86598e1 100644 (file)
--- a/src/WINNT/afsd/cm_scache.c
+++ b/src/WINNT/afsd/cm_scache.c
@@ -1047,6 +1047,8 @@ long cm_SyncOp(cm_scache_t *scp, cm_buf_t *bufp, cm_user_t *userp, cm_req_t *req
     int wakeupCycle;
     int getAccessRights = 1;
 
+    lock_AssertWrite(&scp->rw);
+
     /* lookup this first */
     bufLocked = flags & CM_SCACHESYNC_BUFLOCKED;
 

diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c
index 9cf6eb0..c9d3eb2 100644 (file)
--- a/src/WINNT/afsd/cm_volume.c
+++ b/src/WINNT/afsd/cm_volume.c
@@ -185,6 +185,8 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
 #endif
     afs_uint32 volType;
 
+    lock_AssertWrite(&volp->rw);
+
 #ifdef AFS_FREELANCE_CLIENT
     if ( cellp->cellID == AFS_FAKE_ROOT_CELL_ID && volp->vol[RWVOL].ID == AFS_FAKE_ROOT_VOL_ID ) 
     {

diff --git a/src/WINNT/afsd/rawops.c b/src/WINNT/afsd/rawops.c
index 0848621..1debe4c 100644 (file)
--- a/src/WINNT/afsd/rawops.c
+++ b/src/WINNT/afsd/rawops.c
@@ -342,10 +342,11 @@ long WriteData(cm_scache_t *scp, osi_hyper_t offset, long count, char *op,
 
     if (code == 0 && doWriteBack) {
         lock_ObtainWrite(&scp->rw);
-        cm_SyncOp(scp, NULL, userp, &req, 0, CM_SCACHESYNC_ASYNCSTORE);
+        code = cm_SyncOp(scp, NULL, userp, &req, 0, CM_SCACHESYNC_ASYNCSTORE);
         lock_ReleaseWrite(&scp->rw);
-        cm_QueueBKGRequest(scp, cm_BkgStore, writeBackOffset.LowPart,
-                            writeBackOffset.HighPart, cm_chunkSize, 0, userp);
+        if (code == 0)
+            cm_QueueBKGRequest(scp, cm_BkgStore, writeBackOffset.LowPart,
+                               writeBackOffset.HighPart, cm_chunkSize, 0, userp);
     }   
 
     /* cm_SyncOpDone is called when cm_BkgStore completes */

diff --git a/src/WINNT/afsd/smb3.c b/src/WINNT/afsd/smb3.c
index 26b1650..37bd818 100644 (file)
--- a/src/WINNT/afsd/smb3.c
+++ b/src/WINNT/afsd/smb3.c
@@ -2848,7 +2848,7 @@ long smb_ReceiveTran2QPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
     cm_user_t *userp;
     cm_space_t *spacep;
     cm_scache_t *scp, *dscp;
-    int scp_mx_held = 0;
+    int scp_rw_held = 0;
     int delonclose = 0;
     long code = 0;
     clientchar_t *pathp;
@@ -3012,14 +3012,16 @@ long smb_ReceiveTran2QPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
 #endif /* DFS_SUPPORT */
 
     lock_ObtainWrite(&scp->rw);
-    scp_mx_held = 1;
+    scp_rw_held = 2;
     code = cm_SyncOp(scp, NULL, userp, &req, 0,
                       CM_SCACHESYNC_NEEDCALLBACK | CM_SCACHESYNC_GETSTATUS);
-    if (code) goto done;
+    if (code)
+        goto done;
 
     cm_SyncOpDone(scp, NULL, CM_SCACHESYNC_NEEDCALLBACK | CM_SCACHESYNC_GETSTATUS);
         
     lock_ConvertWToR(&scp->rw);
+    scp_rw_held = 1;
 
     len = 0;
 
@@ -3081,7 +3083,7 @@ long smb_ReceiveTran2QPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
 
        if (fidp) {
            lock_ReleaseRead(&scp->rw);
-           scp_mx_held = 0;
+           scp_rw_held = 0;
            lock_ObtainMutex(&fidp->mx);
            delonclose = fidp->flags & SMB_FID_DELONCLOSE;
            lock_ReleaseMutex(&fidp->mx);
@@ -3125,8 +3127,15 @@ long smb_ReceiveTran2QPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
 
     /* send and free the packets */
   done:
-    if (scp_mx_held)
+    switch (scp_rw_held) {
+    case 1:
        lock_ReleaseRead(&scp->rw);
+        break;
+    case 2:
+        lock_ReleaseWrite(&scp->rw);
+        break;
+    }
+    scp_rw_held = 0;
     cm_ReleaseSCache(scp);
     cm_ReleaseUser(userp);
     if (code == 0) {
@@ -4011,6 +4020,8 @@ smb_ApplyV3DirListPatches(cm_scache_t *dscp, smb_dirListPatch_t **dirPatchespp,
         lock_ObtainWrite(&dscp->rw);
         code = cm_SyncOp(dscp, NULL, userp, reqp, PRSFS_READ,
                           CM_SCACHESYNC_NEEDCALLBACK | CM_SCACHESYNC_GETSTATUS);
+        if (code == 0) 
+            cm_SyncOpDone(dscp, NULL, CM_SCACHESYNC_NEEDCALLBACK | CM_SCACHESYNC_GETSTATUS);
         lock_ReleaseWrite(&dscp->rw);
         if (code == CM_ERROR_NOACCESS) {
             mustFake = 1;
@@ -6237,6 +6248,7 @@ long smb_ReceiveV3GetAttributes(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *
     cm_SyncOpDone(scp, NULL, CM_SCACHESYNC_NEEDCALLBACK | CM_SCACHESYNC_GETSTATUS);
 
     lock_ConvertWToR(&scp->rw);
+    readlock = 1;
 
     /* decode times.  We need a search time, but the response to this
      * call provides the date first, not the time, as returned in the