[ovs-dev] datapath-windows: Proper cleanup in case of driver init failure
diff mbox

Message ID 1441084528-8191-1-git-send-email-svinturis@cloudbasesolutions.com
State Superseded
Headers show

Commit Message

Sorin Vinturis Sept. 1, 2015, 12:16 p.m. UTC
Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
---
 datapath-windows/ovsext/Driver.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Nithin Raju Sept. 1, 2015, 2:40 p.m. UTC | #1
Sorin,
Unless I’m missing something here, what is the new cleanup added in this patch? It seems more of code refactoring. Is that right?

-- Nithin

> On Sep 1, 2015, at 5:16 AM, Sorin Vinturis <svinturis@cloudbasesolutions.com> wrote:

> 

> Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>

> ---

> datapath-windows/ovsext/Driver.c | 20 ++++++++++++++------

> 1 file changed, 14 insertions(+), 6 deletions(-)

> 

> diff --git a/datapath-windows/ovsext/Driver.c b/datapath-windows/ovsext/Driver.c

> index 13fcde2..853886e 100644

> --- a/datapath-windows/ovsext/Driver.c

> +++ b/datapath-windows/ovsext/Driver.c

> @@ -139,18 +139,26 @@ DriverEntry(PDRIVER_OBJECT driverObject,

>     driverObject->DriverUnload = OvsExtUnload;

> 

>     status = NdisFRegisterFilterDriver(driverObject,

> -                                       (NDIS_HANDLE) gOvsExtDriverObject,

> -                                       &driverChars, &gOvsExtDriverHandle);

> +                                       (NDIS_HANDLE)gOvsExtDriverObject,

> +                                       &driverChars,

> +                                       &gOvsExtDriverHandle);

>     if (status != NDIS_STATUS_SUCCESS) {

> -        return status;

> +        goto cleanup;

>     }

> 

> -    /* Create the communication channel for usersapce. */

> +    /* Create the communication channel for userspace. */

>     status = OvsCreateDeviceObject(gOvsExtDriverHandle);

>     if (status != NDIS_STATUS_SUCCESS) {

> +        goto cleanup;

> +    }

> +

> +cleanup:

> +    if (status != NDIS_STATUS_SUCCESS){

>         OvsCleanup();

> -        NdisFDeregisterFilterDriver(gOvsExtDriverHandle);

> -        gOvsExtDriverHandle = NULL;

> +        if (gOvsExtDriverHandle) {

> +            NdisFDeregisterFilterDriver(gOvsExtDriverHandle);

> +            gOvsExtDriverHandle = NULL;

> +        }

>     }

> 

>     return status;

> -- 

> 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=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=UwM9RbSBfs6QT8UYqjkZr3xJUSmm5djIml87xTIB1WI&s=tQaG105CKQjdDBWZXvRzD9eqd2rsnJDji_xikpq6S7A&e=
Sorin Vinturis Sept. 1, 2015, 3:06 p.m. UTC | #2
Nithin,

In my tests with Driver Verifier enabled I bumped into a situation when NdisFRegisterFilterDriver() failed. In this case DriverEntry() returned an error, without releasing the spinlocks allocated in OvsInit(), which resulted in a memory leak BSOD triggered by the Driver Verifier. The patch solves this issue.

-Sorin

-----Original Message-----
From: Nithin Raju [mailto:nithin@vmware.com] 

Sent: Tuesday, 1 September, 2015 17:41
To: Sorin Vinturis
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Proper cleanup in case of driver init failure

Sorin,
Unless I’m missing something here, what is the new cleanup added in this patch? It seems more of code refactoring. Is that right?

-- Nithin

> On Sep 1, 2015, at 5:16 AM, Sorin Vinturis <svinturis@cloudbasesolutions.com> wrote:

> 

> Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>

> ---

> datapath-windows/ovsext/Driver.c | 20 ++++++++++++++------

> 1 file changed, 14 insertions(+), 6 deletions(-)

> 

> diff --git a/datapath-windows/ovsext/Driver.c b/datapath-windows/ovsext/Driver.c

> index 13fcde2..853886e 100644

> --- a/datapath-windows/ovsext/Driver.c

> +++ b/datapath-windows/ovsext/Driver.c

> @@ -139,18 +139,26 @@ DriverEntry(PDRIVER_OBJECT driverObject,

>     driverObject->DriverUnload = OvsExtUnload;

> 

>     status = NdisFRegisterFilterDriver(driverObject,

> -                                       (NDIS_HANDLE) gOvsExtDriverObject,

> -                                       &driverChars, &gOvsExtDriverHandle);

> +                                       (NDIS_HANDLE)gOvsExtDriverObject,

> +                                       &driverChars,

> +                                       &gOvsExtDriverHandle);

