If an application calls Connect or Accept, their IRP is queued to a work queue for asynchronous processing. However, if the application crashes or exits before the work queue can process the IRP, the cleanup code will call WvEpFree(). This destroys the IbCmId.
When the work queue finally runs, it can access a freed IbCmId. This is bad. A similar race exists with the QP and the asynchronous disconnect processing. The disconnect processing can access a the hVerbsQp handle after it has been destroyed. Additionally, in all three cases, the IRPs assume that the WV provider is able to process IRPs. Specifically, they require that the index tables maintained by the provider are still valid. References must be held on the WV provider until the IRPs finish their processing to ensure this. Fix invalid accesses to the IbCmId and hVerbsQp handles by locking around their use after valid state checks. In the case of the QP, we add a guarded mutex for synchronization purposes and use that in place where the PD mutex had been used. Signed-off-by: Sean Hefty <[email protected]> --- This patch should go in WinOF 2.1 and replaces the patch titled: [PATCH] winverbs: convert connect/accept to sync calls attached as patch file wv-ep-sync.diff Index: core/winverbs/kernel/wv_ep.c =================================================================== --- core/winverbs/kernel/wv_ep.c (revision 2387) +++ core/winverbs/kernel/wv_ep.c (working copy) @@ -181,6 +181,10 @@ void WvEpFree(WV_ENDPOINT *pEndpoint) { + WdfObjectAcquireLock(pEndpoint->Queue); + pEndpoint->State = WvEpDestroying; + WdfObjectReleaseLock(pEndpoint->Queue); + if (pEndpoint->pIbCmId != NULL) { IbCmInterface.CM.destroy_id(pEndpoint->pIbCmId); } @@ -369,6 +373,10 @@ pEndpoint->Attributes.Param.Connect.DataLength = len; } +/* + * The QP transition to error may be done from an async worker thread. + * Synchronize against application exit. + */ static NTSTATUS WvEpModifyQpErr(WV_QUEUE_PAIR *pQp, UINT8 *pVerbsData, UINT32 VerbsSize) { @@ -376,14 +384,24 @@ ib_api_status_t ib_status; NTSTATUS status; + KeAcquireGuardedMutex(&pQp->Lock); + if (pQp->hVerbsQp == NULL) { + status = STATUS_NOT_FOUND; + goto out; + } + attr.req_state = IB_QPS_ERROR; ib_status = pQp->pVerbs->ndi_modify_qp(pQp->hVerbsQp, &attr, NULL, VerbsSize, pVerbsData); - if (ib_status != IB_SUCCESS) { - return STATUS_UNSUCCESSFUL; + if (ib_status == IB_SUCCESS) { + status = STATUS_SUCCESS; + } else { + status = STATUS_UNSUCCESSFUL; } - return STATUS_SUCCESS; +out: + KeReleaseGuardedMutex(&pQp->Lock); + return status; } static NTSTATUS WvEpDisconnectQp(WV_PROVIDER *pProvider, UINT64 QpId, @@ -439,6 +457,7 @@ complete: WdfRequestCompleteWithInformation(request, status, outlen); + WvProviderPut(prov); } // We use IRP DriverContext to queue the request for further processing, @@ -451,6 +470,9 @@ NTSTATUS status; WdfObjectAcquireLock(pEndpoint->Queue); + if (pEndpoint->State == WvEpDestroying) { + goto release; + } pEndpoint->State = WvEpDisconnected; status = WdfIoQueueRetrieveNextRequest(pEndpoint->Queue, &request); @@ -463,6 +485,7 @@ work = WorkEntryFromIrp(WdfRequestWdmGetIrp(request)); WdfRequestSetInformation(request, DiscStatus); WorkEntryInit(work, WvEpDisconnectHandler, request); + WvProviderGet(pEndpoint->pProvider); WorkQueueInsert(&pEndpoint->pProvider->WorkQueue, work); } else { WdfRequestComplete(request, DiscStatus); @@ -471,6 +494,7 @@ WdfObjectAcquireLock(pEndpoint->Queue); status = WdfIoQueueRetrieveNextRequest(pEndpoint->Queue, &request); } +release: WdfObjectReleaseLock(pEndpoint->Queue); } @@ -635,6 +659,7 @@ if (!NT_SUCCESS(status)) { WdfRequestComplete(request, status); } + WvProviderPut(prov); } static void WvEpProcessAsync(WV_PROVIDER *pProvider, WDFREQUEST Request, @@ -644,6 +669,7 @@ work = WorkEntryFromIrp(WdfRequestWdmGetIrp(Request)); WorkEntryInit(work, AsyncHandler, Request); + WvProviderGet(pProvider); WorkQueueInsert(&pProvider->WorkQueue, work); } @@ -864,6 +890,7 @@ if (!NT_SUCCESS(status)) { WdfRequestComplete(request, status); } + WvProviderPut(prov); } void WvEpAccept(WV_PROVIDER *pProvider, WDFREQUEST Request) Index: core/winverbs/kernel/wv_ep.h =================================================================== --- core/winverbs/kernel/wv_ep.h (revision 2373) +++ core/winverbs/kernel/wv_ep.h (working copy) @@ -53,7 +53,8 @@ WvEpConnected, WvEpActiveDisconnect, WvEpPassiveDisconnect, - WvEpDisconnected + WvEpDisconnected, + WvEpDestroying } WV_ENDPOINT_STATE; Index: core/winverbs/kernel/wv_qp.c =================================================================== --- core/winverbs/kernel/wv_qp.c (revision 2387) +++ core/winverbs/kernel/wv_qp.c (working copy) @@ -444,6 +444,7 @@ qp->pProvider = pProvider; KeInitializeEvent(&qp->Event, NotificationEvent, FALSE); InitializeListHead(&qp->McList); + KeInitializeGuardedMutex(&qp->Lock); status = WvQpCreateAcquire(pProvider, qp, inattr); if (!NT_SUCCESS(status)) { goto err2; @@ -518,11 +519,21 @@ WdfRequestComplete(Request, status); } +/* + * Synchronize freeing the QP due to application exit with asynchronous + * disconnect processing transitioning the QP into the error state. + */ void WvQpFree(WV_QUEUE_PAIR *pQp) { WV_MULTICAST *mc; LIST_ENTRY *entry; + ib_qp_handle_t hqp; + KeAcquireGuardedMutex(&pQp->Lock); + hqp = pQp->hVerbsQp; + pQp->hVerbsQp = NULL; + KeReleaseGuardedMutex(&pQp->Lock); + while ((entry = RemoveHeadList(&pQp->McList)) != &pQp->McList) { mc = CONTAINING_RECORD(entry, WV_MULTICAST, Entry); if (mc->hVerbsMc != NULL) { @@ -535,8 +546,8 @@ KeWaitForSingleObject(&pQp->Event, Executive, KernelMode, FALSE, NULL); } - if (pQp->hVerbsQp != NULL) { - pQp->pVerbs->destroy_qp(pQp->hVerbsQp, 0); + if (hqp != NULL) { + pQp->pVerbs->destroy_qp(hqp, 0); } if (pQp->pSrq != NULL) { @@ -658,9 +669,9 @@ goto err3; } - KeAcquireGuardedMutex(&qp->pPd->Lock); + KeAcquireGuardedMutex(&qp->Lock); InsertHeadList(&qp->McList, &pmc->Entry); - KeReleaseGuardedMutex(&qp->pPd->Lock); + KeReleaseGuardedMutex(&qp->Lock); WvQpRelease(qp); WdfRequestComplete(Request, STATUS_SUCCESS); @@ -694,14 +705,14 @@ goto complete; } - KeAcquireGuardedMutex(&qp->pPd->Lock); + KeAcquireGuardedMutex(&qp->Lock); for (entry = qp->McList.Flink; entry != &qp->McList; entry = entry->Flink) { pmc = CONTAINING_RECORD(entry, WV_MULTICAST, Entry); if (RtlCompareMemory(&pmc->Gid, &mc->Gid, sizeof(pmc->Gid)) == sizeof(pmc->Gid) && pmc->Lid == (NET16) mc->Id.Data) { RemoveEntryList(&pmc->Entry); - KeReleaseGuardedMutex(&qp->pPd->Lock); + KeReleaseGuardedMutex(&qp->Lock); if (pmc->hVerbsMc != NULL) { qp->pVerbs->detach_mcast(pmc->hVerbsMc); @@ -712,7 +723,7 @@ goto release; } } - KeReleaseGuardedMutex(&qp->pPd->Lock); + KeReleaseGuardedMutex(&qp->Lock); status = STATUS_NOT_FOUND; release: Index: core/winverbs/kernel/wv_qp.h =================================================================== --- core/winverbs/kernel/wv_qp.h (revision 2373) +++ core/winverbs/kernel/wv_qp.h (working copy) @@ -55,6 +55,7 @@ LIST_ENTRY Entry; LIST_ENTRY McList; + KGUARDED_MUTEX Lock; KEVENT Event; LONG Ref;
wv-ep-async.diff
Description: Binary data
_______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
