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

Guru Shetty guru at ovn.org
Thu Sep 28 19:39:38 UTC 2017


On 22 September 2017 at 14:24, Shashank Ram <rams at 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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list