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>

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bea3bf6e72c7cd0d6cfe2a7c27348a9e26da60eb..b9d45a200c436292bc7ac6a804989445ab6acb7f 100644 (file)
--- 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 975933239aba730881a7977daa923a12c2383733..12e354409cf0e830d8a4c7fdbbcb0d1296c93fd5 100644 (file)
--- 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 6f8ddbf74c0be24b62bb7c161af0fdadcfc8b3fa..50962bafb420f63f9d5810aa0526773847ca3d3e 100644 (file)
--- 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 4d4dc73181bcc18746da06ccfe63baf32a32e7e0..8e8accf51fc10dfbf9de063a8f2b9098309a0148 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);
 
 const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr);