From: Jeffrey Altman Date: Thu, 27 Apr 2006 16:55:38 +0000 (+0000) Subject: STABLE14-windows-deadlock-and-race-removal-20060427 X-Git-Tag: openafs-stable-1_4_1b~17 X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=47eb71737ff6619f916a6f58513158cb2a99dfa3;p=packages%2Fo%2Fopenafs.git STABLE14-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 (cherry picked from commit bf7404c3510d63b90c2cb15766f8455f79da90fe) --- diff --git a/src/WINNT/afsd/cm_buf.c b/src/WINNT/afsd/cm_buf.c index ce8e059c4..7ed08c0bf 100644 --- a/src/WINNT/afsd/cm_buf.c +++ b/src/WINNT/afsd/cm_buf.c @@ -877,7 +877,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%x for file 0x%x, offset 0x%x", + osi_Log3(buf_logp, "buf_GetNew returning bp 0x%x for scp 0x%x, offset 0x%x", bp, (long) scp, offsetp->LowPart); return 0; } @@ -1024,7 +1024,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%x for file 0x%x, offset 0x%x", + osi_Log3(buf_logp, "buf_Get returning bp 0x%x for scp 0x%x, offset 0x%x", bp, (long) scp, offsetp->LowPart); #ifdef TESTING buf_ValidateBufQueues(); diff --git a/src/WINNT/afsd/cm_vnodeops.c b/src/WINNT/afsd/cm_vnodeops.c index 83a3c7024..4da598c66 100644 --- 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; diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c index 95505130c..81b781d8d 100644 --- a/src/WINNT/afsd/smb.c +++ b/src/WINNT/afsd/smb.c @@ -966,6 +966,15 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) unsigned short uid; 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); @@ -1043,6 +1052,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); } @@ -5422,7 +5434,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) { @@ -6057,7 +6069,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; } @@ -6242,14 +6255,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 acd5d6669..840ff92cd 100644 --- 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 f570216b2..73ecdbb50 100644 --- a/src/WINNT/afsd/smb3.c +++ b/src/WINNT/afsd/smb3.c @@ -2456,13 +2456,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); @@ -2927,6 +2929,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, @@ -2952,9 +2955,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; @@ -2979,6 +2980,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) @@ -3012,7 +3014,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); @@ -3061,6 +3063,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 @@ -3082,9 +3085,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)); @@ -3103,6 +3104,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) @@ -5567,6 +5569,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 62a9c9269..2167d9fd6 100644 --- 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;