[ovs-dev] [PATCH] datapath-windows: Updated WFP system provider handling
Eitan Eliahu
eliahue at vmware.com
Tue Mar 24 04:36:39 UTC 2015
Thanks Sorin,
What would be the case where BFE is not started before the driver initializes?
When BFE is not started does the driver continue with WFP rule setiings?
Eitan
-----Original Message-----
From: Sorin Vinturis [mailto:svinturis at cloudbasesolutions.com]
Sent: Monday, March 23, 2015 4:40 PM
To: Sorin Vinturis; Eitan Eliahu
Cc: dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system provider handling
As I explained in the patch description, I have added this change because, at driver initialization phase, the system provider failed to be added due to the fact that the Base Filtering Engine (BFE) is not started and no session to the engine could be acquired, i.e. OvsTunnelEngineOpen() fails.
-----Original Message-----
From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Sorin Vinturis
Sent: Tuesday, 24 March, 2015 01:01
To: Eitan Eliahu
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Updated WFP system provider handling
Hi Eitan,
In this case the second call will enter this code path:
status = FwpmProviderAdd(handle,
&provider,
NULL);
if (!NT_SUCCESS(status)) {
- OVS_LOG_ERROR("Fail to add WFP provider, status: %x.", status);
- break;
+ if (STATUS_FWP_ALREADY_EXISTS != status) {
+ OVS_LOG_ERROR("Failed to add WFP provider, status: %x.",
+ status);
+ break;
+ }
}
Thanks,
Sorin
-----Original Message-----
From: Eitan Eliahu [mailto:eliahue at vmware.com]
Sent: Monday, 23 March, 2015 18:13
To: Sorin Vinturis
Cc: dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system provider handling
Sorin,
Ok on [1]. Thanks.
On [2], I can see a scenario when the provider can added twice from OvsRegisterSystemProvider() as well as from the callback
if (FWPM_SERVICE_RUNNING == FwpmBfeStateGet()) { ----> Callback is called from a different thread context !
OvsTunnelEngineOpen(&handle);
How do you prevent it?
Also, can you please explain which issue you ran into which triggered this change?
Thank you,
Eitan
-----Original Message-----
From: Sorin Vinturis [mailto:svinturis at cloudbasesolutions.com]
Sent: Monday, March 23, 2015 1:37 AM
To: Eitan Eliahu
Cc: dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system provider handling
Hi Eitan,
Thanks for reviewing my patch.
[1] The OvsTunnelEngineClose(HANDLE *handle) is verifying the handle it receives before closing it.
[2] In order to retrieve the current state of the filter engine I am using the FwpmBfeStateGet function. But before calling FwpmBfeStateGet, the callout driver MUST call the FwpmBfeStateSubscribeChanges function. That is the reason I am registering the callback no mater of the state of the filter engine.
Only if the engine is running the provider is added by opening the engine handle first and closing it after the provider is added, then unsubscribes the callback.
If the engine is not running the callback is called and the provider is added only for the service running notification.
Hope this answers your comments.
Thanks,
Sorin
-----Original Message-----
From: Eitan Eliahu [mailto:eliahue at vmware.com]
Sent: Monday, 23 March, 2015 02:32
To: Sorin Vinturis
Cc: dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system provider handling
Hi Sorin,
Thank you for posting this change.
Here are few comments:
[1] Please call OvsTunnelEngineClose(&handle) only if the handle is not zero. (you might want to add an assertion if it zero).
[2] I'm somewhat confused about the registration for engine state change. As I understand you check the state of the engine and if it is running you open the engine handle (in the context of the foreground thread). However, you subscribe to state change anyway and you open the handle to the engine from the callback function too.
Did I miss anything?
Thanks,
Eitan
-----Original Message-----
From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Sorin Vinturis
Sent: Monday, 16 March, 2015 20:08
To: dev at openvswitch.org
Subject: [ovs-dev] [PATCH] datapath-windows: Updated WFP system provider handling
If the Base Filtering Engine (BFE) is not started, the WFP system provider failed to be added because no session to the engine could be acquired.
The solution for this was to registered a BFE notification callback that is called whenever the BFE's state changes. Only if the BFE's state is running the WFP system provider is added.
Signed-off-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
Reported-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
Reported-at: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs-2Dissues_issues_65&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=MVriWGIsZGTQ5f_haLS0X5H8R3YdfikbqnWKRpKksKU&s=1iWqFul95i3o_mD41jKjZNdLnK-df_QWRzSY1614RTw&e=
---
datapath-windows/ovsext/Datapath.c | 20 ++-----
datapath-windows/ovsext/TunnelFilter.c | 99 ++++++++++++++++++++++++++++++++--
datapath-windows/ovsext/TunnelIntf.h | 8 +--
3 files changed, 100 insertions(+), 27 deletions(-)
diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index 8eb13f1..c6fe89e 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -353,35 +353,19 @@ PNDIS_SPIN_LOCK gOvsCtrlLock; VOID
OvsInit()
{
- HANDLE handle = NULL;
-
gOvsCtrlLock = &ovsCtrlLockObj;
NdisAllocateSpinLock(gOvsCtrlLock);
OvsInitEventQueue();
-
- OvsTunnelEngineOpen(&handle);
- if (handle) {
- OvsTunnelAddSystemProvider(handle);
- }
- OvsTunnelEngineClose(&handle);
}
VOID
OvsCleanup()
{
- HANDLE handle = NULL;
-
OvsCleanupEventQueue();
if (gOvsCtrlLock) {
NdisFreeSpinLock(gOvsCtrlLock);
gOvsCtrlLock = NULL;
}
-
- OvsTunnelEngineOpen(&handle);
- if (handle) {
- OvsTunnelRemoveSystemProvider(handle);
- }
- OvsTunnelEngineClose(&handle);
}
VOID
@@ -448,6 +432,8 @@ OvsCreateDeviceObject(NDIS_HANDLE ovsExtDriverHandle)
if (ovsExt) {
ovsExt->numberOpenInstance = 0;
}
+ } else {
+ OvsRegisterSystemProvider((PVOID)gOvsDeviceObject);
}
OVS_LOG_TRACE("DeviceObject: %p", gOvsDeviceObject); @@ -471,6 +457,8 @@ OvsDeleteDeviceObject()
NdisDeregisterDeviceEx(gOvsDeviceHandle);
gOvsDeviceHandle = NULL;
gOvsDeviceObject = NULL;
+
+ OvsUnregisterSystemProvider();
}
}
diff --git a/datapath-windows/ovsext/TunnelFilter.c b/datapath-windows/ovsext/TunnelFilter.c
index e0adc37..4b879c0 100644
--- a/datapath-windows/ovsext/TunnelFilter.c
+++ b/datapath-windows/ovsext/TunnelFilter.c
@@ -111,6 +111,7 @@ DEFINE_GUID(
PDEVICE_OBJECT gDeviceObject;
HANDLE gEngineHandle = NULL;
+HANDLE gBfeSubscriptionHandle = NULL;
UINT32 gCalloutIdV4;
@@ -173,17 +174,20 @@ OvsTunnelAddSystemProvider(HANDLE handle)
provider.displayData.name = OVS_TUNNEL_PROVIDER_NAME;
provider.displayData.description = OVS_TUNNEL_PROVIDER_DESC;
/*
- * Since we always want the provider to be present, it's easiest to add
- * it as persistent object during driver load.
- */
+ * Since we always want the provider to be present, it's easiest to add
+ * it as persistent object during driver load.
+ */
provider.flags = FWPM_PROVIDER_FLAG_PERSISTENT;
status = FwpmProviderAdd(handle,
&provider,
NULL);
if (!NT_SUCCESS(status)) {
- OVS_LOG_ERROR("Fail to add WFP provider, status: %x.", status);
- break;
+ if (STATUS_FWP_ALREADY_EXISTS != status) {
+ OVS_LOG_ERROR("Failed to add WFP provider, status: %x.",
+ status);
+ break;
+ }
}
status = FwpmTransactionCommit(handle); @@ -541,3 +545,88 @@ Exit:
return status;
}
+
+VOID NTAPI
+OvsBfeStateChangeCallback(PVOID context,
+ FWPM_SERVICE_STATE bfeState) {
+ HANDLE handle = NULL;
+
+ DBG_UNREFERENCED_PARAMETER(context);
+
+ if (FWPM_SERVICE_RUNNING == bfeState) {
+ OvsTunnelEngineOpen(&handle);
+ if (handle) {
+ OvsTunnelAddSystemProvider(handle);
+ }
+ OvsTunnelEngineClose(&handle);
+ }
+}
+
+NTSTATUS
+OvsSubscribeBfeStateChanges(PVOID deviceObject) {
+ NTSTATUS status = STATUS_SUCCESS;
+
+ if (!gBfeSubscriptionHandle) {
+ status = FwpmBfeStateSubscribeChanges(deviceObject,
+ OvsBfeStateChangeCallback,
+ NULL,
+ &gBfeSubscriptionHandle);
+ if (!NT_SUCCESS(status)) {
+ OVS_LOG_ERROR(
+ "Failed to open subscribe BFE state change callback, status: %x.",
+ status);
+ }
+ }
+
+ return status;
+}
+
+VOID
+OvsUnsubscribeBfeStateChanges()
+{
+ NTSTATUS status = STATUS_SUCCESS;
+
+ if (gBfeSubscriptionHandle) {
+ status = FwpmBfeStateUnsubscribeChanges(gBfeSubscriptionHandle);
+ if (!NT_SUCCESS(status)) {
+ OVS_LOG_ERROR(
+ "Failed to open unsubscribe BFE state change callback, status: %x.",
+ status);
+ }
+ gBfeSubscriptionHandle = NULL;
+ }
+}
+
+VOID OvsRegisterSystemProvider(PVOID deviceObject) {
+ NTSTATUS status = STATUS_SUCCESS;
+ HANDLE handle = NULL;
+
+ status = OvsSubscribeBfeStateChanges(deviceObject);
+ if (NT_SUCCESS(status)) {
+ if (FWPM_SERVICE_RUNNING == FwpmBfeStateGet()) {
+ OvsTunnelEngineOpen(&handle);
+ if (handle) {
+ OvsTunnelAddSystemProvider(handle);
+ }
+ OvsTunnelEngineClose(&handle);
+
+ OvsUnsubscribeBfeStateChanges();
+ }
+ }
+}
+
+VOID OvsUnregisterSystemProvider()
+{
+ HANDLE handle = NULL;
+
+ OvsTunnelEngineOpen(&handle);
+ if (handle) {
+ OvsTunnelRemoveSystemProvider(handle);
+ }
+ OvsTunnelEngineClose(&handle);
+
+ OvsUnsubscribeBfeStateChanges();
+}
diff --git a/datapath-windows/ovsext/TunnelIntf.h b/datapath-windows/ovsext/TunnelIntf.h
index b2bba30..728a53f 100644
--- a/datapath-windows/ovsext/TunnelIntf.h
+++ b/datapath-windows/ovsext/TunnelIntf.h
@@ -22,12 +22,8 @@ NTSTATUS OvsTunnelFilterInitialize(PDRIVER_OBJECT driverObject);
VOID OvsTunnelFilterUninitialize(PDRIVER_OBJECT driverObject);
-NTSTATUS OvsTunnelEngineOpen(HANDLE *handle);
+VOID OvsRegisterSystemProvider(PVOID deviceObject);
-VOID OvsTunnelEngineClose(HANDLE *handle);
-
-VOID OvsTunnelAddSystemProvider(HANDLE handle);
-
-VOID OvsTunnelRemoveSystemProvider(HANDLE handle);
+VOID OvsUnregisterSystemProvider();
#endif /* __TUNNEL_INTF_H_ */
--
1.9.0.msysgit.0
_______________________________________________
dev mailing list
dev at 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=MVriWGIsZGTQ5f_haLS0X5H8R3YdfikbqnWKRpKksKU&s=IRTZMDQzH2bWNFdd7KZkHDqBBXjSOUx5lc0Z5tcIwag&e=
_______________________________________________
dev mailing list
dev at 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=mT4lWtSM2tE_562pKsxJG3mDy1aE2fJR2aZfmUkTut4&s=UKaI73R8LrWspWbBYRIQT7jjdvvNs7YcESLYPRPkppY&e=
More information about the dev
mailing list