From: Jeffrey Altman Date: Mon, 14 May 2012 04:12:17 +0000 (-0400) Subject: Windows: Redir Ioctl thread safety X-Git-Tag: openafs-stable-1_8_0pre1~2408 X-Git-Url: https://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=ac08a0c30d1b8f4f5f13a6e78b86d32e2cb34f65 Windows: Redir Ioctl thread safety A crash dump showed that it is possible for a Cleanup to race with a Read from the ioctl file. Add reference counting to protect against crashing under such a circumstance. Change-Id: I5dada2b5855603807b48a191db46ff48043c1997 Reviewed-on: http://gerrit.openafs.org/7405 Tested-by: BuildBot Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- diff --git a/src/WINNT/afsrdr/user/RDRIoctl.c b/src/WINNT/afsrdr/user/RDRIoctl.c index 804074a..bb2f6a0 100644 --- a/src/WINNT/afsrdr/user/RDRIoctl.c +++ b/src/WINNT/afsrdr/user/RDRIoctl.c @@ -165,6 +165,8 @@ RDR_SetupIoctl(ULONG index, cm_fid_t *parentFid, cm_fid_t *rootFid, cm_user_t *u } if (iop) { + iop->flags = 0; + /* we are reusing a previous ioctl */ if (cm_FidCmp(&iop->parentFid, parentFid)) { iop->parentFid = *parentFid; @@ -204,6 +206,35 @@ RDR_SetupIoctl(ULONG index, cm_fid_t *parentFid, cm_fid_t *rootFid, cm_user_t *u lock_ReleaseWrite(&RDR_globalIoctlLock); } +static void +RDR_DestroyIoctl(RDR_ioctl_t *iop) +{ + if (iop == NULL) + return; + + if (iop->parentScp) + cm_ReleaseSCache(iop->parentScp); + + if (iop->ioctl.inAllocp) + free(iop->ioctl.inAllocp); + if (iop->ioctl.outAllocp) + free(iop->ioctl.outAllocp); + + if (RDR_allIoctls == RDR_allIoctlsLast) + RDR_allIoctls = RDR_allIoctlsLast = NULL; + else { + if (iop->prev == NULL) + RDR_allIoctls = iop->next; + else + iop->prev->next = iop->next; + if (iop->next == NULL) { + RDR_allIoctlsLast = iop->prev; + iop->prev->next = NULL; + } else + iop->next->prev = iop->prev; + } + free(iop); +} void RDR_CleanupIoctl(ULONG index) @@ -217,31 +248,15 @@ RDR_CleanupIoctl(ULONG index) } if (iop) { - if (iop->parentScp) - cm_ReleaseSCache(iop->parentScp); - - if (iop->ioctl.inAllocp) - free(iop->ioctl.inAllocp); - if (iop->ioctl.outAllocp) - free(iop->ioctl.outAllocp); - - if (RDR_allIoctls == RDR_allIoctlsLast) - RDR_allIoctls = RDR_allIoctlsLast = NULL; - else { - if (iop->prev == NULL) - RDR_allIoctls = iop->next; - else - iop->prev->next = iop->next; - if (iop->next == NULL) { - RDR_allIoctlsLast = iop->prev; - iop->prev->next = NULL; - } else - iop->next->prev = iop->prev; - } - free(iop); - } + if (iop->refCount == 0) { + RDR_DestroyIoctl(iop); + } + else + { + iop->flags |= RDR_IOCTL_FLAG_CLEANED; + } + } lock_ReleaseWrite(&RDR_globalIoctlLock); - } /* called when we receive a write call. If this is the first write call after @@ -324,20 +339,37 @@ RDR_FindIoctl(ULONG index) lock_ObtainRead(&RDR_globalIoctlLock); for ( iop=RDR_allIoctls; iop; iop=iop->next) { - if (iop->index == index) + if (iop->index == index && + !(iop->flags & RDR_IOCTL_FLAG_CLEANED)) { + InterlockedIncrement(&iop->refCount); break; + } } lock_ReleaseRead(&RDR_globalIoctlLock); return iop; } +void +RDR_ReleaseIoctl(RDR_ioctl_t * iop) +{ + lock_ObtainWrite(&RDR_globalIoctlLock); + InterlockedDecrement(&iop->refCount); + + if (iop->refCount == 0 && + (iop->flags & RDR_IOCTL_FLAG_CLEANED)) + { + RDR_DestroyIoctl(iop); + } + lock_ReleaseWrite(&RDR_globalIoctlLock); +} + /* called from RDR_ReceiveCoreRead when we receive a read on the ioctl fid */ afs_int32 RDR_IoctlRead(cm_user_t *userp, ULONG RequestId, ULONG BufferLength, void *MappedBuffer, ULONG *pBytesProcessed, cm_req_t *reqp, afs_uint32 pflags) { RDR_ioctl_t *iop; afs_uint32 count; - afs_int32 code; + afs_int32 code = 0; iop = RDR_FindIoctl(RequestId); if (iop == NULL) @@ -345,22 +377,23 @@ RDR_IoctlRead(cm_user_t *userp, ULONG RequestId, ULONG BufferLength, void *Mappe /* turn the connection around, if required */ code = RDR_IoctlPrepareRead(iop, userp, pflags); - if (code) - return code; + if (code == 0) { - count = (afs_int32)((iop->ioctl.outDatap - iop->ioctl.outAllocp) - iop->ioctl.outCopied); - if (BufferLength < count) - count = BufferLength; + count = (afs_int32)((iop->ioctl.outDatap - iop->ioctl.outAllocp) - iop->ioctl.outCopied); + if (BufferLength < count) + count = BufferLength; - /* now copy the data into the response packet */ - memcpy((char *)MappedBuffer, iop->ioctl.outCopied + iop->ioctl.outAllocp, count); + /* now copy the data into the response packet */ + memcpy((char *)MappedBuffer, iop->ioctl.outCopied + iop->ioctl.outAllocp, count); - /* and adjust the counters */ - iop->ioctl.outCopied += count; + /* and adjust the counters */ + iop->ioctl.outCopied += count; - *pBytesProcessed = count; + *pBytesProcessed = count; - return 0; + RDR_ReleaseIoctl(iop); + } + return code; } /* called from RDR_PioctWRite when we receive a write call on the IOCTL @@ -369,6 +402,7 @@ RDR_IoctlRead(cm_user_t *userp, ULONG RequestId, ULONG BufferLength, void *Mappe afs_int32 RDR_IoctlWrite(cm_user_t *userp, ULONG RequestId, ULONG BufferLength, void *MappedBuffer, cm_req_t *reqp) { + afs_int32 code = 0; RDR_ioctl_t *iop; iop = RDR_FindIoctl(RequestId); @@ -377,16 +411,20 @@ RDR_IoctlWrite(cm_user_t *userp, ULONG RequestId, ULONG BufferLength, void *Mapp RDR_IoctlPrepareWrite(iop); - if (BufferLength + iop->ioctl.inCopied > CM_IOCTL_MAXDATA) - return CM_ERROR_TOOBIG; - - /* copy data */ - memcpy(iop->ioctl.inDatap + iop->ioctl.inCopied, (char *)MappedBuffer, BufferLength); + if (BufferLength + iop->ioctl.inCopied > CM_IOCTL_MAXDATA) { + code = CM_ERROR_TOOBIG; + } + else + { + /* copy data */ + memcpy(iop->ioctl.inDatap + iop->ioctl.inCopied, (char *)MappedBuffer, BufferLength); - /* adjust counts */ - iop->ioctl.inCopied += BufferLength; + /* adjust counts */ + iop->ioctl.inCopied += BufferLength; + } - return 0; + RDR_ReleaseIoctl(iop); + return code; } diff --git a/src/WINNT/afsrdr/user/RDRIoctl.h b/src/WINNT/afsrdr/user/RDRIoctl.h index 0bf1689..da73a29 100644 --- a/src/WINNT/afsrdr/user/RDRIoctl.h +++ b/src/WINNT/afsrdr/user/RDRIoctl.h @@ -51,8 +51,12 @@ typedef struct RDR_ioctl { cm_fid_t rootFid; cm_scache_t *parentScp; cm_ioctl_t ioctl; + afs_uint32 flags; + afs_int32 refCount; /* RDR_globalIoctlLock */ } RDR_ioctl_t; +#define RDR_IOCTL_FLAG_CLEANED 1 + /* procedure implementing an ioctl */ typedef long (RDR_ioctlProc_t)(RDR_ioctl_t *, struct cm_user *userp, afs_uint32 pflags);