winverbs: fix race in async connect handling

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@intel.com>
git-svn-id: svn://openib.tc.cornell.edu/gen1/trunk@2410 ad392aa1-c5ef-ae45-8dd8-e69d62a5ef86

diff --git a/core/winverbs/kernel/wv_ep.c b/core/winverbs/kernel/wv_ep.c
index 16d5e87..9a3350c 100644 (file)
--- a/core/winverbs/kernel/wv_ep.c
+++ b/core/winverbs/kernel/wv_ep.c
@@ -181,6 +181,10 @@ out:
 \r
 void WvEpFree(WV_ENDPOINT *pEndpoint)\r
 {\r
+       WdfObjectAcquireLock(pEndpoint->Queue);\r
+       pEndpoint->State = WvEpDestroying;\r
+       WdfObjectReleaseLock(pEndpoint->Queue);\r
+\r
        if (pEndpoint->pIbCmId != NULL) {\r
                IbCmInterface.CM.destroy_id(pEndpoint->pIbCmId);\r
        }\r
@@ -369,6 +373,10 @@ static void WvEpSaveReject(WV_ENDPOINT *pEndpoint, iba_cm_rej_event *pReject)
        pEndpoint->Attributes.Param.Connect.DataLength = len;\r
 }\r
 \r
+/*\r
+ * The QP transition to error may be done from an async worker thread.\r
+ * Synchronize against application exit.\r
+ */\r
 static NTSTATUS WvEpModifyQpErr(WV_QUEUE_PAIR *pQp,\r
                                                                UINT8 *pVerbsData, UINT32 VerbsSize)\r
 {\r
@@ -376,14 +384,24 @@ static NTSTATUS WvEpModifyQpErr(WV_QUEUE_PAIR *pQp,
        ib_api_status_t         ib_status;\r
        NTSTATUS                        status;\r
 \r
+       KeAcquireGuardedMutex(&pQp->Lock);\r
+       if (pQp->hVerbsQp == NULL) {\r
+               status = STATUS_NOT_FOUND;\r
+               goto out;\r
+       }\r
+\r
        attr.req_state = IB_QPS_ERROR;\r
        ib_status = pQp->pVerbs->ndi_modify_qp(pQp->hVerbsQp, &attr, NULL,\r
                                                                                   VerbsSize, pVerbsData);\r
-       if (ib_status != IB_SUCCESS) {\r
-               return STATUS_UNSUCCESSFUL;\r
+       if (ib_status == IB_SUCCESS) {\r
+               status = STATUS_SUCCESS;\r
+       } else {        \r
+               status = STATUS_UNSUCCESSFUL;\r
        }\r
 \r
-       return STATUS_SUCCESS;\r
+out:\r
+       KeReleaseGuardedMutex(&pQp->Lock);\r
+       return status;\r
 }\r
 \r
 static NTSTATUS WvEpDisconnectQp(WV_PROVIDER *pProvider, UINT64 QpId,\r
@@ -439,6 +457,7 @@ static void WvEpDisconnectHandler(WORK_ENTRY *pWork)
 \r
 complete:\r
        WdfRequestCompleteWithInformation(request, status, outlen);\r
+       WvProviderPut(prov);\r
 }\r
 \r
 // We use IRP DriverContext to queue the request for further processing,\r
@@ -451,6 +470,9 @@ static void WvEpCompleteDisconnect(WV_ENDPOINT *pEndpoint, NTSTATUS DiscStatus)
        NTSTATUS                                status;\r
 \r
        WdfObjectAcquireLock(pEndpoint->Queue);\r
+       if (pEndpoint->State == WvEpDestroying) {\r
+               goto release;\r
+       }\r
        pEndpoint->State = WvEpDisconnected;\r
 \r
        status = WdfIoQueueRetrieveNextRequest(pEndpoint->Queue, &request);\r
@@ -463,6 +485,7 @@ static void WvEpCompleteDisconnect(WV_ENDPOINT *pEndpoint, NTSTATUS DiscStatus)
                        work = WorkEntryFromIrp(WdfRequestWdmGetIrp(request));\r
                        WdfRequestSetInformation(request, DiscStatus);\r
                        WorkEntryInit(work, WvEpDisconnectHandler, request);\r
+                       WvProviderGet(pEndpoint->pProvider);\r
                        WorkQueueInsert(&pEndpoint->pProvider->WorkQueue, work);\r
                } else {\r
                        WdfRequestComplete(request, DiscStatus);\r
@@ -471,6 +494,7 @@ static void WvEpCompleteDisconnect(WV_ENDPOINT *pEndpoint, NTSTATUS DiscStatus)
                WdfObjectAcquireLock(pEndpoint->Queue);\r
                status = WdfIoQueueRetrieveNextRequest(pEndpoint->Queue, &request);\r
        }\r
+release:\r
        WdfObjectReleaseLock(pEndpoint->Queue);\r
 }\r
 \r
