Fix potential deadlock across fork() in QEMU driver

https://bugzilla.redhat.com/show_bug.cgi?id=964358

The hook scripts used by virCommand must be careful wrt
accessing any mutexes that may have been held by other
threads in the parent process. With the recent refactoring
there are 2 potential flaws lurking, which will become real
deadlock bugs once the global QEMU driver lock is removed.

Remove use of the QEMU driver lock from the hook function
by passing in the 'virQEMUDriverConfigPtr' instance directly.

Add functions to the virSecurityManager to be invoked before
and after fork, to ensure the mutex is held by the current
thread. This allows it to be safely used in the hook script
in the child process.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 61b52d2e3813cc8c9ff3ab67f232bd0c65f7318d)

Conflicts:
	src/libvirt_private.syms - context
	src/qemu/qemu_process.c - no backport of qemud_driver struct rename
	src/security/security_manager.c - no backport of making the security driver self-locking; just expose the interface

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 24f704736d24d0e10a4dfe827b31a7f9325c5c75..17e5eff511eef3469de59f53d6c005d063a0ca81 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1047,6 +1047,8 @@ virSecurityManagerGetProcessLabel;
 virSecurityManagerNew;
 virSecurityManagerNewStack;
 virSecurityManagerNewDAC;
+virSecurityManagerPostFork;
+virSecurityManagerPreFork;
 virSecurityManagerReleaseLabel;
 virSecurityManagerReserveLabel;
 virSecurityManagerRestoreImageLabel;

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index cfadc2c3e8b83a198dffa39fff72b3e088a6cb10..01c68806ddca10e9ffe42a1858fb5e6ca00f8747 100644 (file)
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2579,6 +2579,11 @@ static int qemuProcessHook(void *data)
     struct qemuProcessHookData *h = data;
     int ret = -1;
     int fd;
+    /* This method cannot use any mutexes, which are not
+     * protected across fork()
+     */
+
+    virSecurityManagerPostFork(h->driver->securityManager);
 
     /* Some later calls want pid present */
     h->vm->pid = getpid();
@@ -3640,7 +3645,9 @@ int qemuProcessStart(virConnectPtr conn,
     virCommandDaemonize(cmd);
     virCommandRequireHandshake(cmd);
 
+    virSecurityManagerPreFork(driver->securityManager);
     ret = virCommandRun(cmd, NULL);
+    virSecurityManagerPostFork(driver->securityManager);
 
     /* wait for qemu process to show up */
     if (ret == 0) {

diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index d446607fcd980573c66d386360c233ff55df3b95..fc7acff4d25e5833de2382ca3d378db2ed9152db 100644 (file)
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -161,6 +161,26 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name,
                                        requireConfined);
 }
 
+
+/*
+ * Must be called before fork()'ing to ensure mutex state
+ * is sane for the child to use
+ */
+void virSecurityManagerPreFork(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
+{
+    /* XXX Grab our own mutex here instead of relying on caller's mutex */
+}
+
+
+/*
+ * Must be called after fork()'ing in both parent and child
+ * to ensure mutex state is sane for the child to use
+ */
+void virSecurityManagerPostFork(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
+{
+    /* XXX Release our own mutex here instead of relying on caller's mutex */
+}
+
 void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
 {
     /* This accesses the memory just beyond mgr, which was allocated

diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 1fdaf8e964c98f9b14bacfb38379d31c8f6f0802..d1a59974ea9e9ecd7facb0867a83f6a7347a5168 100644 (file)
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -46,6 +46,9 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
                                                bool requireConfined,
                                                bool dynamicOwnership);
 
+void virSecurityManagerPreFork(virSecurityManagerPtr mgr);
+void virSecurityManagerPostFork(virSecurityManagerPtr mgr);
+
 void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr);
 
 void virSecurityManagerFree(virSecurityManagerPtr mgr);