Bug 1113327

I just did another test and determined that this can be reproduced on a single host using a regular local filesystem for image storage.  Follow the steps above on a single host, using a local filesystem and create two VMs that use the same image file as their virtual disk.

When the second one fails to start due to the lock, the first loses contact with its disk.  However, the lock remains held, and multiple attempts to start the second VM will continue to fail until the first VM is powered off and the lock released.

The problem is, when starting a new domain libvirt executes the following operations (P = parent (libvirtd), C = Child):

1P) prepare cmd line
2P) fork()
3C) lock_domain_disks()
4P) do the labeling
5P) Notify child
6C) exec(qemu, ...)


The problem is disk locking and labeling are done in parallel, so if one of if fails, we may have unfortunatelly done the other one. Looking deeper into the code this diff should fix the bug:

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 5b598be..7cb0ae9 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2736,6 +2736,12 @@ static int qemuProcessHook(void *data)
     if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def) < 0)
         goto cleanup;
 
+    VIR_DEBUG("Setting domain security labels");
+    if (virSecurityManagerSetAllLabel(driver->securityManager,
+                                      vm->def, stdin_path) < 0)
+        goto cleanup;
+    VIR_DEBUG("Labelling done");
+
     if (virNumaSetupMemoryPolicy(h->vm->def->numatune, h->nodemask) < 0)
         goto cleanup;
 
@@ -4092,17 +4098,6 @@ int qemuProcessStart(virConnectPtr conn,
         qemuProcessInitCpuAffinity(driver, vm, nodemask) < 0)
         goto cleanup;
 
-    VIR_DEBUG("Setting domain security labels");
-    if (virSecurityManagerSetAllLabel(driver->securityManager,
-                                      vm->def, stdin_path) < 0)
-        goto cleanup;
-
-    /* Security manager labeled all devices, therefore
-     * if any operation from now on fails and we goto cleanup,
-     * where virSecurityManagerRestoreAllLabel() is called
-     * (hidden under qemuProcessStop) we need to restore labels. */
-    stop_flags &= ~VIR_QEMU_PROCESS_STOP_NO_RELABEL;
-
     if (stdin_fd != -1) {
         /* if there's an fd to migrate from, and it's a pipe, put the
          * proper security label on it
@@ -4121,12 +4116,17 @@ int qemuProcessStart(virConnectPtr conn,
             goto cleanup;
     }
 
-    VIR_DEBUG("Labelling done, completing handshake to child");
     if (virCommandHandshakeNotify(cmd) < 0) {
         goto cleanup;
     }
     VIR_DEBUG("Handshake complete, child running");
 
+    /* Security manager labeled all devices, therefore
+     * if any operation from now on fails and we goto cleanup,
+     * where virSecurityManagerRestoreAllLabel() is called
+     * (hidden under qemuProcessStop) we need to restore labels. */
+    stop_flags &= ~VIR_QEMU_PROCESS_STOP_NO_RELABEL;
+
     if (migrateFrom)
         flags |= VIR_QEMU_PROCESS_START_PAUSED;

I've missed your comment, and I had a patch prepared already, the proposal is here:

https://www.redhat.com/archives/libvir-list/2014-June/msg01352.html

*** This bug has been marked as a duplicate of bug 547546 ***

How is this a duplicate of 547546?

The bug isn't that permissions are not restored, as described in 547546, but that they should never have been adjusted in the first place when locks had not yet been acquired.  Anything other than strict ordering of lock acquisition prior to any modification to disk image state or permissions will inherently have race conditions and endanger the existing domain that already holds valid locks...

(In reply to Need Real Name from comment #7)
> How is this a duplicate of 547546?
> 
> The bug isn't that permissions are not restored, as described in 547546, but
> that they should never have been adjusted in the first place when locks had
> not yet been acquired.  Anything other than strict ordering of lock
> acquisition prior to any modification to disk image state or permissions
> will inherently have race conditions and endanger the existing domain that
> already holds valid locks...

The patchset proposed in the referenced bug should fix this bug too.