@@ -635,6 +659,7 @@ complete:
        if (!NT_SUCCESS(status)) {\r
                WdfRequestComplete(request, status);\r
        }\r
+       WvProviderPut(prov);\r
 }\r
 \r
 static void WvEpProcessAsync(WV_PROVIDER *pProvider, WDFREQUEST Request,\r
@@ -644,6 +669,7 @@ static void WvEpProcessAsync(WV_PROVIDER *pProvider, WDFREQUEST Request,
 \r
        work = WorkEntryFromIrp(WdfRequestWdmGetIrp(Request));\r
        WorkEntryInit(work, AsyncHandler, Request);\r
+       WvProviderGet(pProvider);\r
        WorkQueueInsert(&pProvider->WorkQueue, work);\r
 }\r
 \r
@@ -864,6 +890,7 @@ complete:
        if (!NT_SUCCESS(status)) {\r
                WdfRequestComplete(request, status);\r
        }\r
+       WvProviderPut(prov);\r
 }\r
 \r
 void WvEpAccept(WV_PROVIDER *pProvider, WDFREQUEST Request)\r

diff --git a/core/winverbs/kernel/wv_ep.h b/core/winverbs/kernel/wv_ep.h
index 10d0d7d..f6d48d3 100644 (file)
--- a/core/winverbs/kernel/wv_ep.h
+++ b/core/winverbs/kernel/wv_ep.h
@@ -53,7 +53,8 @@ typedef enum _WV_ENDPOINT_STATE
        WvEpConnected,\r
        WvEpActiveDisconnect,\r
        WvEpPassiveDisconnect,\r
-       WvEpDisconnected\r
+       WvEpDisconnected,\r
+       WvEpDestroying\r
 \r
 }      WV_ENDPOINT_STATE;\r
 \r

diff --git a/core/winverbs/kernel/wv_qp.c b/core/winverbs/kernel/wv_qp.c
index 08c2a66..3f64bd5 100644 (file)
--- a/core/winverbs/kernel/wv_qp.c
+++ b/core/winverbs/kernel/wv_qp.c
@@ -444,6 +444,7 @@ void WvQpCreate(WV_PROVIDER *pProvider, WDFREQUEST Request)
        qp->pProvider = pProvider;\r
        KeInitializeEvent(&qp->Event, NotificationEvent, FALSE);\r
        InitializeListHead(&qp->McList);\r
+       KeInitializeGuardedMutex(&qp->Lock);\r
        status = WvQpCreateAcquire(pProvider, qp, inattr);\r
        if (!NT_SUCCESS(status)) {\r
                goto err2;\r
@@ -518,10 +519,20 @@ out:
        WdfRequestComplete(Request, status);\r
 }\r
 \r
