| Message ID | 1441084528-8191-1-git-send-email-svinturis@cloudbasesolutions.com |
|---|---|
| State | Superseded |
| Headers | show |
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, 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=
> 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
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
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;
Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com> --- datapath-windows/ovsext/Driver.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)