windows-deadlock-and-race-removal-20060427

This patch fixes:

* race conditions around cm_Lock() calls that were not protected
  by cm_SyncOp(LOCK) [asanka@secure-endpoints.com]

* deadlocks caused by obtaining smb_fid_t->mx after cm_scache_t->mx

* removes an extra Release smb_fid_t->mx that could result in
  releasing a mutex that is not currently held

* changes the log representation of several return codes and fids to
  be consistent with other output

diff --git a/src/WINNT/afsd/cm_buf.c b/src/WINNT/afsd/cm_buf.c
index eaa8dc1..cdfe139 100644 (file)
--- a/src/WINNT/afsd/cm_buf.c
+++ b/src/WINNT/afsd/cm_buf.c
@@ -885,7 +885,7 @@ long buf_GetNew(struct cm_scache *scp, osi_hyper_t *offsetp, cm_buf_t **bufpp)
      */
     lock_ReleaseMutex(&bp->mx);
     *bufpp = bp;
-    osi_Log3(buf_logp, "buf_GetNew returning bp 0x%p for file 0x%p, offset 0x%x",
+    osi_Log3(buf_logp, "buf_GetNew returning bp 0x%p for scp 0x%p, offset 0x%x",
               bp, scp, offsetp->LowPart);
     return 0;
 }
@@ -1032,7 +1032,7 @@ long buf_Get(struct cm_scache *scp, osi_hyper_t *offsetp, cm_buf_t **bufpp)
     }
     lock_ReleaseWrite(&buf_globalLock);
 
-    osi_Log3(buf_logp, "buf_Get returning bp 0x%p for file 0x%p, offset 0x%x",
+    osi_Log3(buf_logp, "buf_Get returning bp 0x%p for scp 0x%p, offset 0x%x",
               bp, scp, offsetp->LowPart);
 #ifdef TESTING
     buf_ValidateBufQueues();

diff --git a/src/WINNT/afsd/cm_vnodeops.c b/src/WINNT/afsd/cm_vnodeops.c
index 51f8072..65b3b22 100644 (file)
--- a/src/WINNT/afsd/cm_vnodeops.c
+++ b/src/WINNT/afsd/cm_vnodeops.c
@@ -265,7 +265,8 @@ long cm_CheckOpen(cm_scache_t *scp, int openMode, int trunc, cm_user_t *userp,
 
     code = cm_SyncOp(scp, NULL, userp, reqp, rights,
                       CM_SCACHESYNC_GETSTATUS
-                      | CM_SCACHESYNC_NEEDCALLBACK);
+                     | CM_SCACHESYNC_NEEDCALLBACK
+                     | CM_SCACHESYNC_LOCK);
 
     if (code == 0 && 
         ((rights & PRSFS_WRITE) || (rights & PRSFS_READ)) &&
@@ -315,8 +316,15 @@ long cm_CheckOpen(cm_scache_t *scp, int openMode, int trunc, cm_user_t *userp,
                }
            }
         }
+
+    } else if (code != 0) {
+        goto _done;
     }
 
+    cm_SyncOpDone(scp, NULL, CM_SCACHESYNC_LOCK);
+
+ _done:
+
     lock_ReleaseMutex(&scp->mx);
 
     return code;
@@ -346,7 +354,8 @@ long cm_CheckNTOpen(cm_scache_t *scp, unsigned int desiredAccess,
 
     code = cm_SyncOp(scp, NULL, userp, reqp, rights,
                       CM_SCACHESYNC_GETSTATUS
-                      | CM_SCACHESYNC_NEEDCALLBACK);
+                     | CM_SCACHESYNC_NEEDCALLBACK
+                     | CM_SCACHESYNC_LOCK);
 
     /*
      * If the open will fail because the volume is readonly, then we will
@@ -356,7 +365,8 @@ long cm_CheckNTOpen(cm_scache_t *scp, unsigned int desiredAccess,
      */
     if (code == CM_ERROR_READONLY)
         code = CM_ERROR_NOACCESS;
-    else if (code == 0 &&
+
+    if (code == 0 &&
              ((rights & PRSFS_WRITE) || (rights & PRSFS_READ)) &&
              scp->fileType == CM_SCACHETYPE_FILE) {
         cm_key_t key;
@@ -403,8 +413,13 @@ long cm_CheckNTOpen(cm_scache_t *scp, unsigned int desiredAccess,
                }
            }
         }
+    } else if (code != 0) {
+        goto _done;
     }
 
