Hi Ben, Sorin plans to address some issues once he is back. Thanks, Eitan
-----Original Message----- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Eitan Eliahu Sent: Wednesday, January 07, 2015 1:16 PM To: Sorin Vinturis; dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] datapath-windows: Support for custom VXLAN tunnel port Hi Sorin, On the default port, it is also created through the user mode using the NL interface so if we create all other ports dynamically I would prefer to create the default port using the same method. (unless you see an issue here). On the concurrency issue: The spec reads as follows: " Each client session can have only one transaction in progress at a time" If we don't serialize transactions we cannot guaranty that transactions are not overlapped . You are right the second transaction will return with IN_PROGRESS but is should have really completeed successfully if we were serializing the transactions. I realize that it involves a significant work to serialize the transactions and to initiate them from another thread which runs in a lower IRQL. Let me get back to you on that ( iad this code before but removed it). Perhaps we could retry if it fails on with FWP_E_TXN_IN_PROGRESS. Thank you, Eitan -----Original Message----- From: Sorin Vinturis [mailto:svintu...@cloudbasesolutions.com] Sent: Wednesday, January 07, 2015 11:59 AM To: Eitan Eliahu; dev@openvswitch.org Subject: RE: [ovs-dev] [PATCH] datapath-windows: Support for custom VXLAN tunnel port Hi Eitan, I think we should keep the default port filter creation at the initialization phase because most users are using the default port. Regarding the new filter creation for other tunnel port, the OvsTunnelAddFilterEx function does not open the session to the filtering engine. The session is already opened when the OvsTunnelAddFilterEx function is called. It is opened during tunnel initialization phase at filter attach and is closed at filter detach. The FwpmTransactionBegin0 function begins an explicit transaction within the current session. The function cannot be called from within an already started transaction. It will fail with FWP_E_TXN_IN_PROGRESS error. So, a subsequent call to OvsTunnelAddFilterEx would be unsuccessful if a transaction is in progress. I don't think that this scenario is common. I can add an error log in case the function fails. What do you think? Thanks, Sorin -----Original Message----- From: Eitan Eliahu [mailto:elia...@vmware.com] Sent: Wednesday, 7 January, 2015 17:04 To: Sorin Vinturis; dev@openvswitch.org Subject: RE: [ovs-dev] [PATCH] datapath-windows: Support for custom VXLAN tunnel port Hi Sorin, thank you for working on this. Do you thing the default port filter had to be created during initialization ? On another thing, since OvsTunnelAddFilterEx is called with IRQL = PASSIVE and no lock is handled it may be called from multiple thread contexts. I am not sure if opening. Closing and aborting the transaction engine would be thread safe. We might need to serialize the calls and to invoke the engine from a different thread context running on PASSIVE_IRQL. Thanks, Eitan -----Original Message----- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Sorin Vinturis Sent: Wednesday, January 07, 2015 6:24 AM To: dev@openvswitch.org Subject: [ovs-dev] [PATCH] datapath-windows: Support for custom VXLAN tunnel port The kernel datapath supports only port 4789 for VXLAN tunnel creation. Added support in order to allow for the VXLAN tunnel port to be configurable to any port number set by the userspace. The patch also checks to see if an existing WFP filter, for the necessary UDP tunnel port, is already created before adding a new one. This is a double check, because currently the userspace also verifies this, but in my opinion is necessary to avoid future inconsistencies. Custom VXLAN tunnel port requires the addition of a new WFP filter with the new UDP tunnel port. The cleanest way for the creation or removal of the new WFP filters is to add them in OvsInitVxlanTunnel or OvsCleanupVxlanTunnel functions. But the latter functions are running at IRQL = DISPATCH_LEVEL, due to the NDIS RW lock acquisition, and all WFP calls must be running at IRQL = PASSIVE_LEVEL. This is why the WFP filter creation/removal is made when the spinlock is released and the IRQL level is lowered at PASSIVE_LEVEL. The default VXLAN tunnel port 4789 behaviour is unchanged. The WFP filter for UDP port 4789 is added when the extension is enabled and is removed when the extension is disabled. No other NL command is able to remove it. Now it is necessary that OvsTunnelFilterUninitialize function is called after OvsClearAllSwitchVports in order to allow for the added WFP filters to be removed. OvsTunnelFilterUninitialize function closes the global engine handle used by most of the WFP calls, including filter removal. Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> Reported-by: Alin Gabriel Serdean <aserd...@cloudbasesolutions.com> Reported-at: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs-2Dissues_issues_66&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=fVUQFtLj8jLS3oqKrhXRCrc7OwBHhpU4eYO-t9Nbm-g&s=R2DrJ23z-ZavsrI21xMGMD6w-yT16Xz2NCFc_qF7slw&e= --- datapath-windows/ovsext/Switch.c | 2 +- datapath-windows/ovsext/TunnelFilter.c | 88 ++++++++++++++++++++++++++++++++-- datapath-windows/ovsext/TunnelIntf.h | 4 ++ datapath-windows/ovsext/Vport.c | 78 ++++++++++++++++++++++++++++-- datapath-windows/ovsext/Vxlan.c | 8 +--- datapath-windows/ovsext/Vxlan.h | 1 + 6 files changed, 166 insertions(+), 15 deletions(-) diff --git a/datapath-windows/ovsext/Switch.c b/datapath-windows/ovsext/Switch.c index a228d8e..cf5e3c4 100644 --- a/datapath-windows/ovsext/Switch.c +++ b/datapath-windows/ovsext/Switch.c @@ -261,8 +261,8 @@ OvsDeleteSwitch(POVS_SWITCH_CONTEXT switchContext) if (switchContext) { dpNo = switchContext->dpNo; - OvsTunnelFilterUninitialize(gOvsExtDriverObject); OvsClearAllSwitchVports(switchContext); + OvsTunnelFilterUninitialize(gOvsExtDriverObject); OvsUninitSwitchContext(switchContext); OvsFreeMemory(switchContext); } diff --git a/datapath-windows/ovsext/TunnelFilter.c b/datapath-windows/ovsext/TunnelFilter.c index e0adc37..dac14c7 100644 --- a/datapath-windows/ovsext/TunnelFilter.c +++ b/datapath-windows/ovsext/TunnelFilter.c @@ -239,7 +239,8 @@ OvsTunnelAddFilter(PWSTR filterName, UINT64 context, const GUID *filterKey, const GUID *layerKey, - const GUID *calloutKey) + const GUID *calloutKey, + UINT64 *filterID) { NTSTATUS status = STATUS_SUCCESS; FWPM_FILTER filter = {0}; @@ -249,7 +250,9 @@ OvsTunnelAddFilter(PWSTR filterName, UNREFERENCED_PARAMETER(remotePort); UNREFERENCED_PARAMETER(direction); - filter.filterKey = *filterKey; + if (filterKey) { + filter.filterKey = *filterKey; + } filter.layerKey = *layerKey; filter.displayData.name = (wchar_t*)filterName; filter.displayData.description = (wchar_t*)filterDesc; @@ -282,7 +285,7 @@ OvsTunnelAddFilter(PWSTR filterName, status = FwpmFilterAdd(gEngineHandle, &filter, NULL, - NULL); + filterID); return status; } @@ -395,7 +398,8 @@ OvsTunnelRegisterDatagramDataCallouts(const GUID *layerKey, 0, &OVS_TUNNEL_FILTER_KEY, layerKey, - calloutKey); + calloutKey, + NULL); Exit: @@ -541,3 +545,79 @@ Exit: return status; } + +NTSTATUS +OvsTunnelAddFilterEx(UINT32 remotePort, + UINT64* filterID) +{ + NTSTATUS status = STATUS_SUCCESS; + BOOLEAN inTransaction = FALSE; + + do { + status = FwpmTransactionBegin(gEngineHandle, 0); + if (!NT_SUCCESS(status)) { + break; + } + + inTransaction = TRUE; + + status = OvsTunnelAddFilter(L"Datagram-Data OVS Filter (Inbound)", + L"address/port for UDP", + (USHORT)remotePort, + FWP_DIRECTION_INBOUND, + 0, + NULL, + &FWPM_LAYER_DATAGRAM_DATA_V4, + &OVS_TUNNEL_CALLOUT_V4, + filterID); + if (!NT_SUCCESS(status)) { + break; + } + + status = FwpmTransactionCommit(gEngineHandle); + if (!NT_SUCCESS(status)){ + break; + } + + inTransaction = FALSE; + } while (inTransaction); + + if (inTransaction) { + FwpmTransactionAbort(gEngineHandle); + } + return status; +} + +NTSTATUS +OvsTunnelRemoveFilterEx(UINT64 filterID) { + NTSTATUS status = STATUS_SUCCESS; + BOOLEAN inTransaction = FALSE; + + do { + if (!filterID) { + break; + } + + status = FwpmTransactionBegin(gEngineHandle, 0); + if (!NT_SUCCESS(status)) { + break; + } + inTransaction = TRUE; + + status = FwpmFilterDeleteById(gEngineHandle, filterID); + if (!NT_SUCCESS(status)) { + break; + } + + status = FwpmTransactionCommit(gEngineHandle); + + inTransaction = FALSE; + } while (inTransaction); + + if (inTransaction) { + FwpmTransactionAbort(gEngineHandle); + } + + return status; +} diff --git a/datapath-windows/ovsext/TunnelIntf.h b/datapath-windows/ovsext/TunnelIntf.h index b2bba30..5084a94 100644 --- a/datapath-windows/ovsext/TunnelIntf.h +++ b/datapath-windows/ovsext/TunnelIntf.h @@ -30,4 +30,8 @@ VOID OvsTunnelAddSystemProvider(HANDLE handle); VOID OvsTunnelRemoveSystemProvider(HANDLE handle); +NTSTATUS OvsTunnelAddFilterEx(UINT32 remotePort, UINT64* filterID); + +NTSTATUS OvsTunnelRemoveFilterEx(UINT64 filterID); + #endif /* __TUNNEL_INTF_H_ */ diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c index c9dfaea..c9ba19a 100644 --- a/datapath-windows/ovsext/Vport.c +++ b/datapath-windows/ovsext/Vport.c @@ -70,6 +70,8 @@ static POVS_VPORT_ENTRY OvsFindVportByHvNameW(POVS_SWITCH_CONTEXT switchContext, static NDIS_STATUS InitHvVportCommon(POVS_SWITCH_CONTEXT switchContext, POVS_VPORT_ENTRY vport, BOOLEAN newPort); +static BOOLEAN OvsIsTunnelFilterCreated(POVS_SWITCH_CONTEXT switchContext, + UINT16 udpPortDest); /* * Functions implemented in relaton to NDIS port manipulation. @@ -1316,6 +1318,12 @@ OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT switchContext) ASSERT(OvsIsTunnelVportType(vport->ovsType) || (vport->ovsType == OVS_VPORT_TYPE_INTERNAL && vport->isBridgeInternal) || vport->isPresentOnHv == TRUE); + + if (vport->priv) { + OvsTunnelRemoveFilterEx( + ((POVS_VXLAN_VPORT)vport->priv)->filterID); + } + OvsRemoveAndDeleteVport(switchContext, vport, TRUE, TRUE, NULL); } } @@ -1908,7 +1916,7 @@ Cleanup: /* * -------------------------------------------------------------------------- - * Command Handler for 'OVS_VPORT_CMD_NEW'. + * Command Handler for 'OVS_VPORT_CMD_GET'. * * The function handles the initial call to setup the dump state, as well as * subsequent calls to continue dumping data. @@ -1971,6 +1979,7 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, PCHAR portName; ULONG portNameLen; UINT32 portType; + UINT16 udpPortDest = VXLAN_UDP_PORT; BOOLEAN isBridgeInternal = FALSE; BOOLEAN vportAllocated = FALSE, vportInitialized = FALSE; BOOLEAN addInternalPortAsNetdev = FALSE; @@ -2044,7 +2053,10 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, vportAllocated = TRUE; if (OvsIsTunnelVportType(portType)) { - status = OvsInitTunnelVport(vport, portType, VXLAN_UDP_PORT); + PNL_ATTR attr = NlAttrFindNested(vportAttrs[OVS_VPORT_ATTR_OPTIONS], + OVS_TUNNEL_ATTR_DST_PORT); + udpPortDest = NlAttrGetU16(attr); + status = OvsInitTunnelVport(vport, portType, udpPortDest); nlError = NlMapStatusToNlErr(status); } else { OvsInitBridgeInternalVport(vport); @@ -2119,7 +2131,21 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, Cleanup: NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); - if (nlError != NL_ERROR_SUCCESS) { + if (nlError == NL_ERROR_SUCCESS) { + if (VXLAN_UDP_PORT != udpPortDest && + !OvsIsTunnelFilterCreated(gOvsSwitchContext, udpPortDest)) { + /* + * All necessary calls to the WFP filtering engine must be running + * at IRQL = PASSIVE_LEVEL. Thus the new WFP tunnel filter creation + * is added here, outside the lock which raises the IRQL to + * DISPATCH_LEVEL. + */ + if (vport->priv) { + OvsTunnelAddFilterEx(udpPortDest, + &((POVS_VXLAN_VPORT)vport->priv)->filterID); + } + } + } else { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; @@ -2269,6 +2295,7 @@ OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, NL_ERROR nlError = NL_ERROR_SUCCESS; PSTR portName = NULL; UINT32 portNameLen = 0; + UINT64 vxlanFilterID = 0; static const NL_POLICY ovsVportPolicy[] = { [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32, .optional = TRUE }, @@ -2308,6 +2335,14 @@ OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, nlError = NL_ERROR_NODEV; goto Cleanup; } + + if (vport->priv) { + /* + * Get VXLAN filterID before vport deletion in order to remove the + * WFP filter. + */ + vxlanFilterID = ((POVS_VXLAN_VPORT)vport->priv)->filterID; + } status = OvsCreateMsgFromVport(vport, msgIn, usrParamsCtx->outputBuffer, usrParamsCtx->outputLength, @@ -2324,6 +2359,10 @@ OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, Cleanup: NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); + if (vxlanFilterID) { + OvsTunnelRemoveFilterEx(vxlanFilterID); + } + if (nlError != NL_ERROR_SUCCESS) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; @@ -2334,3 +2373,36 @@ Cleanup: return STATUS_SUCCESS; } + +static BOOLEAN +OvsIsTunnelFilterCreated(POVS_SWITCH_CONTEXT switchContext, + UINT16 udpPortDest) { + for (UINT hash = 0; hash < OVS_MAX_VPORT_ARRAY_SIZE; hash++) { + PLIST_ENTRY head, link, next; + + head = &(switchContext->portNoHashArray[hash & OVS_VPORT_MASK]); + LIST_FORALL_SAFE(head, link, next) { + POVS_VPORT_ENTRY vport = NULL; + POVS_VXLAN_VPORT vxlanPort = NULL; + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink); + vxlanPort = (POVS_VXLAN_VPORT)vport->priv; + if (vxlanPort) { + if ((udpPortDest == vxlanPort->dstPort) && + (0 != vxlanPort->filterID)) { + /* + * VXLAN destination port matching is not enough to decide + * if the WFP filter was created or not, because the newly + * allocated VXLAN vport is already added to the + * portNoHashArray in InitOvsVportCommon function. If the + * filterID of the VXLAN vport is zero it means that the + * WFP filter is not created yet. + */ + return TRUE; + } + } + } + } + + return FALSE; +} \ No newline at end of file diff --git a/datapath-windows/ovsext/Vxlan.c b/datapath-windows/ovsext/Vxlan.c index 1ce5af2..8f40b6a 100644 --- a/datapath-windows/ovsext/Vxlan.c +++ b/datapath-windows/ovsext/Vxlan.c @@ -57,7 +57,7 @@ NTSTATUS OvsInitVxlanTunnel(POVS_VPORT_ENTRY vport, UINT16 udpDestPort) { - POVS_VXLAN_VPORT vxlanPort; + POVS_VXLAN_VPORT vxlanPort = NULL; vxlanPort = OvsAllocateMemory(sizeof (*vxlanPort)); if (vxlanPort == NULL) { @@ -66,12 +66,6 @@ OvsInitVxlanTunnel(POVS_VPORT_ENTRY vport, RtlZeroMemory(vxlanPort, sizeof(*vxlanPort)); vxlanPort->dstPort = udpDestPort; - /* - * since we are installing the WFP filter before the port is created - * We need to check if it is the same number - * XXX should be removed later - */ - ASSERT(vxlanPort->dstPort == VXLAN_UDP_PORT); vport->priv = (PVOID)vxlanPort; return STATUS_SUCCESS; diff --git a/datapath-windows/ovsext/Vxlan.h b/datapath-windows/ovsext/Vxlan.h index d84796d..4343da1 100644 --- a/datapath-windows/ovsext/Vxlan.h +++ b/datapath-windows/ovsext/Vxlan.h @@ -24,6 +24,7 @@ typedef struct _OVS_VXLAN_VPORT { UINT64 outPkts; UINT64 slowInPkts; UINT64 slowOutPkts; + UINT64 filterID; /* * To be filled */ -- 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=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=fVUQFtLj8jLS3oqKrhXRCrc7OwBHhpU4eYO-t9Nbm-g&s=J3wP7n4jBwn5krFRF2RrwDRv1tRN_yxfZF6K_tpNT7s&e= _______________________________________________ dev mailing list dev@openvswitch.org https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=0PJMRqoiQGgjgV8XRSA6FUJr47k0iVlG9CRNKAqh3pY&s=ciUMGo6A6FQguvNbGkODMbvZauqOvdddoL9gG6Sdtbk&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
