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

@@ -1055,6 +1055,8 @@ virSecurityManagerGetProcessLabel;
 virSecurityManagerNew;
 virSecurityManagerNewDAC;
 virSecurityManagerNewStack;
+virSecurityManagerPostFork;
+virSecurityManagerPreFork;
 virSecurityManagerReleaseLabel;
 virSecurityManagerReserveLabel;
 virSecurityManagerRestoreAllLabel;

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) {

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;

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);