diff mbox series

[ovs-dev] datapath-windows: Fix bugs in cleaner threads

Message ID 20170922212451.12868-1-rams@vmware.com
State Accepted
Headers show
Series [ovs-dev] datapath-windows: Fix bugs in cleaner threads | expand

Commit Message

Shashank Ram Sept. 22, 2017, 9:24 p.m. UTC 
Conntrack, Conntrack-related, Stt, and IP fragmentation
have cleaner threads that run periodically to clean
up their respective tables. During driver unload,
OvsExtDetach() calls into routines that are meant
for explicitly cleaning these tables up and freeing
the resources associated with these threads.

If during driver unload, these cleaner threads run
immediately after the resources are freed, such as locks
used by these threads, then the cleaner threads result
in a kernel crash since they try to acquire locks
that have already been freed.

For eg, OvsIpFragmentEntryCleaner() caused a kernel
crash because it tried to acquire a lock that was
already freed by OvsCleanupIpFragment().

The fix is to simply exit the cleaner thread if the
lock associated with the thread is not initialized,
because the only way the threads can run when the lock
is invalid is when the lock has been freed up during
driver unload.

Testint done:
Verified that cleaner threads run as expected without
crashing during driver unload.

Signed-off-by: Shashank Ram <rams@vmware.com>
---
 datapath-windows/ovsext/Conntrack-related.c | 6 +++++-
 datapath-windows/ovsext/Conntrack.c         | 6 +++++-
 datapath-windows/ovsext/IpFragment.c        | 6 +++++-
 datapath-windows/ovsext/Stt.c               | 4 ++++
 4 files changed, 19 insertions(+), 3 deletions(-)

--
2.9.3.windows.2

Comments

Anand Kumar Sept. 22, 2017, 9:39 p.m. UTC | #1 
Good catch. Thanks for fixing it.

Acked-by: Anand Kumar <kumaranand@vmware.com> 

Thanks,
Anand Kumar

