From 12d5ed1afeea3a30192791cf45d2e96102d0e38d Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Sun, 22 Oct 2006 13:25:38 +0000 Subject: [PATCH] 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) --- src/WINNT/afsd/smb.c | 17 ++++++++++------- src/WINNT/afsd/smb3.c | 38 +++++++++++++++++--------------------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c index 0d53ddd7ce..12eba606fc 100644 --- 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 5d7a612c98..b50ed8f1f6 100644 --- 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 */