+    cm_SyncOpDone(scp, NULL, CM_SCACHESYNC_LOCK);
+
+ _done:
     lock_ReleaseMutex(&scp->mx);
 
     return code;
@@ -3565,7 +3580,7 @@ long cm_Lock(cm_scache_t *scp, unsigned char sLockType,
                first place. */
             code = cm_LockCheckPerms(scp, Which, userp, reqp);
             if (code == 0)
-            code = CM_ERROR_WOULDBLOCK;
+               code = CM_ERROR_WOULDBLOCK;
             else if (code == CM_ERROR_NOACCESS && Which == LockRead) {
                 osi_Log0(afsd_logp, "   User has no read-lock perms.  Forcing client-side lock");
                 force_client_lock = TRUE;
@@ -3747,7 +3762,7 @@ long cm_Lock(cm_scache_t *scp, unsigned char sLockType,
                  (int)(signed char) scp->serverLock);
     } else {
         osi_Log1(afsd_logp,
-                 "cm_Lock Rejecting lock (code = %d)", code);
+                 "cm_Lock Rejecting lock (code = 0x%x)", code);
     }
 
     return code;

diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c
index 977da1f..f7bbba1 100644 (file)
--- a/src/WINNT/afsd/smb.c
+++ b/src/WINNT/afsd/smb.c
@@ -1016,6 +1016,15 @@ void smb_CleanupDeadVC(smb_vc_t *vcp)
     smb_user_t *uidpNext;
     smb_vc_t **vcpp;
 
+
+    lock_ObtainMutex(&vcp->mx);
+    if (vcp->flags & SMB_VCFLAG_CLEAN_IN_PROGRESS) {
+       lock_ReleaseMutex(&vcp->mx);
+       osi_Log1(smb_logp, "Clean of dead vcp 0x%x in progress", vcp);
+       return;
+    }
+    vcp->flags |= SMB_VCFLAG_CLEAN_IN_PROGRESS;
+    lock_ReleaseMutex(&vcp->mx);
     osi_Log1(smb_logp, "Cleaning up dead vcp 0x%x", vcp);
 
     lock_ObtainWrite(&smb_rctLock);
@@ -1092,6 +1101,9 @@ void smb_CleanupDeadVC(smb_vc_t *vcp)
     smb_ReleaseVCNoLock(vcp);
     
     lock_ReleaseWrite(&smb_rctLock);
+    lock_ObtainMutex(&vcp->mx);
+    vcp->flags &= ~SMB_VCFLAG_CLEAN_IN_PROGRESS;
+    lock_ReleaseMutex(&vcp->mx);
     osi_Log1(smb_logp, "Finished cleaning up dead vcp 0x%x", vcp);
 }
 
@@ -5437,7 +5449,7 @@ smb_Link(smb_vc_t *vcp, smb_packet_t *inp, char * oldPathp, char * newPathp)
     /* now create the hardlink */
     osi_Log1(smb_logp,"  Attempting to create new link [%s]", osi_LogSaveString(smb_logp, newLastNamep));
     code = cm_Link(newDscp, newLastNamep, sscp, 0, userp, &req);
