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