On 9/22/17, 2:24 PM, "ovs-dev-bounces@openvswitch.org on behalf of Shashank Ram" <ovs-dev-bounces@openvswitch.org on behalf of rams@vmware.com> wrote:

    Conntrack, Conntrack-related, Stt, and IP fragmentation
    have cleaner threads that run periodically to clean
    up their respective tables. During driver unload,
    OvsExtDetach() calls into routines that are meant
    for explicitly cleaning these tables up and freeing
    the resources associated with these threads.
    
    If during driver unload, these cleaner threads run
    immediately after the resources are freed, such as locks
    used by these threads, then the cleaner threads result
    in a kernel crash since they try to acquire locks
    that have already been freed.
    
    For eg, OvsIpFragmentEntryCleaner() caused a kernel
    crash because it tried to acquire a lock that was
    already freed by OvsCleanupIpFragment().
    
    The fix is to simply exit the cleaner thread if the
    lock associated with the thread is not initialized,
    because the only way the threads can run when the lock
    is invalid is when the lock has been freed up during
    driver unload.
    
    Testint done:
    Verified that cleaner threads run as expected without
    crashing during driver unload.
    
    Signed-off-by: Shashank Ram <rams@vmware.com>
    ---
     datapath-windows/ovsext/Conntrack-related.c | 6 +++++-
     datapath-windows/ovsext/Conntrack.c         | 6 +++++-
     datapath-windows/ovsext/IpFragment.c        | 6 +++++-
     datapath-windows/ovsext/Stt.c               | 4 ++++
     4 files changed, 19 insertions(+), 3 deletions(-)
    
    diff --git a/datapath-windows/ovsext/Conntrack-related.c b/datapath-windows/ovsext/Conntrack-related.c
    index 16ed6f7..ec4b536 100644
    --- a/datapath-windows/ovsext/Conntrack-related.c
    +++ b/datapath-windows/ovsext/Conntrack-related.c
    @@ -181,10 +181,14 @@ OvsCtRelatedEntryCleaner(PVOID data)
         POVS_CT_THREAD_CTX context = (POVS_CT_THREAD_CTX)data;
         PLIST_ENTRY link, next;
         POVS_CT_REL_ENTRY entry;
    +    LOCK_STATE_EX lockState;
         BOOLEAN success = TRUE;
    
         while (success) {
    -        LOCK_STATE_EX lockState;
    +        if (ovsCtRelatedLockObj == NULL) {
    +            /* Lock has been freed by 'OvsCleanupCtRelated()' */
    +            break;
    +        }
             NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0);
             if (context->exit) {
                 NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState);
    diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
    index f0d135b..3203411 100644
    --- a/datapath-windows/ovsext/Conntrack.c
    +++ b/datapath-windows/ovsext/Conntrack.c
    @@ -980,10 +980,14 @@ OvsConntrackEntryCleaner(PVOID data)
         POVS_CT_THREAD_CTX context = (POVS_CT_THREAD_CTX)data;
         PLIST_ENTRY link, next;
         POVS_CT_ENTRY entry;
    +    LOCK_STATE_EX lockState;
         BOOLEAN success = TRUE;
    
         while (success) {
    -        LOCK_STATE_EX lockState;
    +        if (ovsConntrackLockObj == NULL) {
    +            /* Lock has been freed by 'OvsCleanupConntrack()' */
    +            break;
    +        }
             NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
             if (context->exit) {
                 NdisReleaseRWLock(ovsConntrackLockObj, &lockState);
    diff --git a/datapath-windows/ovsext/IpFragment.c b/datapath-windows/ovsext/IpFragment.c
    index fee361a..ad48834 100644
    --- a/datapath-windows/ovsext/IpFragment.c
    +++ b/datapath-windows/ovsext/IpFragment.c
    @@ -444,10 +444,14 @@ OvsIpFragmentEntryCleaner(PVOID data)
         POVS_IPFRAG_THREAD_CTX context = (POVS_IPFRAG_THREAD_CTX)data;
         PLIST_ENTRY link, next;
         POVS_IPFRAG_ENTRY entry;
    +    LOCK_STATE_EX lockState;
         BOOLEAN success = TRUE;
    
         while (success) {
    -        LOCK_STATE_EX lockState;
    +        if (ovsIpFragmentHashLockObj == NULL) {
    +            /* Lock has been freed by 'OvsCleanupIpFragment()' */
    +            break;
    +        }
             NdisAcquireRWLockWrite(ovsIpFragmentHashLockObj, &lockState, 0);
             if (context->exit) {
                 NdisReleaseRWLock(ovsIpFragmentHashLockObj, &lockState);
    diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
    index b6236fd..f98070f 100644
    --- a/datapath-windows/ovsext/Stt.c
    +++ b/datapath-windows/ovsext/Stt.c
    @@ -551,6 +551,10 @@ OvsSttDefragCleaner(PVOID data)
         BOOLEAN success = TRUE;
    
         while (success) {
    +        if (&OvsSttSpinLock == NULL) {
    +            /* Lock has been freed by 'OvsCleanupSttDefragmentation()' */
    +            break;
    +        }
             NdisAcquireSpinLock(&OvsSttSpinLock);
             if (context->exit) {
                 NdisReleaseSpinLock(&OvsSttSpinLock);
    --
    2.9.3.windows.2
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=jdE8nm0LF-D218Vdl62i0XhoTgKlh7SqUi36OLUyxB0&s=huOsWqUkGDUGwfiXSXM1R-pN5ls0sSQklPEsn-gsr98&e=
Gurucharan Shetty Sept. 28, 2017, 7:39 p.m. UTC | #2 
On 22 September 2017 at 14:24, Shashank Ram <rams@vmware.com> wrote:

> Conntrack, Conntrack-related, Stt, and IP fragmentation
> have cleaner threads that run periodically to clean
> up their respective tables. During driver unload,
> OvsExtDetach() calls into routines that are meant
> for explicitly cleaning these tables up and freeing
> the resources associated with these threads.
>
> If during driver unload, these cleaner threads run
> immediately after the resources are freed, such as locks
> used by these threads, then the cleaner threads result
> in a kernel crash since they try to acquire locks
> that have already been freed.
>
> For eg, OvsIpFragmentEntryCleaner() caused a kernel
> crash because it tried to acquire a lock that was
> already freed by OvsCleanupIpFragment().
>
> The fix is to simply exit the cleaner thread if the
> lock associated with the thread is not initialized,
> because the only way the threads can run when the lock
> is invalid is when the lock has been freed up during
> driver unload.
>
> Testint done:
> Verified that cleaner threads run as expected without
> crashing during driver unload.
>
> Signed-off-by: Shashank Ram <rams@vmware.com>
>
Applied to master and 2.8. Thanks!


> ---
>  datapath-windows/ovsext/Conntrack-related.c | 6 +++++-
>  datapath-windows/ovsext/Conntrack.c         | 6 +++++-
>  datapath-windows/ovsext/IpFragment.c        | 6 +++++-
>  datapath-windows/ovsext/Stt.c               | 4 ++++
>  4 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Conntrack-related.c
> b/datapath-windows/ovsext/Conntrack-related.c
> index 16ed6f7..ec4b536 100644
> --- a/datapath-windows/ovsext/Conntrack-related.c
> +++ b/datapath-windows/ovsext/Conntrack-related.c
> @@ -181,10 +181,14 @@ OvsCtRelatedEntryCleaner(PVOID data)
>      POVS_CT_THREAD_CTX context = (POVS_CT_THREAD_CTX)data;
>      PLIST_ENTRY link, next;
>      POVS_CT_REL_ENTRY entry;
> +    LOCK_STATE_EX lockState;
>      BOOLEAN success = TRUE;
>
>      while (success) {
> -        LOCK_STATE_EX lockState;
> +        if (ovsCtRelatedLockObj == NULL) {
> +            /* Lock has been freed by 'OvsCleanupCtRelated()' */
> +            break;
> +        }
>          NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0);
>          if (context->exit) {
>              NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState);
> diff --git a/datapath-windows/ovsext/Conntrack.c
> b/datapath-windows/ovsext/Conntrack.c
> index f0d135b..3203411 100644
> --- a/datapath-windows/ovsext/Conntrack.c
> +++ b/datapath-windows/ovsext/Conntrack.c
> @@ -980,10 +980,14 @@ OvsConntrackEntryCleaner(PVOID data)
>      POVS_CT_THREAD_CTX context = (POVS_CT_THREAD_CTX)data;
>      PLIST_ENTRY link, next;
>      POVS_CT_ENTRY entry;
> +    LOCK_STATE_EX lockState;
>      BOOLEAN success = TRUE;
>
>      while (success) {
> -        LOCK_STATE_EX lockState;
> +        if (ovsConntrackLockObj == NULL) {
> +            /* Lock has been freed by 'OvsCleanupConntrack()' */
> +            break;
> +        }
>          NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
>          if (context->exit) {
>              NdisReleaseRWLock(ovsConntrackLockObj, &lockState);
> diff --git a/datapath-windows/ovsext/IpFragment.c
> b/datapath-windows/ovsext/IpFragment.c
> index fee361a..ad48834 100644
> --- a/datapath-windows/ovsext/IpFragment.c
> +++ b/datapath-windows/ovsext/IpFragment.c
> @@ -444,10 +444,14 @@ OvsIpFragmentEntryCleaner(PVOID data)
>      POVS_IPFRAG_THREAD_CTX context = (POVS_IPFRAG_THREAD_CTX)data;
>      PLIST_ENTRY link, next;
>      POVS_IPFRAG_ENTRY entry;
> +    LOCK_STATE_EX lockState;
>      BOOLEAN success = TRUE;
>
>      while (success) {
> -        LOCK_STATE_EX lockState;
> +        if (ovsIpFragmentHashLockObj == NULL) {
> +            /* Lock has been freed by 'OvsCleanupIpFragment()' */
> +            break;
> +        }
>          NdisAcquireRWLockWrite(ovsIpFragmentHashLockObj, &lockState, 0);
>          if (context->exit) {
>              NdisReleaseRWLock(ovsIpFragmentHashLockObj, &lockState);
> diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
> index b6236fd..f98070f 100644
> --- a/datapath-windows/ovsext/Stt.c
> +++ b/datapath-windows/ovsext/Stt.c
> @@ -551,6 +551,10 @@ OvsSttDefragCleaner(PVOID data)
>      BOOLEAN success = TRUE;
>
>      while (success) {
> +        if (&OvsSttSpinLock == NULL) {
> +            /* Lock has been freed by 'OvsCleanupSttDefragmentation()' */
> +            break;
> +        }
>          NdisAcquireSpinLock(&OvsSttSpinLock);
>          if (context->exit) {
>              NdisReleaseSpinLock(&OvsSttSpinLock);
> --
> 2.9.3.windows.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Conntrack-related.c b/datapath-windows/ovsext/Conntrack-related.c
index 16ed6f7..ec4b536 100644
--- a/datapath-windows/ovsext/Conntrack-related.c
+++ b/datapath-windows/ovsext/Conntrack-related.c
@@ -181,10 +181,14 @@  OvsCtRelatedEntryCleaner(PVOID data)
     POVS_CT_THREAD_CTX context = (POVS_CT_THREAD_CTX)data;
     PLIST_ENTRY link, next;
     POVS_CT_REL_ENTRY entry;
+    LOCK_STATE_EX lockState;
     BOOLEAN success = TRUE;

     while (success) {
-        LOCK_STATE_EX lockState;
+        if (ovsCtRelatedLockObj == NULL) {
+            /* Lock has been freed by 'OvsCleanupCtRelated()' */
+            break;
+        }
         NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0);
         if (context->exit) {
             NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState);
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index f0d135b..3203411 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -980,10 +980,14 @@  OvsConntrackEntryCleaner(PVOID data)
     POVS_CT_THREAD_CTX context = (POVS_CT_THREAD_CTX)data;
     PLIST_ENTRY link, next;
     POVS_CT_ENTRY entry;
+    LOCK_STATE_EX lockState;
     BOOLEAN success = TRUE;

     while (success) {
-        LOCK_STATE_EX lockState;
+        if (ovsConntrackLockObj == NULL) {
+            /* Lock has been freed by 'OvsCleanupConntrack()' */
+            break;
+        }
         NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
         if (context->exit) {
             NdisReleaseRWLock(ovsConntrackLockObj, &lockState);
diff --git a/datapath-windows/ovsext/IpFragment.c b/datapath-windows/ovsext/IpFragment.c
index fee361a..ad48834 100644
--- a/datapath-windows/ovsext/IpFragment.c
+++ b/datapath-windows/ovsext/IpFragment.c
@@ -444,10 +444,14 @@  OvsIpFragmentEntryCleaner(PVOID data)
     POVS_IPFRAG_THREAD_CTX context = (POVS_IPFRAG_THREAD_CTX)data;
     PLIST_ENTRY link, next;
     POVS_IPFRAG_ENTRY entry;
+    LOCK_STATE_EX lockState;
     BOOLEAN success = TRUE;

     while (success) {
-        LOCK_STATE_EX lockState;
+        if (ovsIpFragmentHashLockObj == NULL) {
+            /* Lock has been freed by 'OvsCleanupIpFragment()' */
+            break;
+        }
         NdisAcquireRWLockWrite(ovsIpFragmentHashLockObj, &lockState, 0);
         if (context->exit) {
             NdisReleaseRWLock(ovsIpFragmentHashLockObj, &lockState);
diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
index b6236fd..f98070f 100644
--- a/datapath-windows/ovsext/Stt.c
+++ b/datapath-windows/ovsext/Stt.c
@@ -551,6 +551,10 @@  OvsSttDefragCleaner(PVOID data)
     BOOLEAN success = TRUE;

     while (success) {
+        if (&OvsSttSpinLock == NULL) {
+            /* Lock has been freed by 'OvsCleanupSttDefragmentation()' */
+            break;
+        }
         NdisAcquireSpinLock(&OvsSttSpinLock);
         if (context->exit) {
             NdisReleaseSpinLock(&OvsSttSpinLock);