+/*\r
+ * Synchronize freeing the QP due to application exit with asynchronous\r
+ * disconnect processing transitioning the QP into the error state.\r
+ */\r
 void WvQpFree(WV_QUEUE_PAIR *pQp)\r
 {\r
        WV_MULTICAST            *mc;\r
        LIST_ENTRY                      *entry;\r
+       ib_qp_handle_t          hqp;\r
+\r
+       KeAcquireGuardedMutex(&pQp->Lock);\r
+       hqp = pQp->hVerbsQp;\r
+       pQp->hVerbsQp = NULL;\r
+       KeReleaseGuardedMutex(&pQp->Lock);\r
 \r
        while ((entry = RemoveHeadList(&pQp->McList)) != &pQp->McList) {\r
                mc = CONTAINING_RECORD(entry, WV_MULTICAST, Entry);\r
@@ -535,8 +546,8 @@ void WvQpFree(WV_QUEUE_PAIR *pQp)
                KeWaitForSingleObject(&pQp->Event, Executive, KernelMode, FALSE, NULL);\r
        }\r
 \r
-       if (pQp->hVerbsQp != NULL) {\r
-               pQp->pVerbs->destroy_qp(pQp->hVerbsQp, 0);\r
+       if (hqp != NULL) {\r
+               pQp->pVerbs->destroy_qp(hqp, 0);\r
        }\r
 \r
        if (pQp->pSrq != NULL) {\r
@@ -658,9 +669,9 @@ void WvQpAttach(WV_PROVIDER *pProvider, WDFREQUEST Request)
                goto err3;\r
        }\r
 \r
-       KeAcquireGuardedMutex(&qp->pPd->Lock);\r
+       KeAcquireGuardedMutex(&qp->Lock);\r
        InsertHeadList(&qp->McList, &pmc->Entry);\r
-       KeReleaseGuardedMutex(&qp->pPd->Lock);\r
+       KeReleaseGuardedMutex(&qp->Lock);\r
 \r
        WvQpRelease(qp);\r
        WdfRequestComplete(Request, STATUS_SUCCESS);\r
@@ -694,14 +705,14 @@ void WvQpDetach(WV_PROVIDER *pProvider, WDFREQUEST Request)
                goto complete;\r
        }\r
 \r
-       KeAcquireGuardedMutex(&qp->pPd->Lock);\r
+       KeAcquireGuardedMutex(&qp->Lock);\r
        for (entry = qp->McList.Flink; entry != &qp->McList; entry = entry->Flink) {\r
                pmc = CONTAINING_RECORD(entry, WV_MULTICAST, Entry);\r
 \r
                if (RtlCompareMemory(&pmc->Gid, &mc->Gid, sizeof(pmc->Gid)) == sizeof(pmc->Gid) &&\r
                        pmc->Lid == (NET16) mc->Id.Data) {\r
                        RemoveEntryList(&pmc->Entry);\r
-                       KeReleaseGuardedMutex(&qp->pPd->Lock);\r
+                       KeReleaseGuardedMutex(&qp->Lock);\r
 \r
                        if (pmc->hVerbsMc != NULL) {\r
                                qp->pVerbs->detach_mcast(pmc->hVerbsMc);\r
@@ -712,7 +723,7 @@ void WvQpDetach(WV_PROVIDER *pProvider, WDFREQUEST Request)
                        goto release;\r
                }\r
        }\r
-       KeReleaseGuardedMutex(&qp->pPd->Lock);\r
+       KeReleaseGuardedMutex(&qp->Lock);\r
        status = STATUS_NOT_FOUND;\r
 \r
 release:\r

diff --git a/core/winverbs/kernel/wv_qp.h b/core/winverbs/kernel/wv_qp.h
index 449de59..796fb65 100644 (file)
--- a/core/winverbs/kernel/wv_qp.h
+++ b/core/winverbs/kernel/wv_qp.h
@@ -55,6 +55,7 @@ typedef struct _WV_QUEUE_PAIR
        LIST_ENTRY                              Entry;\r
        LIST_ENTRY                              McList;\r
 \r
+       KGUARDED_MUTEX                  Lock;\r
        KEVENT                                  Event;\r
        LONG                                    Ref;\r
 \r