[libvirt] [PATCH 2/2] qemu: Don't overwrite security labels

[Michal Privoznik]

On 11.06.2012 18:54, Eric Blake wrote:
> On 06/11/2012 08:29 AM, Michal Privoznik wrote:
>> Currently, if qemuProcessStart fail at some point, e.g. because
>> domain being started wants a PCI/USB device already assigned to
>> a different domain, we jump to cleanup label where qemuProcessStop
>> is performed. This unconditionally calls virSecurityManagerRestoreAllLabel
>> which is wrong because the other domain is still using those devices.
>>
>> However, once we successfully label all devices/paths in
>> qemuProcessStart() from that point on, we have to perform a rollback
>> on failure - that is - we have to virSecurityManagerRestoreAllLabel.
>> ---
>>  src/qemu/qemu_process.c |   12 ++++++++----
>>  src/qemu/qemu_process.h |    3 ++-
>>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> Double-negative logic.  But I guess we're stuck with it, as the default
> of 'flags==0' must imply the relabel.
> 
>> @@ -3984,9 +3987,10 @@ void qemuProcessStop(struct qemud_driver *driver,
>>      }
>>  
>>      /* Reset Security Labels */
>> -    virSecurityManagerRestoreAllLabel(driver->securityManager,
>> -                                      vm->def,
>> -                                      flags & VIR_QEMU_PROCESS_STOP_MIGRATED);
>> +    if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL))
> 
> Took me a couple reads to convince myself that I couldn't come up with
> any nicer wording of this condition without breaking defaults.
> 
> ACK.
> 

Yeah, it is quite confusing, therefore I am squashing in some comments
before pushing:

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0309bfb..5f3aa99 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3281,7 +3281,7 @@ int qemuProcessStart(virConnectPtr conn,
     int i;
     char *nodeset = NULL;
     char *nodemask = NULL;
-    unsigned int stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL;
+    unsigned int stop_flags;

     /* Okay, these are just internal flags,
      * but doesn't hurt to check */
@@ -3289,6 +3289,12 @@ int qemuProcessStart(virConnectPtr conn,
                   VIR_QEMU_PROCESS_START_PAUSED |
                   VIR_QEMU_PROCESS_START_AUTODESROY, -1);

+    /* From now on until domain security labeling is done:
+     * if any operation fails and we goto cleanup, we must not
+     * restore any security label as we would overwrite labels
+     * we did not set. */
+    stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL;
+
     hookData.conn = conn;
     hookData.vm = vm;
     hookData.driver = driver;
@@ -3632,6 +3638,10 @@ int qemuProcessStart(virConnectPtr conn,
                                       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) {
@@ -3986,7 +3996,7 @@ void qemuProcessStop(struct qemud_driver *driver,
         VIR_FREE(xml);
     }

-    /* Reset Security Labels */
+    /* Reset Security Labels unless caller don't want us to */
     if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL))
         virSecurityManagerRestoreAllLabel(driver->securityManager,
                                           vm->def,


---

And pushed now. Thanks for review!

Michal