[ovs-dev] [PATCH] datapath-windows: Enable failure after restarting extension

Sorin Vinturis svinturis at cloudbasesolutions.com
Tue Aug 4 11:27:08 UTC 2015


Nithin, please see my answers inline. Thanks!

-----Original Message-----
From: Nithin Raju [mailto:nithin at vmware.com] 
Sent: Monday, 3 August, 2015 20:22
To: Sorin Vinturis
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Enable failure after restarting extension

> On Jul 15, 2015, at 7:50 AM, Sorin Vinturis <svinturis at cloudbasesolutions.com> wrote:
> 
> If the extension was previously enabled and running, after issuing a 
> restart, stop+start, the extension fails to be enabled. This happens 
> because the extension's DeviceObject is not yet initialized before the 
> FilterAttach routine is called.
> 
> This patch addresses this issue.
> 
> 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_openvs
> witch_ovs-2Dissues_issues_96&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-
> YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=-ocq5Ewu
> 5A3PmYqKlMAHiQh2W82fu3WhPBCdk1S4PMI&s=XWbGE6dPBCtV-3-HfUG8yPDlUW6mlSor
> NqP6k27Z4Gk&e=
> ---
> This patch is for both master and 2.4 branch.
> ---
> datapath-windows/ovsext/TunnelFilter.c | 28 
> ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/TunnelFilter.c 
> b/datapath-windows/ovsext/TunnelFilter.c
> index 231750e..5466fe4 100644
> --- a/datapath-windows/ovsext/TunnelFilter.c
> +++ b/datapath-windows/ovsext/TunnelFilter.c
> @@ -825,20 +825,24 @@ OvsInitTunnelFilter(PDRIVER_OBJECT driverObject, 
> PVOID deviceObject) {
>     NTSTATUS status = STATUS_SUCCESS;
> 
> -    status = OvsSubscribeTunnelInitBfeStateChanges(driverObject, deviceObject);
> -    if (NT_SUCCESS(status)) {
> -        if (FWPM_SERVICE_RUNNING == FwpmBfeStateGet()) {
> -            status = OvsTunnelFilterInitialize(driverObject);
> -            if (!NT_SUCCESS(status)) {
> -                /* XXX: We need to decide what actions to take in case of
> -                 * failure to initialize tunnel filter. */
> -                ASSERT(status == NDIS_STATUS_SUCCESS);
> -                OVS_LOG_ERROR(
> -                    "Failed to initialize tunnel filter, status: %x.",
> -                    status);
> +    if (deviceObject) {
> +        status = OvsSubscribeTunnelInitBfeStateChanges(driverObject, deviceObject);
> +        if (NT_SUCCESS(status)) {
> +            if (FWPM_SERVICE_RUNNING == FwpmBfeStateGet()) {
> +                status = OvsTunnelFilterInitialize(driverObject);
> +                if (!NT_SUCCESS(status)) {
> +                    /* XXX: We need to decide what actions to take in case of
> +                     * failure to initialize tunnel filter. */
> +                    ASSERT(status == NDIS_STATUS_SUCCESS);
> +                    OVS_LOG_ERROR(
> +                        "Failed to initialize tunnel filter, status: %x.",
> +                        status);
> +                }
> +                OvsUnsubscribeTunnelInitBfeStateChanges();
>             }
> -            OvsUnsubscribeTunnelInitBfeStateChanges();
>         }
> +    } else {
> +        status = OvsTunnelFilterInitialize(driverObject);
>     }
> 

Why can’t we use the code from the ‘else’ case all the time? Why should there be any dependency on ‘gOvsDeviceObject’?
SV: At boot time, if the FilterAttach routine is called to enable the extension, the BFE is not running and the OvsTunnelFilterInitialize() will fail because the WFP callout cannot be added. For this case, we are registering a callback to be called when the BFE state changes and execute the OvsTunnelFilterInitialize() only if the BFE is running. This is why we can't use just the code from 'else' all the time and need the other part.

Also, in what conditions would OvsInitTunnelFilter() get called before ‘gOvsDeviceObject’ is initialized? As long as the driver is loaded, ‘gOvsDeviceObject’ should not be NULL at all.
SV: The scenario you are asking about is described in the patch description. This happens when the OS is already loaded and running and the BFE is also running. It is a race condition that unfortunately we cannot avoid and I will explain it further.
When the OVS extension is restarted, DriverEntry routine is called and the execution continues just after the call to NdisFRegisterFilterDriver(). We are calling the later function to register our FilterXxx routines to NDIS and to obtain the 'gOvsExtDriverHandle' handle in order to be used by the OvsCreateDeviceObject() and create the 'gOvsDeviceObject'. But just after the call to NdisFRegisterFilterDriver() and before calling OvsCreateDeviceObject(), the FilterAttach routine is called by NDIS in order to enable the extension. In this way the OvsInitTunnelFilter() gets called before the 'gOvsDeviceObject' is initialized.

The MSFT documentation to the NdisFRegisterFilterDriver () states the following: "Drivers that call NdisFRegisterFilterDriver must be prepared for an immediate call to any of their FilterXxx functions."
Which is what we are experiencing in the scenario I explained above.

thanks,
-- Nithin


More information about the dev mailing list