[ofw] [PATCH v2] winverbs: fix race in async connect handling
Sean Hefty
sean.hefty at intel.com
Fri Aug 28 15:39:57 PDT 2009
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 <sean.hefty at intel.com>
---
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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wv-ep-async.diff
Type: application/octet-stream
Size: 6137 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20090828/2a68e805/attachment.obj>
More information about the ofw
mailing list