DEVEL15-windows-smb_fid_t-deadlock-20061022

smb_ReleaseFID cannot be called while a cm_scache_t->mx is held

shuffle the order of the smb_ReleaseFID calls so they are always after
cm_XXXRelease calls for performance.

(cherry picked from commit c484781531ce29d3d1b5c3753322be4a87dd0841)

diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c
index 0d53ddd7ce6a1c4937025f2b75914860a02729f3..12eba606fc97cfba42a3c8992199db12296ed8d8 100644 (file)
--- a/src/WINNT/afsd/smb.c
+++ b/src/WINNT/afsd/smb.c
@@ -1547,6 +1547,9 @@ void smb_HoldFIDNoLock(smb_fid_t *fidp)
     fidp->refCount++;
 }
 
+
+/* smb_ReleaseFID cannot be called while an cm_scache_t mutex lock is held */
+/* the sm_fid_t->mx and smb_rctLock must not be held */
 void smb_ReleaseFID(smb_fid_t *fidp)
 {
     cm_scache_t *scp = NULL;
@@ -1561,13 +1564,13 @@ void smb_ReleaseFID(smb_fid_t *fidp)
         vcp = fidp->vcp;
         fidp->vcp = NULL;
         scp = fidp->scp;    /* release after lock is released */
-               if (scp) {
-               lock_ObtainMutex(&scp->mx);
-               scp->flags &= ~CM_SCACHEFLAG_SMB_FID;
-               lock_ReleaseMutex(&scp->mx);
-               osi_Log2(afsd_logp,"smb_ReleaseFID fidp 0x%p scp 0x%p", fidp, scp);
-        fidp->scp = NULL;
-               }
+       if (scp) {
+           lock_ObtainMutex(&scp->mx);
+           scp->flags &= ~CM_SCACHEFLAG_SMB_FID;
+           lock_ReleaseMutex(&scp->mx);
+           osi_Log2(afsd_logp,"smb_ReleaseFID fidp 0x%p scp 0x%p", fidp, scp);
+           fidp->scp = NULL;
+       }
         userp = fidp->userp;
         fidp->userp = NULL;
 

diff --git a/src/WINNT/afsd/smb3.c b/src/WINNT/afsd/smb3.c
index 5d7a612c9839bbc1204cfbd9c8f43c3a8e12d133..b50ed8f1f615fa8d92d205f08c3c5ee236c9c2d1 100644 (file)
--- a/src/WINNT/afsd/smb3.c
+++ b/src/WINNT/afsd/smb3.c
@@ -2654,6 +2654,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 delonclose = 0;
     long code = 0;
     char *pathp;
@@ -2814,6 +2815,7 @@ long smb_ReceiveTran2QPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
 #endif /* DFS_SUPPORT */
 
     lock_ObtainMutex(&scp->mx);
+    scp_mx_held = 1;
     code = cm_SyncOp(scp, NULL, userp, &req, 0,
                       CM_SCACHESYNC_NEEDCALLBACK | CM_SCACHESYNC_GETSTATUS);
     if (code) goto done;
@@ -2879,8 +2881,8 @@ long smb_ReceiveTran2QPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
 
        if (fidp) {
            lock_ReleaseMutex(&scp->mx);
+           scp_mx_held = 0;
            lock_ObtainMutex(&fidp->mx);
-           lock_ObtainMutex(&scp->mx);
            delonclose = fidp->flags & SMB_FID_DELONCLOSE;
            lock_ReleaseMutex(&fidp->mx);
            smb_ReleaseFID(fidp);
@@ -2923,7 +2925,8 @@ long smb_ReceiveTran2QPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
 
     /* send and free the packets */
   done:
-    lock_ReleaseMutex(&scp->mx);
+    if (scp_mx_held)
+       lock_ReleaseMutex(&scp->mx);
     cm_ReleaseSCache(scp);
     cm_ReleaseUser(userp);
     if (code == 0) {
@@ -3064,8 +3067,8 @@ long smb_ReceiveTran2SetPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet
 
     fidp = smb_FindFIDByScache(vcp, scp);
     if (!fidp) {
-        cm_ReleaseUser(userp);
         cm_ReleaseSCache(scp);
+        cm_ReleaseUser(userp);
        smb_SendTran2Error(vcp, p, opx, code);
         return 0;
     }
@@ -3073,9 +3076,9 @@ long smb_ReceiveTran2SetPathInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet
     lock_ObtainMutex(&fidp->mx);
     if (!(fidp->flags & SMB_FID_OPENWRITE)) {
        lock_ReleaseMutex(&fidp->mx);
+        cm_ReleaseSCache(scp);
         smb_ReleaseFID(fidp);
         cm_ReleaseUser(userp);
-        cm_ReleaseSCache(scp);
         smb_SendTran2Error(vcp, p, opx, CM_ERROR_NOACCESS);
         return 0;
     }
@@ -5737,8 +5740,8 @@ long smb_ReceiveV3WriteX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp)
     smb_SetSMBDataLength(outp, 0);
 
  done:
-    smb_ReleaseFID(fidp);
     cm_ReleaseUser(userp);
+    smb_ReleaseFID(fidp);
 
     return code;
 }
@@ -5860,9 +5863,8 @@ long smb_ReceiveV3ReadX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp)
     smb_SetSMBParm(outp, 5, finalCount);
     smb_SetSMBDataLength(outp, finalCount);
 
