Hi Eitan, The BSOD reported in issue #53 is still reproducible with this patch. I tested and manage to reproduce the BSOD. It is normal since the request that leads to the BSOD described in issue #53 is a cleanup one and not a device control one.
In issue #53 a IRP_MJ_CLEANUP request is received and the OvsCleanupDevice cleanup routine is called, which tries to release the current instance by calling the OvsCleanupOpenInstance function. Here the event queue is empty so the OvsCleanupEvent exits quickly, then OvsCleanupPacketQueue is called. In the OvsCleanupPacketQueue function, the packet queue is also empty so no further processing is needed, but then it tries to remove the current instance from the pidHashArray, which is a member of OVS_SWITCH_CONTEXT structure. The driver crashes before accessing the pidHashArray, when it tries to acquire the pid hash lock, gOvsSwitchContext->pidHashLock, in OvsAcquirePidHashLock function. You can verify the above description from the stack trace displayed in the issue #53 description from here: https://github.com/openvswitch/ovs-issues/issues/53. That is why a check against NULL is needed for gOvsSwitchContext, before trying to remove the current instance. Thank you, Sorin -----Original Message----- From: Eitan Eliahu [mailto:elia...@vmware.com] Sent: Friday, 14 November, 2014 14:15 To: Sorin Vinturis; dev@openvswitch.org Subject: RE: [ovs-dev] [PATCH] datapath-windows: Removed all duplicate checking of Hi Sorin, Thank you for working on this issue. Can you please confirm that BSOD described in issue #53 does not happen with your change applied? Eitan -----Original Message----- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Sorin Vinturis Sent: Friday, November 14, 2014 3:35 AM To: dev@openvswitch.org Subject: [ovs-dev] [PATCH] datapath-windows: Removed all duplicate checking of Right now the gOvsSwitchContext pointer is checked against NULL in a lot of places of the OVS extension code. This check should be done only once to avoid wasteful checks. Thus I have added the check in the dispatch routine, before doing any processing, and removed all other checks from the rest of the code. Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> --- datapath-windows/ovsext/Datapath.c | 66 ++++++-------------------------------- datapath-windows/ovsext/Flow.c | 12 +++---- datapath-windows/ovsext/User.c | 11 ------- datapath-windows/ovsext/Vport.c | 4 --- 4 files changed, 13 insertions(+), 80 deletions(-) diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c index 1b504a2..6de011c 100644 --- a/datapath-windows/ovsext/Datapath.c +++ b/datapath-windows/ovsext/Datapath.c @@ -470,8 +470,7 @@ OvsGetOpenInstance(PFILE_OBJECT fileObject, POVS_OPEN_INSTANCE instance = (POVS_OPEN_INSTANCE)fileObject->FsContext; ASSERT(instance); ASSERT(instance->fileObject == fileObject); - if (gOvsSwitchContext == NULL || - gOvsSwitchContext->dpNo != dpNo) { + if (gOvsSwitchContext->dpNo != dpNo) { return NULL; } return instance; @@ -653,7 +652,6 @@ NTSTATUS OvsDeviceControl(PDEVICE_OBJECT deviceObject, PIRP irp) { - PIO_STACK_LOCATION irpSp; NTSTATUS status = STATUS_SUCCESS; PFILE_OBJECT fileObject; @@ -690,6 +688,12 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, outputBufferLen = irpSp->Parameters.DeviceIoControl.OutputBufferLength; inputBuffer = irp->AssociatedIrp.SystemBuffer; + /* Check if the extension is enabled. */ + if (NULL == gOvsSwitchContext) { + status = STATUS_DEVICE_NOT_READY; + goto done; + } + /* Concurrent netlink operations are not supported. */ if (InterlockedCompareExchange((LONG volatile *)&instance->inUse, 1, 0)) { status = STATUS_RESOURCE_IN_USE; @@ -891,7 +895,7 @@ ValidateNetlinkCmd(UINT32 devOp, /* Validate the DP for commands that require a DP. */ if (nlFamilyOps->cmds[i].validateDpIndex == TRUE) { OvsAcquireCtrlLock(); - if (!gOvsSwitchContext || ovsMsg->ovsHdr.dp_ifindex != + if (ovsMsg->ovsHdr.dp_ifindex != (INT)gOvsSwitchContext->dpNo) { status = STATUS_INVALID_PARAMETER; OvsReleaseCtrlLock(); @@ -1184,13 +1188,6 @@ HandleGetDpDump(POVS_USER_PARAMS_CONTEXT usrParamsCtx, usrParamsCtx->outputLength); OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - /* Treat this as a dump done. */ - OvsReleaseCtrlLock(); - *replyLen = 0; - FreeUserDumpState(instance); - return STATUS_SUCCESS; - } status = OvsDpFillInfo(gOvsSwitchContext, msgIn, &nlBuf); OvsReleaseCtrlLock(); @@ -1280,8 +1277,7 @@ HandleDpTransactionCommon(POVS_USER_PARAMS_CONTEXT usrParamsCtx, OvsAcquireCtrlLock(); if (dpAttrs[OVS_DP_ATTR_NAME] != NULL) { - if (!gOvsSwitchContext && - !OvsCompareString(NlAttrGet(dpAttrs[OVS_DP_ATTR_NAME]), + if (!OvsCompareString(NlAttrGet(dpAttrs[OVS_DP_ATTR_NAME]), OVS_SYSTEM_DP_NAME)) { OvsReleaseCtrlLock(); @@ -1495,13 +1491,6 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx, msgIn = instance->dumpState.ovsMsg; OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - /* Treat this as a dump done. */ - OvsReleaseCtrlLock(); - *replyLen = 0; - FreeUserDumpState(instance); - return STATUS_SUCCESS; - } /* * XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath, @@ -1629,13 +1618,6 @@ OvsGetVport(POVS_USER_PARAMS_CONTEXT usrParamsCtx, return STATUS_INVALID_BUFFER_SIZE; } - OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - OvsReleaseCtrlLock(); - return STATUS_INVALID_PARAMETER; - } - OvsReleaseCtrlLock(); - NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0); if (vportAttrs[OVS_VPORT_ATTR_NAME] != NULL) { portName = NlAttrGet(vportAttrs[OVS_VPORT_ATTR_NAME]); @@ -1776,13 +1758,6 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, return STATUS_INVALID_PARAMETER; } - OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - OvsReleaseCtrlLock(); - return STATUS_INVALID_PARAMETER; - } - OvsReleaseCtrlLock(); - portName = NlAttrGet(vportAttrs[OVS_VPORT_ATTR_NAME]); portNameLen = NlAttrGetSize(vportAttrs[OVS_VPORT_ATTR_NAME]); portType = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_TYPE]); @@ -1962,10 +1937,6 @@ OvsSetVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, } OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - OvsReleaseCtrlLock(); - return STATUS_INVALID_PARAMETER; - } NdisAcquireRWLockWrite(gOvsSwitchContext->dispatchLock, &lockState, 0); if (vportAttrs[OVS_VPORT_ATTR_NAME] != NULL) { @@ -2071,13 +2042,6 @@ OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, return STATUS_NDIS_INVALID_LENGTH; } - OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - OvsReleaseCtrlLock(); - return STATUS_INVALID_PARAMETER; - } - OvsReleaseCtrlLock(); - NdisAcquireRWLockWrite(gOvsSwitchContext->dispatchLock, &lockState, 0); if (vportAttrs[OVS_VPORT_ATTR_NAME] != NULL) { portName = NlAttrGet(vportAttrs[OVS_VPORT_ATTR_NAME]); @@ -2285,10 +2249,6 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, NlBufInit(&nlBuf, usrParamsCtx->outputBuffer, usrParamsCtx->outputLength); OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - status = STATUS_SUCCESS; - goto cleanup; - } /* remove an event entry from the event queue */ status = OvsRemoveEventEntry(usrParamsCtx->ovsInstance, &eventEntry); @@ -2337,14 +2297,6 @@ OvsReadPacketCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, /* Output buffer has been validated while validating read dev op. */ ASSERT(msgOut != NULL && usrParamsCtx->outputLength >= sizeof *msgOut); - OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - status = STATUS_SUCCESS; - OvsReleaseCtrlLock(); - return status; - } - OvsReleaseCtrlLock(); - /* Read a packet from the instance queue */ status = OvsReadDpIoctl(instance->fileObject, usrParamsCtx->outputBuffer, usrParamsCtx->outputLength, replyLen); diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c index d2d0ae5..0e88d8c 100644 --- a/datapath-windows/ovsext/Flow.c +++ b/datapath-windows/ovsext/Flow.c @@ -1985,8 +1985,7 @@ OvsDoDumpFlows(OvsFlowDumpInput *dumpInput, dpNo = dumpInput->dpNo; NdisAcquireSpinLock(gOvsCtrlLock); - if (gOvsSwitchContext == NULL || - gOvsSwitchContext->dpNo != dpNo) { + if (gOvsSwitchContext->dpNo != dpNo) { status = STATUS_INVALID_PARAMETER; goto unlock; } @@ -2137,8 +2136,7 @@ OvsPutFlowIoctl(PVOID inputBuffer, dpNo = put->dpNo; NdisAcquireSpinLock(gOvsCtrlLock); - if (gOvsSwitchContext == NULL || - gOvsSwitchContext->dpNo != dpNo) { + if (gOvsSwitchContext->dpNo != dpNo) { status = STATUS_INVALID_PARAMETER; goto unlock; } @@ -2319,8 +2317,7 @@ OvsGetFlowIoctl(PVOID inputBuffer, dpNo = getInput->dpNo; NdisAcquireSpinLock(gOvsCtrlLock); - if (gOvsSwitchContext == NULL || - gOvsSwitchContext->dpNo != dpNo) { + if (gOvsSwitchContext->dpNo != dpNo) { status = STATUS_INVALID_PARAMETER; goto unlock; } @@ -2353,8 +2350,7 @@ OvsFlushFlowIoctl(UINT32 dpNo) LOCK_STATE_EX dpLockState; NdisAcquireSpinLock(gOvsCtrlLock); - if (gOvsSwitchContext == NULL || - gOvsSwitchContext->dpNo != dpNo) { + if (gOvsSwitchContext->dpNo != dpNo) { status = STATUS_INVALID_PARAMETER; goto unlock; } diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c index fc27f7d..96adb5c 100644 --- a/datapath-windows/ovsext/User.c +++ b/datapath-windows/ovsext/User.c @@ -155,13 +155,6 @@ OvsSubscribeDpIoctl(PVOID instanceP, POVS_USER_PACKET_QUEUE queue; POVS_OPEN_INSTANCE instance = (POVS_OPEN_INSTANCE)instanceP; - OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - OvsReleaseCtrlLock(); - return STATUS_INVALID_PARAMETER; - } - OvsReleaseCtrlLock(); - if (instance->packetQueue && !join) { /* unsubscribe */ OvsCleanupPacketQueue(instance); @@ -445,10 +438,6 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) POVS_VPORT_ENTRY vport; NdisAcquireSpinLock(gOvsCtrlLock); - if (gOvsSwitchContext == NULL) { - status = STATUS_INVALID_PARAMETER; - goto unlock; - } if (execute->packetLen == 0) { status = STATUS_INVALID_PARAMETER; diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c index 68755b9..2dc5f0a 100644 --- a/datapath-windows/ovsext/Vport.c +++ b/datapath-windows/ovsext/Vport.c @@ -1373,10 +1373,6 @@ OvsGetNetdevCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, } OvsAcquireCtrlLock(); - if (!gOvsSwitchContext) { - OvsReleaseCtrlLock(); - return STATUS_INVALID_PARAMETER; - } vportGet.portNo = 0; RtlCopyMemory(&vportGet.name, NlAttrGet(netdevAttrs[OVS_VPORT_ATTR_NAME]), -- 1.9.0.msysgit.0 _______________________________________________ dev mailing list dev@openvswitch.org https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AAIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=UZO-YUDVFD8m4qcHQTELMPrV42LuveNW1GW4-AXTsUQ&s=fp--QsWd028_QIQsxFsF8dUbjlLHcRw6EMZhUG6lYU0&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