>     if (status != NDIS_STATUS_SUCCESS) {

> -        return status;

> +        goto cleanup;

>     }

> 

> -    /* Create the communication channel for usersapce. */

> +    /* Create the communication channel for userspace. */

>     status = OvsCreateDeviceObject(gOvsExtDriverHandle);

>     if (status != NDIS_STATUS_SUCCESS) {

> +        goto cleanup;

> +    }

> +

> +cleanup:

> +    if (status != NDIS_STATUS_SUCCESS){

>         OvsCleanup();

> -        NdisFDeregisterFilterDriver(gOvsExtDriverHandle);

> -        gOvsExtDriverHandle = NULL;

> +        if (gOvsExtDriverHandle) {

> +            NdisFDeregisterFilterDriver(gOvsExtDriverHandle);

> +            gOvsExtDriverHandle = NULL;

> +        }

>     }

> 

>     return status;

> -- 

> 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=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=UwM9RbSBfs6QT8UYqjkZr3xJUSmm5djIml87xTIB1WI&s=tQaG105CKQjdDBWZXvRzD9eqd2rsnJDji_xikpq6S7A&e=
Nithin Raju Sept. 1, 2015, 3:26 p.m. UTC | #3
> On Sep 1, 2015, at 8:06 AM, Sorin Vinturis <svinturis@cloudbasesolutions.com> wrote:
> 
> Nithin,
> 
> In my tests with Driver Verifier enabled I bumped into a situation when NdisFRegisterFilterDriver() failed. In this case DriverEntry() returned an error, without releasing the spinlocks allocated in OvsInit(), which resulted in a memory leak BSOD triggered by the Driver Verifier. The patch solves this issue.

Can we move the call to OvsInit() to after NdisFRegisterFilterDriver()?

-- Nithin
Sorin Vinturis Sept. 1, 2015, 4:09 p.m. UTC | #4
Nithin,

The documentation for the NdisFRegisterFilterDriver() function states the following:

" Drivers that call NdisFRegisterFilterDriver must be prepared for an immediate call to any of their FilterXxx functions."

So all initialization must be done before calling the above function.

-Sorin

-----Original Message-----
From: Nithin Raju [mailto:nithin@vmware.com] 
Sent: Tuesday, 1 September, 2015 18:27
To: Sorin Vinturis
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Proper cleanup in case of driver init failure

> On Sep 1, 2015, at 8:06 AM, Sorin Vinturis <svinturis@cloudbasesolutions.com> wrote:
> 
> Nithin,
> 
> In my tests with Driver Verifier enabled I bumped into a situation when NdisFRegisterFilterDriver() failed. In this case DriverEntry() returned an error, without releasing the spinlocks allocated in OvsInit(), which resulted in a memory leak BSOD triggered by the Driver Verifier. The patch solves this issue.

Can we move the call to OvsInit() to after NdisFRegisterFilterDriver()?

-- Nithin

Patch
diff mbox

diff --git a/datapath-windows/ovsext/Driver.c b/datapath-windows/ovsext/Driver.c
index 13fcde2..853886e 100644
--- a/datapath-windows/ovsext/Driver.c
+++ b/datapath-windows/ovsext/Driver.c
@@ -139,18 +139,26 @@  DriverEntry(PDRIVER_OBJECT driverObject,
     driverObject->DriverUnload = OvsExtUnload;
 
     status = NdisFRegisterFilterDriver(driverObject,
-                                       (NDIS_HANDLE) gOvsExtDriverObject,
-                                       &driverChars, &gOvsExtDriverHandle);
+                                       (NDIS_HANDLE)gOvsExtDriverObject,
+                                       &driverChars,
+                                       &gOvsExtDriverHandle);
     if (status != NDIS_STATUS_SUCCESS) {
-        return status;
+        goto cleanup;
     }
 
-    /* Create the communication channel for usersapce. */
+    /* Create the communication channel for userspace. */
     status = OvsCreateDeviceObject(gOvsExtDriverHandle);
     if (status != NDIS_STATUS_SUCCESS) {
+        goto cleanup;
+    }
+
+cleanup:
+    if (status != NDIS_STATUS_SUCCESS){
         OvsCleanup();
-        NdisFDeregisterFilterDriver(gOvsExtDriverHandle);
-        gOvsExtDriverHandle = NULL;
+        if (gOvsExtDriverHandle) {
+            NdisFDeregisterFilterDriver(gOvsExtDriverHandle);
+            gOvsExtDriverHandle = NULL;
+        }
     }
 
     return status;