-    smb_ReleaseFID(fidp);
-
     cm_ReleaseUser(userp);
+    smb_ReleaseFID(fidp);
     return code;
 }   
         
@@ -6213,9 +6215,9 @@ long smb_ReceiveNTCreateX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp)
                     treeStartp = realPathp + (tp - spacep->data);
 
                     if (*tp && !smb_IsLegalFilename(tp)) {
+                        cm_ReleaseUser(userp);
                         if (baseFidp) 
                             smb_ReleaseFID(baseFidp);
-                        cm_ReleaseUser(userp);
                         free(realPathp);
                         if (scp)
                             cm_ReleaseSCache(scp);
@@ -6580,16 +6582,14 @@ long smb_ReceiveNTCreateX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp)
         lock_ReleaseMutex(&scp->mx);
 
         if (code) {
-            /* shouldn't this be smb_CloseFID() fidp->flags = SMB_FID_DELETE; */
-           smb_CloseFID(vcp, fidp, NULL, 0);
-            smb_ReleaseFID(fidp);
-
             cm_ReleaseSCache(scp);
             if (dscp)
                 cm_ReleaseSCache(dscp);
             cm_ReleaseUser(userp);
+            /* shouldn't this be smb_CloseFID() fidp->flags = SMB_FID_DELETE; */
+           smb_CloseFID(vcp, fidp, NULL, 0);
+            smb_ReleaseFID(fidp);
             free(realPathp);
-            
             return code;
         }
     }
@@ -6657,9 +6657,8 @@ long smb_ReceiveNTCreateX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp)
     osi_Log2(smb_logp, "SMB NT CreateX opening fid %d path %s", fidp->fid,
               osi_LogSaveString(smb_logp, realPathp));
 
-    smb_ReleaseFID(fidp);
-
     cm_ReleaseUser(userp);
+    smb_ReleaseFID(fidp);
 
     /* Can't free realPathp if we get here since
        fidp->NTopen_wholepathp is pointing there */
@@ -7200,14 +7199,12 @@ long smb_ReceiveNTTranCreate(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *out
         lock_ReleaseMutex(&scp->mx);
 
         if (code) {
+            cm_ReleaseSCache(scp);
+            cm_ReleaseUser(userp);
             /* Shouldn't this be smb_CloseFID()?  fidp->flags = SMB_FID_DELETE; */
            smb_CloseFID(vcp, fidp, NULL, 0);
             smb_ReleaseFID(fidp);
-
-            cm_ReleaseSCache(scp);
-            cm_ReleaseUser(userp);
             free(realPathp);
-            
             return CM_ERROR_SHARING_VIOLATION;
         }
     }
@@ -7354,9 +7351,8 @@ long smb_ReceiveNTTranCreate(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *out
 
     osi_Log1(smb_logp, "SMB NTTranCreate opening fid %d", fidp->fid);
 
-    smb_ReleaseFID(fidp);
-
     cm_ReleaseUser(userp);
+    smb_ReleaseFID(fidp);
 
     /* free(realPathp); Can't free realPathp here because fidp->NTopen_wholepathp points there */
     /* leave scp held since we put it in fidp->scp */