[libvirt] [PATCH v3 0/4] Fix deadlock in nwfilter code
Since we introduced fine grained locking into the QEMU driver so that VM start can run in parallel, we appear to have caused a race with the nwfilter code. In particular since we no longer hold the global QEMU driver lock for the duration of VM startup we have a lock ordering flaw. This results in deadlock when nwfilter operations happen in parallel with VM startup. This also affects the LXC driver. This patch series attempts to address the problem https://bugzilla.redhat.com/show_bug.cgi?id=929412 the removal of the windows thread impl isn't strictly required, I just didn't want to waste time creating a read/write lock impl for Windows threads. See also this callgraph http://berrange.fedorapeople.org/nwfilter.ps In v3: - Removed addition of extra lock for serializing iptables since it was overkill - Added patch to make libvirt compile with mingw32-pthreads from Fedora 19 and earlier - Fix errno handling with rwlock creation In v2: - Re-ordered patches and squashed two together - Add missing locking in state reload function - Don't remove locking from virNWFilterInstantiateFilterLate *** BLURB HERE *** Daniel P. Berrange (4): Add a read/write lock implementation Fix pthread_sigmask check for mingw32 without winpthreads Remove windows thread implementation in favour of pthreads Push nwfilter update locking up to top level configure.ac | 21 +- src/Makefile.am | 4 - src/conf/nwfilter_conf.c | 23 +- src/conf/nwfilter_conf.h | 3 +- src/libvirt_private.syms | 8 +- src/lxc/lxc_driver.c | 6 + src/nwfilter/nwfilter_driver.c | 10 +- src/nwfilter/nwfilter_gentech_driver.c | 6 +- src/qemu/qemu_driver.c | 6 + src/uml/uml_driver.c | 4 + src/util/virthread.c | 293 +++++++++++++++++++++++- src/util/virthread.h | 51 ++++- src/util/virthreadpthread.c | 278 ----------------------- src/util/virthreadpthread.h | 49 ---- src/util/virthreadwin32.c | 396 --------------------------------- src/util/virthreadwin32.h | 53 ----- 16 files changed, 388 insertions(+), 823 deletions(-) delete mode 100644 src/util/virthreadpthread.c delete mode 100644 src/util/virthreadpthread.h delete mode 100644 src/util/virthreadwin32.c delete mode 100644 src/util/virthreadwin32.h -- 1.8.4.2
Add virRWLock backed up by a POSIX rwlock primitive Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 5 +++++ src/util/virthread.h | 10 ++++++++++ src/util/virthreadpthread.c | 33 +++++++++++++++++++++++++++++++++ src/util/virthreadpthread.h | 4 ++++ src/util/virthreadwin32.c | 19 +++++++++++++++++++ src/util/virthreadwin32.h | 4 ++++ 6 files changed, 75 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..ad9ad28 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1818,6 +1818,11 @@ virMutexInitRecursive; virMutexLock; virMutexUnlock; virOnce; +virRWLockDestroy; +virRWLockInit; +virRWLockRead; +virRWLockUnlock; +virRWLockWrite; virThreadCancel; virThreadCreate; virThreadID; diff --git a/src/util/virthread.h b/src/util/virthread.h index 84d3bdc..7015d60 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -28,6 +28,9 @@ typedef struct virMutex virMutex; typedef virMutex *virMutexPtr; +typedef struct virRWLock virRWLock; +typedef virRWLock *virRWLockPtr; + typedef struct virCond virCond; typedef virCond *virCondPtr; @@ -89,6 +92,13 @@ void virMutexLock(virMutexPtr m); void virMutexUnlock(virMutexPtr m); +int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; +void virRWLockDestroy(virRWLockPtr m); + +void virRWLockRead(virRWLockPtr m); +void virRWLockWrite(virRWLockPtr m); +void virRWLockUnlock(virRWLockPtr m); + int virCondInit(virCondPtr c) ATTRIBUTE_RETURN_CHECK; int virCondDestroy(virCondPtr c); diff --git a/src/util/virthreadpthread.c b/src/util/virthreadpthread.c index ca841e4..38b6454 100644 --- a/src/util/virthreadpthread.c +++ b/src/util/virthreadpthread.c @@ -91,6 +91,39 @@ void virMutexUnlock(virMutexPtr m) } +int virRWLockInit(virRWLockPtr m) +{ + int ret; + ret = pthread_rwlock_init(&m->lock, NULL); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock); +} + + +void virRWLockRead(virRWLockPtr m) +{ + pthread_rwlock_rdlock(&m->lock); +} + +void virRWLockWrite(virRWLockPtr m) +{ + pthread_rwlock_wrlock(&m->lock); +} + + +void virRWLockUnlock(virRWLockPtr m) +{ + pthread_rwlock_unlock(&m->lock); +} + int virCondInit(virCondPtr c) { int ret; diff --git a/src/util/virthreadpthread.h b/src/util/virthreadpthread.h index b9f1319..cb607d0 100644 --- a/src/util/virthreadpthread.h +++ b/src/util/virthreadpthread.h @@ -27,6 +27,10 @@ struct virMutex { pthread_mutex_t lock; }; +struct virRWLock { + pthread_rwlock_t lock; +}; + struct virCond { pthread_cond_t cond; }; diff --git a/src/util/virthreadwin32.c b/src/util/virthreadwin32.c index 5d6277f..a9e2353 100644 --- a/src/util/virthreadwin32.c +++ b/src/util/virthreadwin32.c @@ -123,6 +123,25 @@ void virMutexUnlock(virMutexPtr m) } +int virRWLockInit(virRWLockPtr m ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + return -1; +} + +void virRWLockDestroy(virRWLockPtr m ATTRIBUTE_UNUSED) +{} + + +void virRWLockRead(virRWLockPtr m ATTRIBUTE_UNUSED) +{} + +void virRWLockWrite(virRWLockPtr m ATTRIBUTE_UNUSED) +{} + + +void virRWLockUnlock(virRWLockPtr m ATTRIBUTE_UNUSED) +{} int virCondInit(virCondPtr c) { diff --git a/src/util/virthreadwin32.h b/src/util/virthreadwin32.h index 645031b..31d9444 100644 --- a/src/util/virthreadwin32.h +++ b/src/util/virthreadwin32.h @@ -30,6 +30,10 @@ struct virMutex { HANDLE lock; }; +struct virRWLock { + bool ignored; +}; + struct virCond { virMutex lock; size_t nwaiters; -- 1.8.4.2
On Fedora 19 and older the pthreads impl provided with mingw does not have any pthread_sigmask impl at all. The configure.ac check was not distinguishing this scenario from that of a broken pthread_sigmask impl, so was mistakenly enabling the libvirt workaround even when it was not needed. This in turn conflicted with the gnulib provided pthread_sigmask impl. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- configure.ac | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 3a70375..20d6202 100644 --- a/configure.ac +++ b/configure.ac @@ -285,8 +285,10 @@ AC_CACHE_CHECK([whether pthread_sigmask does anything], #include <sys/types.h> #include <signal.h> ]], [[ - int (*foo)(int, const sigset_t *, sigset_t *) = &pthread_sigmask; - return !foo; + #ifdef pthread_sigmask + int (*foo)(int, const sigset_t *, sigset_t *) = &pthread_sigmask; + return !foo; + #endif ]])], [lv_cv_pthread_sigmask_works=yes], [lv_cv_pthread_sigmask_works=no])]) if test "x$lv_cv_pthread_sigmask_works" != xyes; then AC_DEFINE([FUNC_PTHREAD_SIGMASK_BROKEN], [1], -- 1.8.4.2
On 01/29/2014 07:53 AM, Daniel P. Berrange wrote:
On Fedora 19 and older the pthreads impl provided with mingw does not have any pthread_sigmask impl at all. The configure.ac check was not distinguishing this scenario from that of a broken pthread_sigmask impl, so was mistakenly enabling the libvirt workaround even when it was not needed. This in turn conflicted with the gnulib provided pthread_sigmask impl.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- configure.ac | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
There are a number of pthreads impls available on Win32 these days, in particular the mingw64 project has a good impl. Delete the native windows thread implementation and rely on using pthreads everywhere. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- configure.ac | 15 +- src/Makefile.am | 4 - src/util/virthread.c | 293 ++++++++++++++++++++++++++++++- src/util/virthread.h | 41 ++++- src/util/virthreadpthread.c | 311 --------------------------------- src/util/virthreadpthread.h | 53 ------ src/util/virthreadwin32.c | 415 -------------------------------------------- src/util/virthreadwin32.h | 57 ------ 8 files changed, 331 insertions(+), 858 deletions(-) delete mode 100644 src/util/virthreadpthread.c delete mode 100644 src/util/virthreadpthread.h delete mode 100644 src/util/virthreadwin32.c delete mode 100644 src/util/virthreadwin32.h diff --git a/configure.ac b/configure.ac index 20d6202..3ff30fa 100644 --- a/configure.ac +++ b/configure.ac @@ -270,12 +270,21 @@ AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \ posix_memalign prlimit regexec sched_getaffinity setgroups setns \ setrlimit symlink sysctlbyname]) -dnl Availability of pthread functions (if missing, win32 threading is -dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. -dnl LIB_PTHREAD and LIBMULTITHREAD were set during gl_INIT by gnulib. +dnl Availability of pthread functions. Because of $LIB_PTHREAD, we +dnl cannot use AC_CHECK_FUNCS_ONCE. LIB_PTHREAD and LIBMULTITHREAD +dnl were set during gl_INIT by gnulib. old_LIBS=$LIBS LIBS="$LIBS $LIB_PTHREAD $LIBMULTITHREAD" + +pthread_found=yes AC_CHECK_FUNCS([pthread_mutexattr_init]) +AC_CHECK_HEADER([pthread.h],,[pthread_found=no]) + +if test "$ac_cv_func_pthread_mutexattr_init:$pthread_found" != "yes:yes" +then + AC_MSG_ERROR([A pthreads impl is required for building libvirt]) +fi + dnl At least mingw64-winpthreads #defines pthread_sigmask to 0, dnl which in turn causes compilation to complain about unused variables. dnl Expose this broken implementation, so we can work around it. diff --git a/src/Makefile.am b/src/Makefile.am index c1a58dd..ac2d1d9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -141,8 +141,6 @@ UTIL_SOURCES = \ util/virsysinfo.c util/virsysinfo.h \ util/virsystemd.c util/virsystemd.h \ util/virthread.c util/virthread.h \ - util/virthreadpthread.h \ - util/virthreadwin32.h \ util/virthreadpool.c util/virthreadpool.h \ util/virtime.h util/virtime.c \ util/virtpm.h util/virtpm.c \ @@ -165,8 +163,6 @@ util/virkeymaps.h: $(srcdir)/util/keymaps.csv \ $(AM_V_GEN)$(PYTHON) $(srcdir)/util/virkeycode-mapgen.py \ <$(srcdir)/util/keymaps.csv >$(srcdir)/util/virkeymaps.h -EXTRA_DIST += util/virthreadpthread.c util/virthreadwin32.c - # Internal generic driver infrastructure NODE_INFO_SOURCES = nodeinfo.h nodeinfo.c nodeinfopriv.h DATATYPES_SOURCES = datatypes.h datatypes.c diff --git a/src/util/virthread.c b/src/util/virthread.c index dd1768e..d3dfd16 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -23,12 +23,291 @@ #include "virthread.h" -/* On mingw, we prefer native threading over the sometimes-broken - * pthreads-win32 library wrapper. */ -#ifdef WIN32 -# include "virthreadwin32.c" -#elif defined HAVE_PTHREAD_MUTEXATTR_INIT -# include "virthreadpthread.c" +#include <unistd.h> +#include <inttypes.h> +#if HAVE_SYS_SYSCALL_H +# include <sys/syscall.h> +#endif + +#include "viralloc.h" + + +/* Nothing special required for pthreads */ +int virThreadInitialize(void) +{ + return 0; +} + +void virThreadOnExit(void) +{ +} + +int virOnce(virOnceControlPtr once, virOnceFunc init) +{ + return pthread_once(&once->once, init); +} + + +int virMutexInit(virMutexPtr m) +{ + int ret; + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); + ret = pthread_mutex_init(&m->lock, &attr); + pthread_mutexattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virMutexInitRecursive(virMutexPtr m) +{ + int ret; + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + ret = pthread_mutex_init(&m->lock, &attr); + pthread_mutexattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virMutexDestroy(virMutexPtr m) +{ + pthread_mutex_destroy(&m->lock); +} + +void virMutexLock(virMutexPtr m){ + pthread_mutex_lock(&m->lock); +} + +void virMutexUnlock(virMutexPtr m) +{ + pthread_mutex_unlock(&m->lock); +} + + +int virRWLockInit(virRWLockPtr m) +{ + int ret; + ret = pthread_rwlock_init(&m->lock, NULL); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock); +} + + +void virRWLockRead(virRWLockPtr m) +{ + pthread_rwlock_rdlock(&m->lock); +} + +void virRWLockWrite(virRWLockPtr m) +{ + pthread_rwlock_wrlock(&m->lock); +} + + +void virRWLockUnlock(virRWLockPtr m) +{ + pthread_rwlock_unlock(&m->lock); +} + +int virCondInit(virCondPtr c) +{ + int ret; + if ((ret = pthread_cond_init(&c->cond, NULL)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondDestroy(virCondPtr c) +{ + int ret; + if ((ret = pthread_cond_destroy(&c->cond)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondWait(virCondPtr c, virMutexPtr m) +{ + int ret; + if ((ret = pthread_cond_wait(&c->cond, &m->lock)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) +{ + int ret; + struct timespec ts; + + ts.tv_sec = whenms / 1000; + ts.tv_nsec = (whenms % 1000) * 1000; + + if ((ret = pthread_cond_timedwait(&c->cond, &m->lock, &ts)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virCondSignal(virCondPtr c) +{ + pthread_cond_signal(&c->cond); +} + +void virCondBroadcast(virCondPtr c) +{ + pthread_cond_broadcast(&c->cond); +} + +struct virThreadArgs { + virThreadFunc func; + void *opaque; +}; + +static void *virThreadHelper(void *data) +{ + struct virThreadArgs *args = data; + struct virThreadArgs local = *args; + + /* Free args early, rather than tying it up during the entire thread. */ + VIR_FREE(args); + local.func(local.opaque); + return NULL; +} + +int virThreadCreate(virThreadPtr thread, + bool joinable, + virThreadFunc func, + void *opaque) +{ + struct virThreadArgs *args; + pthread_attr_t attr; + int ret = -1; + int err; + + if ((err = pthread_attr_init(&attr)) != 0) + goto cleanup; + if (VIR_ALLOC_QUIET(args) < 0) { + err = ENOMEM; + goto cleanup; + } + + args->func = func; + args->opaque = opaque; + + if (!joinable) + pthread_attr_setdetachstate(&attr, 1); + + err = pthread_create(&thread->thread, &attr, virThreadHelper, args); + if (err != 0) { + VIR_FREE(args); + goto cleanup; + } + /* New thread owns 'args' in success case, so don't free */ + + ret = 0; +cleanup: + pthread_attr_destroy(&attr); + if (ret < 0) + errno = err; + return ret; +} + +void virThreadSelf(virThreadPtr thread) +{ + thread->thread = pthread_self(); +} + +bool virThreadIsSelf(virThreadPtr thread) +{ + return pthread_equal(pthread_self(), thread->thread) ? true : false; +} + +/* For debugging use only; this result is not guaranteed unique if + * pthread_t is larger than a 64-bit pointer, nor does it always match + * the pthread_self() id on Linux. */ +unsigned long long virThreadSelfID(void) +{ +#if defined(HAVE_SYS_SYSCALL_H) && defined(SYS_gettid) + pid_t tid = syscall(SYS_gettid); + return tid; #else -# error "Either pthreads or Win32 threads are required" + union { + unsigned long long l; + pthread_t t; + } u; + u.t = pthread_self(); + return u.l; #endif +} + +/* For debugging use only; this result is not guaranteed unique if + * pthread_t is larger than a 64-bit pointer, nor does it always match + * the thread id of virThreadSelfID on Linux. */ +unsigned long long virThreadID(virThreadPtr thread) +{ + union { + unsigned long long l; + pthread_t t; + } u; + u.t = thread->thread; + return u.l; +} + +void virThreadJoin(virThreadPtr thread) +{ + pthread_join(thread->thread, NULL); +} + +void virThreadCancel(virThreadPtr thread) +{ + pthread_cancel(thread->thread); +} + +int virThreadLocalInit(virThreadLocalPtr l, + virThreadLocalCleanup c) +{ + int ret; + if ((ret = pthread_key_create(&l->key, c)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +void *virThreadLocalGet(virThreadLocalPtr l) +{ + return pthread_getspecific(l->key); +} + +int virThreadLocalSet(virThreadLocalPtr l, void *val) +{ + int err = pthread_setspecific(l->key, val); + if (err) { + errno = err; + return -1; + } + return 0; +} diff --git a/src/util/virthread.h b/src/util/virthread.h index 7015d60..eba8dc3 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -25,24 +25,57 @@ # include "internal.h" # include "virerror.h" +# include <pthread.h> + typedef struct virMutex virMutex; typedef virMutex *virMutexPtr; +struct virMutex { + pthread_mutex_t lock; +}; + typedef struct virRWLock virRWLock; typedef virRWLock *virRWLockPtr; +struct virRWLock { + pthread_rwlock_t lock; +}; + + typedef struct virCond virCond; typedef virCond *virCondPtr; +struct virCond { + pthread_cond_t cond; +}; + typedef struct virThreadLocal virThreadLocal; typedef virThreadLocal *virThreadLocalPtr; +struct virThreadLocal { + pthread_key_t key; +}; + typedef struct virThread virThread; typedef virThread *virThreadPtr; +struct virThread { + pthread_t thread; +}; + typedef struct virOnceControl virOnceControl; typedef virOnceControl *virOnceControlPtr; +struct virOnceControl { + pthread_once_t once; +}; + + +# define VIR_ONCE_CONTROL_INITIALIZER \ + { \ + .once = PTHREAD_ONCE_INIT \ + } + typedef void (*virOnceFunc)(void); int virThreadInitialize(void) ATTRIBUTE_RETURN_CHECK; @@ -121,14 +154,6 @@ int virThreadLocalInit(virThreadLocalPtr l, void *virThreadLocalGet(virThreadLocalPtr l); int virThreadLocalSet(virThreadLocalPtr l, void*) ATTRIBUTE_RETURN_CHECK; -# ifdef WIN32 -# include "virthreadwin32.h" -# elif defined HAVE_PTHREAD_MUTEXATTR_INIT -# include "virthreadpthread.h" -# else -# error "Either pthreads or Win32 threads are required" -# endif - /** * VIR_ONCE_GLOBAL_INIT: diff --git a/src/util/virthreadpthread.c b/src/util/virthreadpthread.c deleted file mode 100644 index 38b6454..0000000 --- a/src/util/virthreadpthread.c +++ /dev/null @@ -1,311 +0,0 @@ -/* - * virthreadpthread.c: basic thread synchronization primitives - * - * Copyright (C) 2009-2011, 2013 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> - -#include <unistd.h> -#include <inttypes.h> -#if HAVE_SYS_SYSCALL_H -# include <sys/syscall.h> -#endif - -#include "viralloc.h" - - -/* Nothing special required for pthreads */ -int virThreadInitialize(void) -{ - return 0; -} - -void virThreadOnExit(void) -{ -} - -int virOnce(virOnceControlPtr once, virOnceFunc init) -{ - return pthread_once(&once->once, init); -} - - -int virMutexInit(virMutexPtr m) -{ - int ret; - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); - ret = pthread_mutex_init(&m->lock, &attr); - pthread_mutexattr_destroy(&attr); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virMutexInitRecursive(virMutexPtr m) -{ - int ret; - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - ret = pthread_mutex_init(&m->lock, &attr); - pthread_mutexattr_destroy(&attr); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virMutexDestroy(virMutexPtr m) -{ - pthread_mutex_destroy(&m->lock); -} - -void virMutexLock(virMutexPtr m){ - pthread_mutex_lock(&m->lock); -} - -void virMutexUnlock(virMutexPtr m) -{ - pthread_mutex_unlock(&m->lock); -} - - -int virRWLockInit(virRWLockPtr m) -{ - int ret; - ret = pthread_rwlock_init(&m->lock, NULL); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virRWLockDestroy(virRWLockPtr m) -{ - pthread_rwlock_destroy(&m->lock); -} - - -void virRWLockRead(virRWLockPtr m) -{ - pthread_rwlock_rdlock(&m->lock); -} - -void virRWLockWrite(virRWLockPtr m) -{ - pthread_rwlock_wrlock(&m->lock); -} - - -void virRWLockUnlock(virRWLockPtr m) -{ - pthread_rwlock_unlock(&m->lock); -} - -int virCondInit(virCondPtr c) -{ - int ret; - if ((ret = pthread_cond_init(&c->cond, NULL)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondDestroy(virCondPtr c) -{ - int ret; - if ((ret = pthread_cond_destroy(&c->cond)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondWait(virCondPtr c, virMutexPtr m) -{ - int ret; - if ((ret = pthread_cond_wait(&c->cond, &m->lock)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) -{ - int ret; - struct timespec ts; - - ts.tv_sec = whenms / 1000; - ts.tv_nsec = (whenms % 1000) * 1000; - - if ((ret = pthread_cond_timedwait(&c->cond, &m->lock, &ts)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virCondSignal(virCondPtr c) -{ - pthread_cond_signal(&c->cond); -} - -void virCondBroadcast(virCondPtr c) -{ - pthread_cond_broadcast(&c->cond); -} - -struct virThreadArgs { - virThreadFunc func; - void *opaque; -}; - -static void *virThreadHelper(void *data) -{ - struct virThreadArgs *args = data; - struct virThreadArgs local = *args; - - /* Free args early, rather than tying it up during the entire thread. */ - VIR_FREE(args); - local.func(local.opaque); - return NULL; -} - -int virThreadCreate(virThreadPtr thread, - bool joinable, - virThreadFunc func, - void *opaque) -{ - struct virThreadArgs *args; - pthread_attr_t attr; - int ret = -1; - int err; - - if ((err = pthread_attr_init(&attr)) != 0) - goto cleanup; - if (VIR_ALLOC_QUIET(args) < 0) { - err = ENOMEM; - goto cleanup; - } - - args->func = func; - args->opaque = opaque; - - if (!joinable) - pthread_attr_setdetachstate(&attr, 1); - - err = pthread_create(&thread->thread, &attr, virThreadHelper, args); - if (err != 0) { - VIR_FREE(args); - goto cleanup; - } - /* New thread owns 'args' in success case, so don't free */ - - ret = 0; -cleanup: - pthread_attr_destroy(&attr); - if (ret < 0) - errno = err; - return ret; -} - -void virThreadSelf(virThreadPtr thread) -{ - thread->thread = pthread_self(); -} - -bool virThreadIsSelf(virThreadPtr thread) -{ - return pthread_equal(pthread_self(), thread->thread) ? true : false; -} - -/* For debugging use only; this result is not guaranteed unique if - * pthread_t is larger than a 64-bit pointer, nor does it always match - * the pthread_self() id on Linux. */ -unsigned long long virThreadSelfID(void) -{ -#if defined(HAVE_SYS_SYSCALL_H) && defined(SYS_gettid) - pid_t tid = syscall(SYS_gettid); - return tid; -#else - union { - unsigned long long l; - pthread_t t; - } u; - u.t = pthread_self(); - return u.l; -#endif -} - -/* For debugging use only; this result is not guaranteed unique if - * pthread_t is larger than a 64-bit pointer, nor does it always match - * the thread id of virThreadSelfID on Linux. */ -unsigned long long virThreadID(virThreadPtr thread) -{ - union { - unsigned long long l; - pthread_t t; - } u; - u.t = thread->thread; - return u.l; -} - -void virThreadJoin(virThreadPtr thread) -{ - pthread_join(thread->thread, NULL); -} - -void virThreadCancel(virThreadPtr thread) -{ - pthread_cancel(thread->thread); -} - -int virThreadLocalInit(virThreadLocalPtr l, - virThreadLocalCleanup c) -{ - int ret; - if ((ret = pthread_key_create(&l->key, c)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -void *virThreadLocalGet(virThreadLocalPtr l) -{ - return pthread_getspecific(l->key); -} - -int virThreadLocalSet(virThreadLocalPtr l, void *val) -{ - int err = pthread_setspecific(l->key, val); - if (err) { - errno = err; - return -1; - } - return 0; -} diff --git a/src/util/virthreadpthread.h b/src/util/virthreadpthread.h deleted file mode 100644 index cb607d0..0000000 --- a/src/util/virthreadpthread.h +++ /dev/null @@ -1,53 +0,0 @@ -/* - * virthreadpthread.c: basic thread synchronization primitives - * - * Copyright (C) 2009, 2011 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include "internal.h" - -#include <pthread.h> - -struct virMutex { - pthread_mutex_t lock; -}; - -struct virRWLock { - pthread_rwlock_t lock; -}; - -struct virCond { - pthread_cond_t cond; -}; - -struct virThread { - pthread_t thread; -}; - -struct virThreadLocal { - pthread_key_t key; -}; - -struct virOnceControl { - pthread_once_t once; -}; - -#define VIR_ONCE_CONTROL_INITIALIZER \ -{ \ - .once = PTHREAD_ONCE_INIT \ -} diff --git a/src/util/virthreadwin32.c b/src/util/virthreadwin32.c deleted file mode 100644 index a9e2353..0000000 --- a/src/util/virthreadwin32.c +++ /dev/null @@ -1,415 +0,0 @@ -/* - * virthreadwin32.c: basic thread synchronization primitives - * - * Copyright (C) 2009-2011, 2013 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> - -#include <process.h> - -#include "viralloc.h" - -#define VIR_FROM_THIS VIR_FROM_NONE - -struct virThreadLocalData { - DWORD key; - virThreadLocalCleanup cleanup; -}; -typedef struct virThreadLocalData virThreadLocalData; -typedef virThreadLocalData *virThreadLocalDataPtr; - -virMutex virThreadLocalLock; -size_t virThreadLocalCount = 0; -virThreadLocalDataPtr virThreadLocalList = NULL; -DWORD selfkey; - -virThreadLocal virCondEvent; - -void virCondEventCleanup(void *data); - -int virThreadInitialize(void) -{ - if (virMutexInit(&virThreadLocalLock) < 0) - return -1; - if (virThreadLocalInit(&virCondEvent, virCondEventCleanup) < 0) - return -1; - if ((selfkey = TlsAlloc()) == TLS_OUT_OF_INDEXES) - return -1; - return 0; -} - -void virThreadOnExit(void) -{ - size_t i; - virMutexLock(&virThreadLocalLock); - for (i = 0; i < virThreadLocalCount; i++) { - if (virThreadLocalList[i].cleanup) { - void *data = TlsGetValue(virThreadLocalList[i].key); - if (data) { - TlsSetValue(virThreadLocalList[i].key, NULL); - - (virThreadLocalList[i].cleanup)(data); - } - } - } - virMutexUnlock(&virThreadLocalLock); -} - -int virOnce(virOnceControlPtr once, virOnceFunc func) -{ - if (!once->complete) { - if (InterlockedIncrement(&once->init) == 1) { - /* We're the first thread. */ - func(); - once->complete = 1; - } else { - /* We're a later thread. Decrement the init counter back - * to avoid overflow, then yield until the first thread - * marks that the function is complete. It is rare that - * multiple threads will be waiting here, and since each - * thread is yielding except the first, we should get out - * soon enough. */ - InterlockedDecrement(&once->init); - while (!once->complete) - Sleep(0); - } - } - return 0; -} - -int virMutexInit(virMutexPtr m) -{ - return virMutexInitRecursive(m); -} - -int virMutexInitRecursive(virMutexPtr m) -{ - if (!(m->lock = CreateMutex(NULL, FALSE, NULL))) { - errno = ESRCH; - return -1; - } - return 0; -} - -void virMutexDestroy(virMutexPtr m) -{ - CloseHandle(m->lock); -} - -void virMutexLock(virMutexPtr m) -{ - WaitForSingleObject(m->lock, INFINITE); -} - -void virMutexUnlock(virMutexPtr m) -{ - ReleaseMutex(m->lock); -} - - -int virRWLockInit(virRWLockPtr m ATTRIBUTE_UNUSED) -{ - errno = ENOSYS; - return -1; -} - -void virRWLockDestroy(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - - -void virRWLockRead(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - -void virRWLockWrite(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - - -void virRWLockUnlock(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - -int virCondInit(virCondPtr c) -{ - c->waiters = NULL; - if (virMutexInit(&c->lock) < 0) - return -1; - return 0; -} - -int virCondDestroy(virCondPtr c) -{ - if (c->waiters) { - errno = EINVAL; - return -1; - } - virMutexDestroy(&c->lock); - return 0; -} - -void virCondEventCleanup(void *data) -{ - HANDLE event = data; - CloseHandle(event); -} - -int virCondWait(virCondPtr c, virMutexPtr m) -{ - HANDLE event = virThreadLocalGet(&virCondEvent); - - if (!event) { - event = CreateEvent(0, FALSE, FALSE, NULL); - if (!event) { - return -1; - } - if (virThreadLocalSet(&virCondEvent, event) < 0) { - CloseHandle(event); - return -1; - } - } - - virMutexLock(&c->lock); - - if (VIR_REALLOC_N(c->waiters, c->nwaiters + 1) < 0) { - virMutexUnlock(&c->lock); - return -1; - } - c->waiters[c->nwaiters] = event; - c->nwaiters++; - - virMutexUnlock(&c->lock); - - virMutexUnlock(m); - - if (WaitForSingleObject(event, INFINITE) == WAIT_FAILED) { - virMutexLock(m); - errno = EINVAL; - return -1; - } - - virMutexLock(m); - return 0; -} - -int virCondWaitUntil(virCondPtr c ATTRIBUTE_UNUSED, - virMutexPtr m ATTRIBUTE_UNUSED, - unsigned long long whenms ATTRIBUTE_UNUSED) -{ - /* FIXME: this function is currently only used by the QEMU driver that - * is not compiled on Windows, so it's okay for now to just - * miss an implementation */ - return -1; -} - -void virCondSignal(virCondPtr c) -{ - virMutexLock(&c->lock); - - if (c->nwaiters) { - HANDLE event = c->waiters[0]; - if (c->nwaiters > 1) - memmove(c->waiters, - c->waiters + 1, - sizeof(c->waiters[0]) * (c->nwaiters-1)); - if (VIR_REALLOC_N(c->waiters, c->nwaiters - 1) < 0) { - ; - } - c->nwaiters--; - SetEvent(event); - } - - virMutexUnlock(&c->lock); -} - -void virCondBroadcast(virCondPtr c) -{ - virMutexLock(&c->lock); - - if (c->nwaiters) { - size_t i; - for (i = 0; i < c->nwaiters; i++) { - HANDLE event = c->waiters[i]; - SetEvent(event); - } - VIR_FREE(c->waiters); - c->nwaiters = 0; - } - - virMutexUnlock(&c->lock); -} - - -struct virThreadArgs { - virThreadFunc func; - void *opaque; -}; - -static void virThreadHelperDaemon(void *data) -{ - struct virThreadArgs *args = data; - virThread self; - HANDLE handle = GetCurrentThread(); - HANDLE process = GetCurrentProcess(); - - self.joinable = false; - DuplicateHandle(process, handle, process, - &self.thread, 0, FALSE, - DUPLICATE_SAME_ACCESS); - TlsSetValue(selfkey, &self); - - args->func(args->opaque); - - TlsSetValue(selfkey, NULL); - CloseHandle(self.thread); - - VIR_FREE(args); -} - -static unsigned int __stdcall virThreadHelperJoinable(void *data) -{ - struct virThreadArgs *args = data; - virThread self; - HANDLE handle = GetCurrentThread(); - HANDLE process = GetCurrentProcess(); - - self.joinable = true; - DuplicateHandle(process, handle, process, - &self.thread, 0, FALSE, - DUPLICATE_SAME_ACCESS); - TlsSetValue(selfkey, &self); - - args->func(args->opaque); - - TlsSetValue(selfkey, NULL); - CloseHandle(self.thread); - - VIR_FREE(args); - return 0; -} - -int virThreadCreate(virThreadPtr thread, - bool joinable, - virThreadFunc func, - void *opaque) -{ - struct virThreadArgs *args; - uintptr_t ret; - - if (VIR_ALLOC(args) < 0) - return -1; - - args->func = func; - args->opaque = opaque; - - thread->joinable = joinable; - if (joinable) { - ret = _beginthreadex(NULL, 0, - virThreadHelperJoinable, - args, 0, NULL); - if (ret == 0) - return -1; - } else { - ret = _beginthread(virThreadHelperDaemon, - 0, args); - if (ret == -1L) - return -1; - } - - thread->thread = (HANDLE)ret; - - return 0; -} - -void virThreadSelf(virThreadPtr thread) -{ - virThreadPtr self = TlsGetValue(selfkey); - - if (self == NULL) { - /* called on a thread not created by virThreadCreate, e.g. the main thread */ - thread->thread = 0; - thread->joinable = false; - } else { - thread->thread = self->thread; - thread->joinable = self->joinable; - } -} - -bool virThreadIsSelf(virThreadPtr thread) -{ - virThread self; - virThreadSelf(&self); - return self.thread == thread->thread ? true : false; -} - -/* For debugging use only; see comments in virthreadpthread.c. */ -unsigned long long virThreadSelfID(void) -{ - return GetCurrentThreadId(); -} - -/* For debugging use only; see comments in virthreadpthread.c. */ -unsigned long long virThreadID(virThreadPtr thread) -{ - return (intptr_t)thread->thread; -} - - -void virThreadJoin(virThreadPtr thread) -{ - if (thread->joinable) { - WaitForSingleObject(thread->thread, INFINITE); - CloseHandle(thread->thread); - thread->thread = 0; - thread->joinable = false; - } -} - -void virThreadCancel(virThreadPtr thread ATTRIBUTE_UNUSED) -{} - -int virThreadLocalInit(virThreadLocalPtr l, - virThreadLocalCleanup c) -{ - if ((l->key = TlsAlloc()) == TLS_OUT_OF_INDEXES) { - errno = ESRCH; - return -1; - } - TlsSetValue(l->key, NULL); - - if (c) { - virMutexLock(&virThreadLocalLock); - if (VIR_REALLOC_N(virThreadLocalList, - virThreadLocalCount + 1) < 0) - return -1; - virThreadLocalList[virThreadLocalCount].key = l->key; - virThreadLocalList[virThreadLocalCount].cleanup = c; - virThreadLocalCount++; - virMutexUnlock(&virThreadLocalLock); - } - - return 0; -} - -void *virThreadLocalGet(virThreadLocalPtr l) -{ - return TlsGetValue(l->key); -} - -int virThreadLocalSet(virThreadLocalPtr l, void *val) -{ - return TlsSetValue(l->key, val) == 0 ? -1 : 0; -} diff --git a/src/util/virthreadwin32.h b/src/util/virthreadwin32.h deleted file mode 100644 index 31d9444..0000000 --- a/src/util/virthreadwin32.h +++ /dev/null @@ -1,57 +0,0 @@ -/* - * virthreadwin32.h basic thread synchronization primitives - * - * Copyright (C) 2009, 2011-2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include "internal.h" - -#ifdef HAVE_WINSOCK2_H -# include <winsock2.h> -#endif -#include <windows.h> - -struct virMutex { - HANDLE lock; -}; - -struct virRWLock { - bool ignored; -}; - -struct virCond { - virMutex lock; - size_t nwaiters; - HANDLE *waiters; -}; - -struct virThread { - HANDLE thread; - bool joinable; -}; - -struct virThreadLocal { - DWORD key; -}; - -struct virOnceControl { - volatile long init; /* 0 at startup, > 0 if init has started */ - volatile long complete; /* 0 until first thread completes callback */ -}; - -#define VIR_ONCE_CONTROL_INITIALIZER { 0, 0 } -- 1.8.4.2
The NWFilter code has as a deadlock race condition between the virNWFilter{Define,Undefine} APIs and starting of guest VMs due to mis-matched lock ordering. In the virNWFilter{Define,Undefine} codepaths the lock ordering is 1. nwfilter driver lock 2. virt driver lock 3. nwfilter update lock 4. domain object lock In the VM guest startup paths the lock ordering is 1. virt driver lock 2. domain object lock 3. nwfilter update lock As can be seen the domain object and nwfilter update locks are not acquired in a consistent order. The fix used is to push the nwfilter update lock upto the top level resulting in a lock ordering for virNWFilter{Define,Undefine} of 1. nwfilter driver lock 2. nwfilter update lock 3. virt driver lock 4. domain object lock and VM start using 1. nwfilter update lock 2. virt driver lock 3. domain object lock This has the effect of serializing VM startup once again, even if no nwfilters are applied to the guest. There is also the possibility of deadlock due to a call graph loop via virNWFilterInstantiate and virNWFilterInstantiateFilterLate. These two problems mean the lock must be turned into a read/write lock instead of a plain mutex at the same time. The lock is used to serialize changes to the "driver->nwfilters" hash, so the write lock only needs to be held by the define/undefine methods. All other methods can rely on a read lock which allows good concurrency. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 23 +++++++++++------------ src/conf/nwfilter_conf.h | 3 ++- src/libvirt_private.syms | 3 ++- src/lxc/lxc_driver.c | 6 ++++++ src/nwfilter/nwfilter_driver.c | 10 ++++++---- src/nwfilter/nwfilter_gentech_driver.c | 6 +----- src/qemu/qemu_driver.c | 6 ++++++ src/uml/uml_driver.c | 4 ++++ 8 files changed, 38 insertions(+), 23 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 6db8ea9..52e1c06 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -143,17 +143,22 @@ static const struct int_map chain_priorities[] = { /* * only one filter update allowed */ -static virMutex updateMutex; +static virRWLock updateLock; static bool initialized = false; void -virNWFilterLockFilterUpdates(void) { - virMutexLock(&updateMutex); +virNWFilterReadLockFilterUpdates(void) { + virRWLockRead(&updateLock); +} + +void +virNWFilterWriteLockFilterUpdates(void) { + virRWLockWrite(&updateLock); } void virNWFilterUnlockFilterUpdates(void) { - virMutexUnlock(&updateMutex); + virRWLockUnlock(&updateLock); } @@ -2990,14 +2995,12 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, return NULL; } - virNWFilterLockFilterUpdates(); if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) { if (virNWFilterDefEqual(def, nwfilter->def, false)) { virNWFilterDefFree(nwfilter->def); nwfilter->def = def; - virNWFilterUnlockFilterUpdates(); return nwfilter; } @@ -3005,7 +3008,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild()) { nwfilter->newDef = NULL; - virNWFilterUnlockFilterUpdates(); virNWFilterObjUnlock(nwfilter); return NULL; } @@ -3013,12 +3015,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefFree(nwfilter->def); nwfilter->def = def; nwfilter->newDef = NULL; - virNWFilterUnlockFilterUpdates(); return nwfilter; } - virNWFilterUnlockFilterUpdates(); - if (VIR_ALLOC(nwfilter) < 0) return NULL; @@ -3483,7 +3482,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, initialized = true; - if (virMutexInitRecursive(&updateMutex) < 0) + if (virRWLockInit(&updateLock) < 0) return -1; return 0; @@ -3495,7 +3494,7 @@ void virNWFilterConfLayerShutdown(void) if (!initialized) return; - virMutexDestroy(&updateMutex); + virRWLockDestroy(&updateLock); initialized = false; virNWFilterDomainFWUpdateOpaque = NULL; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 6b8b515..0d09b6a 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -716,7 +716,8 @@ virNWFilterDefPtr virNWFilterDefParseFile(const char *filename); void virNWFilterObjLock(virNWFilterObjPtr obj); void virNWFilterObjUnlock(virNWFilterObjPtr obj); -void virNWFilterLockFilterUpdates(void); +void virNWFilterWriteLockFilterUpdates(void); +void virNWFilterReadLockFilterUpdates(void); void virNWFilterUnlockFilterUpdates(void); int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ad9ad28..d77bbb2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -575,7 +575,6 @@ virNWFilterDefParseString; virNWFilterInstFiltersOnAllVMs; virNWFilterJumpTargetTypeToString; virNWFilterLoadAllConfigs; -virNWFilterLockFilterUpdates; virNWFilterObjAssignDef; virNWFilterObjDeleteDef; virNWFilterObjFindByName; @@ -587,6 +586,7 @@ virNWFilterObjSaveDef; virNWFilterObjUnlock; virNWFilterPrintStateMatchFlags; virNWFilterPrintTCPFlags; +virNWFilterReadLockFilterUpdates; virNWFilterRegisterCallbackDriver; virNWFilterRuleActionTypeToString; virNWFilterRuleDirectionTypeToString; @@ -594,6 +594,7 @@ virNWFilterRuleProtocolTypeToString; virNWFilterTestUnassignDef; virNWFilterUnlockFilterUpdates; virNWFilterUnRegisterCallbackDriver; +virNWFilterWriteLockFilterUpdates; # conf/nwfilter_ipaddrmap.h diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 982f3fc..73a2986 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1015,6 +1015,8 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); + virNWFilterReadLockFilterUpdates(); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; @@ -1053,6 +1055,7 @@ cleanup: if (event) virObjectEventStateQueue(driver->domainEventState, event); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); return ret; } @@ -1109,6 +1112,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); + virNWFilterReadLockFilterUpdates(); + if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -1164,6 +1169,7 @@ cleanup: virObjectEventStateQueue(driver->domainEventState, event); virObjectUnref(caps); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); return dom; } diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d21dd82..5908df7 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -283,12 +283,14 @@ nwfilterStateReload(void) virNWFilterLearnThreadsTerminate(true); nwfilterDriverLock(driverState); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); virNWFilterLoadAllConfigs(&driverState->nwfilters, driverState->configDir); virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driverState); virNWFilterInstFiltersOnAllVMs(); @@ -554,6 +556,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(driver); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); if (!(def = virNWFilterDefParseString(xml))) @@ -580,6 +583,7 @@ cleanup: virNWFilterObjUnlock(nwfilter); virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driver); return ret; } @@ -592,10 +596,9 @@ nwfilterUndefine(virNWFilterPtr obj) { int ret = -1; nwfilterDriverLock(driver); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - virNWFilterLockFilterUpdates(); - nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); if (!nwfilter) { virReportError(VIR_ERR_NO_NWFILTER, @@ -626,9 +629,8 @@ cleanup: if (nwfilter) virNWFilterObjUnlock(nwfilter); - virNWFilterUnlockFilterUpdates(); - virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driver); return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..8c5cd57 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -935,8 +935,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int ifindex; int rc; - virNWFilterLockFilterUpdates(); - /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed (while holding the lock) and we don't want to build new ones */ @@ -964,8 +962,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter); cleanup: - virNWFilterUnlockFilterUpdates(); - return rc; } @@ -984,7 +980,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int rc; bool foundNewFilter = false; - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); rc = __virNWFilterInstantiateFilter(driver, vmuuid, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a555470..4e6e6fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1576,6 +1576,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_AUTODESTROY) start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY; + virNWFilterReadLockFilterUpdates(); + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -1656,6 +1658,7 @@ cleanup: } virObjectUnref(caps); virObjectUnref(qemuCaps); + virNWFilterUnlockFilterUpdates(); return dom; } @@ -6095,6 +6098,8 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_START_BYPASS_CACHE | VIR_DOMAIN_START_FORCE_BOOT, -1); + virNWFilterReadLockFilterUpdates(); + if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -6122,6 +6127,7 @@ endjob: cleanup: if (vm) virObjectUnlock(vm); + virNWFilterUnlockFilterUpdates(); return ret; } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 89afefe..309c10e 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1574,6 +1574,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); + virNWFilterReadLockFilterUpdates(); umlDriverLock(driver); if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, 1 << VIR_DOMAIN_VIRT_UML, @@ -1613,6 +1614,7 @@ cleanup: if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); + virNWFilterUnlockFilterUpdates(); return dom; } @@ -1997,6 +1999,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); + virNWFilterReadLockFilterUpdates(); umlDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); @@ -2023,6 +2026,7 @@ cleanup: if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); + virNWFilterUnlockFilterUpdates(); return ret; } -- 1.8.4.2
On 01/29/2014 09:53 AM, Daniel P. Berrange wrote:
The NWFilter code has as a deadlock race condition between the virNWFilter{Define,Undefine} APIs and starting of guest VMs due to mis-matched lock ordering.
In the virNWFilter{Define,Undefine} codepaths the lock ordering is
1. nwfilter driver lock 2. virt driver lock 3. nwfilter update lock 4. domain object lock
In the VM guest startup paths the lock ordering is
1. virt driver lock 2. domain object lock 3. nwfilter update lock
As can be seen the domain object and nwfilter update locks are not acquired in a consistent order.
The fix used is to push the nwfilter update lock upto the top level resulting in a lock ordering for virNWFilter{Define,Undefine} of
1. nwfilter driver lock 2. nwfilter update lock 3. virt driver lock 4. domain object lock
and VM start using
1. nwfilter update lock 2. virt driver lock 3. domain object lock
This has the effect of serializing VM startup once again, even if no nwfilters are applied to the guest. There is also the possibility of deadlock due to a call graph loop via virNWFilterInstantiate and virNWFilterInstantiateFilterLate.
These two problems mean the lock must be turned into a read/write lock instead of a plain mutex at the same time. The lock is used to serialize changes to the "driver->nwfilters" hash, so the write lock only needs to be held by the define/undefine methods. All other methods can rely on a read lock which allows good concurrency.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
ACK Stefan
On 01/29/2014 09:53 AM, Daniel P. Berrange wrote: I did a few concurrency tests now and the results look good. Stefan
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Stefan Berger