-    osi_Log1(smb_logp,"  Link returns %d", code);
+    osi_Log1(smb_logp,"  Link returns 0x%x", code);
 
     /* Handle Change Notification */
     if (code == 0) {
@@ -6082,7 +6094,8 @@ long smb_WriteData(smb_fid_t *fidp, osi_hyper_t *offsetp, long count, char *op,
         
     /* make sure we have a writable FD */
     if (!(fidp->flags & SMB_FID_OPENWRITE)) {
-       lock_ReleaseMutex(&fidp->mx);
+       osi_Log2(smb_logp, "smb_WriteData fid %d not OPENWRITE flags 0x%x",
+                 fidp->fid, fidp->flags);
         code = CM_ERROR_BADFDOP;
         goto done;
     }
@@ -6267,14 +6280,14 @@ long smb_WriteData(smb_fid_t *fidp, osi_hyper_t *offsetp, long count, char *op,
         osi_Log1(smb_logp, "smb_WriteData fid %d calling cm_SyncOp ASYNCSTORE",
                   fidp->fid);
         code2 = cm_SyncOp(scp, NULL, userp, &req, 0, CM_SCACHESYNC_ASYNCSTORE);
-        osi_Log2(smb_logp, "smb_WriteData fid %d calling cm_SyncOp ASYNCSTORE returns %d",
-                  fidp->fid,code2);
+        osi_Log2(smb_logp, "smb_WriteData fid %d calling cm_SyncOp ASYNCSTORE returns 0x%x",
+                  fidp->fid, code2);
         lock_ReleaseMutex(&scp->mx);
         cm_QueueBKGRequest(scp, cm_BkgStore, writeBackOffset.LowPart,
                             writeBackOffset.HighPart, cm_chunkSize, 0, userp);
     }
 
-    osi_Log3(smb_logp, "smb_WriteData fid %d returns %d written %d",
+    osi_Log3(smb_logp, "smb_WriteData fid %d returns 0x%x written %d bytes",
               fidp->fid, code, *writtenp);
     return code;
 }

diff --git a/src/WINNT/afsd/smb.h b/src/WINNT/afsd/smb.h
index f25c335..6cf67ed 100644 (file)
--- a/src/WINNT/afsd/smb.h
+++ b/src/WINNT/afsd/smb.h
@@ -224,6 +224,7 @@ typedef struct smb_vc {
 #define SMB_VCFLAG_ALREADYDEAD 0x20    /* do not get tokens from this vc */
 #define SMB_VCFLAG_SESSX_RCVD  0x40    /* we received at least one session setups on this vc */
 #define SMB_VCFLAG_AUTH_IN_PROGRESS 0x80 /* a SMB NT extended authentication is in progress */
+#define SMB_VCFLAG_CLEAN_IN_PROGRESS 0x100
 
 /* one per user session */
 typedef struct smb_user {

diff --git a/src/WINNT/afsd/smb3.c b/src/WINNT/afsd/smb3.c
index 4dfe8ac..cc890bc 100644 (file)
--- a/src/WINNT/afsd/smb3.c
+++ b/src/WINNT/afsd/smb3.c
@@ -2436,13 +2436,15 @@ long smb_ReceiveTran2QFSInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t *
     switch (p->parmsp[0]) {
     case 1: responseSize = sizeof(qi.u.allocInfo); break;
     case 2: responseSize = sizeof(qi.u.volumeInfo); break;
+       break;
     case 0x102: responseSize = sizeof(qi.u.FSvolumeInfo); break;
     case 0x103: responseSize = sizeof(qi.u.FSsizeInfo); break;
     case 0x104: responseSize = sizeof(qi.u.FSdeviceInfo); break;
     case 0x105: responseSize = sizeof(qi.u.FSattributeInfo); break;
     case 0x200: /* CIFS Unix Info */
     case 0x301: /* Mac FS Info */
-    default: return CM_ERROR_INVAL;
+    default: 
+       return CM_ERROR_INVAL;
     }
 
     outp = smb_GetTran2ResponsePacket(vcp, p, op, 0, responseSize);
@@ -2907,6 +2909,7 @@ long smb_ReceiveTran2QFileInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
        goto done;
     }   
 
+    lock_ObtainMutex(&fidp->mx);
     scp = fidp->scp;
     lock_ObtainMutex(&scp->mx);
     code = cm_SyncOp(scp, NULL, userp, &req, 0,
@@ -2932,9 +2935,7 @@ long smb_ReceiveTran2QFileInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
         *((LARGE_INTEGER *)op) = scp->length; op += 8; /* alloc size */
         *((LARGE_INTEGER *)op) = scp->length; op += 8; /* EOF */
         *((u_long *)op) = scp->linkCount; op += 4;
-       lock_ObtainMutex(&fidp->mx);
         *op++ = ((fidp->flags & SMB_FID_DELONCLOSE) ? 1 : 0);
-       lock_ReleaseMutex(&fidp->mx);
         *op++ = (scp->fileType == CM_SCACHETYPE_DIRECTORY ? 1 : 0);
         *op++ = 0;
         *op++ = 0;
@@ -2959,6 +2960,7 @@ long smb_ReceiveTran2QFileInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet_t
     /* send and free the packets */
   done:
     lock_ReleaseMutex(&scp->mx);
+    lock_ReleaseMutex(&fidp->mx);
     cm_ReleaseUser(userp);
     smb_ReleaseFID(fidp);
     if (code == 0) 
@@ -2992,7 +2994,7 @@ long smb_ReceiveTran2SetFileInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet
     }
 
     infoLevel = p->parmsp[1];
-    osi_Log2(smb_logp,"ReceiveTran2SetFileInfo type=[%x] fid=[%x]", infoLevel, fid);
+    osi_Log2(smb_logp,"ReceiveTran2SetFileInfo type 0x%x fid %d", infoLevel, fid);
     if (infoLevel > 0x104 || infoLevel < 0x101) {
         osi_Log2(smb_logp, "Bad Tran2 op 0x%x infolevel 0x%x",
                   p->opcode, infoLevel);
@@ -3041,6 +3043,7 @@ long smb_ReceiveTran2SetFileInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet
         /* lock the vnode with a callback; we need the current status
          * to determine what the new status is, in some cases.
          */
+       lock_ObtainMutex(&fidp->mx);
         lock_ObtainMutex(&scp->mx);
         code = cm_SyncOp(scp, NULL, userp, &req, 0,
                           CM_SCACHESYNC_GETSTATUS
@@ -3062,9 +3065,7 @@ long smb_ReceiveTran2SetFileInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet
              lastMod.dwLowDateTime != -1 && lastMod.dwHighDateTime != -1) {
             attr.mask |= CM_ATTRMASK_CLIENTMODTIME;
             smb_UnixTimeFromLargeSearchTime(&attr.clientModTime, &lastMod);
-           lock_ObtainMutex(&fidp->mx);
             fidp->flags |= SMB_FID_MTIMESETDONE;
-           lock_ReleaseMutex(&fidp->mx);
         }
                
         attribute = *((u_long *)(p->datap + 32));
@@ -3083,6 +3084,7 @@ long smb_ReceiveTran2SetFileInfo(smb_vc_t *vcp, smb_tran2Packet_t *p, smb_packet
             }
         }
         lock_ReleaseMutex(&scp->mx);
+       lock_ReleaseMutex(&fidp->mx);
 
         /* call setattr */
         if (attr.mask)
@@ -5555,6 +5557,7 @@ long smb_ReceiveNTCreateX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp)
     if (shareAccess & FILE_SHARE_WRITE)
         fidflags |= SMB_FID_SHARE_WRITE;
 
+    osi_Log1(smb_logp, "NTCreateX fidflags 0x%x", fidflags);
     code = 0;
 
     /* For an exclusive create, we want to do a case sensitive match for the last component. */

diff --git a/src/WINNT/afsd/smb3.h b/src/WINNT/afsd/smb3.h
index 62a9c92..2167d9f 100644 (file)
--- a/src/WINNT/afsd/smb3.h
+++ b/src/WINNT/afsd/smb3.h
@@ -44,40 +44,40 @@ typedef struct smb_tran2QFSInfo {
     union {
 #pragma pack(push, 2)
         struct {
-            long FSID;                 /* file system ID */
-            long sectorsPerAllocUnit;
-            long totalAllocUnits;              /* on the disk */
-            long availAllocUnits;              /* free blocks */
+            unsigned long FSID;                        /* file system ID */
+            unsigned long sectorsPerAllocUnit;
+            unsigned long totalAllocUnits;     /* on the disk */
+            unsigned long availAllocUnits;     /* free blocks */
             unsigned short bytesPerSector;     /* bytes per sector */
         } allocInfo;
 #pragma pack(pop)
         struct {
-            long vsn;          /* volume serial number */
+            unsigned long vsn; /* volume serial number */
             char vnCount;      /* count of chars in label, incl null */
             char label[12];    /* pad out with nulls */
         } volumeInfo;
         struct {
-            FILETIME vct;      /* volume creation time */
-            long vsn;          /* volume serial number */
-            long vnCount;      /* length of volume label in bytes */
-            char res[2];       /* reserved */
-            char label[10];    /* volume label */
+            FILETIME vct;              /* volume creation time */
+            unsigned long vsn;         /* volume serial number */
+            unsigned long vnCount;     /* length of volume label in bytes */
+            char res[2];               /* reserved */
+            char label[10];            /* volume label */
         } FSvolumeInfo;
         struct {
             osi_hyper_t totalAllocUnits;       /* on the disk */
             osi_hyper_t availAllocUnits;       /* free blocks */
-            long sectorsPerAllocUnit;
-            long bytesPerSector;               /* bytes per sector */
+            unsigned long sectorsPerAllocUnit;
+            unsigned long bytesPerSector;      /* bytes per sector */
         } FSsizeInfo;
         struct {
-            long devType;      /* device type */
-            long characteristics;
+            unsigned long devType;             /* device type */
+            unsigned long characteristics;
         } FSdeviceInfo;
         struct {
-            long attributes;
-            long maxCompLength;        /* max path component length */
-            long FSnameLength; /* length of file system name */
-            char FSname[12];
+            unsigned long attributes;
+            unsigned long maxCompLength;       /* max path component length */
+            unsigned long FSnameLength;                /* length of file system name */
+            unsigned char FSname[12];
         } FSattributeInfo;
     } u;
 } smb_tran2QFSInfo_t;