From 61b52d2e3813cc8c9ff3ab67f232bd0c65f7318d Mon Sep 17 00:00:00 2001
From: "Daniel P. Berrange" <berrange@redhat.com>
Date: Mon, 11 Feb 2013 16:08:42 +0000
Subject: [PATCH] Fix potential deadlock across fork() in QEMU driver

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>
---
 src/libvirt_private.syms        |  2 ++
 src/qemu/qemu_process.c         | 16 ++++++++++++----
 src/security/security_manager.c | 20 ++++++++++++++++++++
 src/security/security_manager.h |  3 +++
 4 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bea3bf6e72..b9d45a200c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1055,6 +1055,8 @@ virSecurityManagerGetProcessLabel;
 virSecurityManagerNew;
 virSecurityManagerNewDAC;
 virSecurityManagerNewStack;
+virSecurityManagerPostFork;
+virSecurityManagerPreFork;
 virSecurityManagerReleaseLabel;
 virSecurityManagerReserveLabel;
 virSecurityManagerRestoreAllLabel;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 975933239a..12e354409c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2773,6 +2773,7 @@ struct qemuProcessHookData {
     virDomainObjPtr vm;
     virQEMUDriverPtr driver;
     virBitmapPtr nodemask;
+    virQEMUDriverConfigPtr cfg;
 };
 
 static int qemuProcessHook(void *data)
@@ -2780,7 +2781,11 @@ static int qemuProcessHook(void *data)
     struct qemuProcessHookData *h = data;
     int ret = -1;
     int fd;
-    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(h->driver);
+    /* 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();
@@ -2796,7 +2801,7 @@ static int qemuProcessHook(void *data)
     if (virSecurityManagerSetSocketLabel(h->driver->securityManager, h->vm->def) < 0)
         goto cleanup;
     if (virDomainLockProcessStart(h->driver->lockManager,
-                                  cfg->uri,
+                                  h->cfg->uri,
                                   h->vm,
                                   /* QEMU is always paused initially */
                                   true,
@@ -2805,7 +2810,7 @@ static int qemuProcessHook(void *data)
     if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def) < 0)
         goto cleanup;
 
-    if (qemuProcessLimits(cfg) < 0)
+    if (qemuProcessLimits(h->cfg) < 0)
         goto cleanup;
 
     /* This must take place before exec(), so that all QEMU
@@ -2831,7 +2836,7 @@ static int qemuProcessHook(void *data)
     ret = 0;
 
 cleanup:
-    virObjectUnref(cfg);
+    virObjectUnref(h->cfg);
     VIR_DEBUG("Hook complete ret=%d", ret);
     return ret;
 }
@@ -3642,6 +3647,7 @@ int qemuProcessStart(virConnectPtr conn,
     hookData.conn = conn;
     hookData.vm = vm;
     hookData.driver = driver;
+    hookData.cfg = virObjectRef(cfg);
 
     VIR_DEBUG("Beginning VM startup process");
 
@@ -3973,7 +3979,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 6f8ddbf74c..50962bafb4 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -192,6 +192,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)
+{
+    virObjectLock(mgr);
+}
+
+
+/*
+ * 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)
+{
+    virObjectUnlock(mgr);
+}
+
 void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
 {
     return mgr->privateData;
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 4d4dc73181..8e8accf51f 100644
--- 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);
 
 const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr);