[libvirt] [PATCH 0/2] Don't overwrite security labels
If we have a running domain A with a PCI/USB device X passthrough, and user wants to start domain B with again device X, we reject start however, call qemuProcessStop to perform rollback. This however does not do any rollback just cleanup therefore rewrite labels on device X leaving domain A in silly situation. Michal Privoznik (2): qemuProcessStop: Switch to flags qemu: Don't overwrite security labels src/qemu/qemu_driver.c | 16 ++++++++-------- src/qemu/qemu_migration.c | 20 +++++++++++++------- src/qemu/qemu_process.c | 28 +++++++++++++++++----------- src/qemu/qemu_process.h | 9 +++++++-- 4 files changed, 45 insertions(+), 28 deletions(-) -- 1.7.8.5
Currently, we are passing only one boolean (migrated) so there is no real profit in this. But it creates starting position for next patch. --- src/qemu/qemu_driver.c | 16 ++++++++-------- src/qemu/qemu_migration.c | 20 +++++++++++++------- src/qemu/qemu_process.c | 22 ++++++++++++---------- src/qemu/qemu_process.h | 8 ++++++-- 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3f74d2..bda07c7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1870,7 +1870,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, goto endjob; } - qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_DESTROYED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); @@ -2731,7 +2731,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, ret = 0; /* Shut it down */ - qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_SAVED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0); virDomainAuditStop(vm, "saved"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -3136,7 +3136,7 @@ static int qemudDomainCoreDump(virDomainPtr dom, endjob: if ((ret == 0) && (flags & VIR_DUMP_CRASH)) { - qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_CRASHED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0); virDomainAuditStop(vm, "crashed"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -9781,7 +9781,7 @@ qemuDomainSnapshotCreateActive(virConnectPtr conn, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); - qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); /* We already filtered the _HALT flag for persistent domains * only, so this end job never drops the last reference. */ @@ -10255,7 +10255,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); - qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); /* We already filtered the _HALT flag for persistent domains * only, so this end job never drops the last reference. */ @@ -11072,8 +11072,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } virResetError(err); - qemuProcessStop(driver, vm, 0, - VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + qemuProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventNewFromObj(vm, @@ -11186,7 +11186,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (virDomainObjIsActive(vm)) { /* Transitions 4, 7 */ - qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventNewFromObj(vm, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a1fb962..b893fd5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1329,7 +1329,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, virReportSystemError(errno, "%s", _("cannot pass pipe for tunnelled migration")); virDomainAuditStart(vm, "migrated", false); - qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_FAILED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0); goto endjob; } dataFD[1] = -1; /* 'st' owns the FD now & will close it */ @@ -2647,7 +2647,8 @@ qemuMigrationPerformJob(struct qemud_driver *driver, * confirm step. */ if (!v3proto) { - qemuProcessStop(driver, vm, 1, VIR_DOMAIN_SHUTOFF_MIGRATED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED, + VIR_QEMU_PROCESS_STOP_MIGRATED); virDomainAuditStop(vm, "migrated"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -2944,7 +2945,8 @@ qemuMigrationFinish(struct qemud_driver *driver, } if (qemuMigrationVPAssociatePortProfiles(vm->def) < 0) { - qemuProcessStop(driver, vm, 1, VIR_DOMAIN_SHUTOFF_FAILED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, + VIR_QEMU_PROCESS_STOP_MIGRATED); virDomainAuditStop(vm, "failed"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -2977,7 +2979,8 @@ qemuMigrationFinish(struct qemud_driver *driver, * to restart during confirm() step, so we kill it off now. */ if (v3proto) { - qemuProcessStop(driver, vm, 1, VIR_DOMAIN_SHUTOFF_FAILED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, + VIR_QEMU_PROCESS_STOP_MIGRATED); virDomainAuditStop(vm, "failed"); if (newVM) vm->persistent = 0; @@ -3023,7 +3026,8 @@ qemuMigrationFinish(struct qemud_driver *driver, * things up */ if (v3proto) { - qemuProcessStop(driver, vm, 1, VIR_DOMAIN_SHUTOFF_FAILED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, + VIR_QEMU_PROCESS_STOP_MIGRATED); virDomainAuditStop(vm, "failed"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -3054,7 +3058,8 @@ qemuMigrationFinish(struct qemud_driver *driver, /* Guest is successfully running, so cancel previous auto destroy */ qemuProcessAutoDestroyRemove(driver, vm); } else { - qemuProcessStop(driver, vm, 1, VIR_DOMAIN_SHUTOFF_FAILED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, + VIR_QEMU_PROCESS_STOP_MIGRATED); virDomainAuditStop(vm, "failed"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -3118,7 +3123,8 @@ int qemuMigrationConfirm(struct qemud_driver *driver, * domain object, but if no, resume CPUs */ if (retcode == 0) { - qemuProcessStop(driver, vm, 1, VIR_DOMAIN_SHUTOFF_MIGRATED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED, + VIR_QEMU_PROCESS_STOP_MIGRATED); virDomainAuditStop(vm, "migrated"); event = virDomainEventNewFromObj(vm, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 33f57be..4cb0a83 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -311,7 +311,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, eventReason); - qemuProcessStop(driver, vm, 0, stopReason); + qemuProcessStop(driver, vm, stopReason, 0); virDomainAuditStop(vm, auditReason); if (!vm->persistent) { @@ -3151,7 +3151,7 @@ error: * really is and FAILED means "failed to start" */ state = VIR_DOMAIN_SHUTOFF_UNKNOWN; } - qemuProcessStop(driver, obj, 0, state); + qemuProcessStop(driver, obj, state, 0); if (!obj->persistent) qemuDomainRemoveInactive(driver, obj); else @@ -3229,7 +3229,7 @@ qemuProcessReconnectHelper(void *payload, } else if (virDomainObjUnref(obj) > 0) { /* We can't spawn a thread and thus connect to monitor. * Kill qemu */ - qemuProcessStop(src->driver, obj, 0, VIR_DOMAIN_SHUTOFF_FAILED); + qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, 0); if (!obj->persistent) qemuDomainRemoveInactive(src->driver, obj); else @@ -3770,7 +3770,7 @@ cleanup: VIR_FREE(nodemask); virCommandFree(cmd); VIR_FORCE_CLOSE(logfile); - qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_FAILED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0); return -1; } @@ -3873,8 +3873,8 @@ cleanup: void qemuProcessStop(struct qemud_driver *driver, virDomainObjPtr vm, - int migrated, - virDomainShutoffReason reason) + virDomainShutoffReason reason, + unsigned int flags) { int ret; int retries = 0; @@ -3887,8 +3887,8 @@ void qemuProcessStop(struct qemud_driver *driver, char *timestamp; char ebuf[1024]; - VIR_DEBUG("Shutting down VM '%s' pid=%d migrated=%d", - vm->def->name, vm->pid, migrated); + VIR_DEBUG("Shutting down VM '%s' pid=%d flags=%x", + vm->def->name, vm->pid, flags); if (!virDomainObjIsActive(vm)) { VIR_DEBUG("VM '%s' not active", vm->def->name); @@ -3985,7 +3985,8 @@ void qemuProcessStop(struct qemud_driver *driver, /* Reset Security Labels */ virSecurityManagerRestoreAllLabel(driver->securityManager, - vm->def, migrated); + vm->def, + flags & VIR_QEMU_PROCESS_STOP_MIGRATED); virSecurityManagerReleaseLabel(driver->securityManager, vm->def); /* Clear out dynamically assigned labels */ @@ -4304,7 +4305,8 @@ qemuProcessAutoDestroy(struct qemud_driver *driver, goto cleanup; VIR_DEBUG("Killing domain"); - qemuProcessStop(driver, dom, 1, VIR_DOMAIN_SHUTOFF_DESTROYED); + qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED, + VIR_QEMU_PROCESS_STOP_MIGRATED); virDomainAuditStop(dom, "destroyed"); event = virDomainEventNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2d75b005..ce59215 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -60,10 +60,14 @@ int qemuProcessStart(virConnectPtr conn, enum virNetDevVPortProfileOp vmop, unsigned int flags); +typedef enum { + VIR_QEMU_PROCESS_STOP_MIGRATED = 1 << 0, +} qemuProcessStopFlags; + void qemuProcessStop(struct qemud_driver *driver, virDomainObjPtr vm, - int migrated, - virDomainShutoffReason reason); + virDomainShutoffReason reason, + unsigned int flags); int qemuProcessAttach(virConnectPtr conn, struct qemud_driver *driver, -- 1.7.8.5
On 06/11/2012 08:29 AM, Michal Privoznik wrote:
Currently, we are passing only one boolean (migrated) so there is no real profit in this. But it creates starting position for next patch. --- src/qemu/qemu_driver.c | 16 ++++++++-------- src/qemu/qemu_migration.c | 20 +++++++++++++------- src/qemu/qemu_process.c | 22 ++++++++++++---------- src/qemu/qemu_process.h | 8 ++++++-- 4 files changed, 39 insertions(+), 27 deletions(-)
ACK - fairly mechanical and straightforward.
@@ -3985,7 +3985,8 @@ void qemuProcessStop(struct qemud_driver *driver,
/* Reset Security Labels */ virSecurityManagerRestoreAllLabel(driver->securityManager, - vm->def, migrated); + vm->def, + flags & VIR_QEMU_PROCESS_STOP_MIGRATED);
It's a bit odd that virSecurityManagerRestoreAllLabel uses an int instead of a bool, but that's independent of your patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4cb0a83..0309bfb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3281,6 +3281,7 @@ int qemuProcessStart(virConnectPtr conn, int i; char *nodeset = NULL; char *nodemask = NULL; + unsigned int stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL; /* Okay, these are just internal flags, * but doesn't hurt to check */ @@ -3631,6 +3632,8 @@ int qemuProcessStart(virConnectPtr conn, vm->def, stdin_path) < 0) goto cleanup; + 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 @@ -3770,7 +3773,7 @@ cleanup: VIR_FREE(nodemask); virCommandFree(cmd); VIR_FORCE_CLOSE(logfile); - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags); return -1; } @@ -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)) + virSecurityManagerRestoreAllLabel(driver->securityManager, + vm->def, + flags & VIR_QEMU_PROCESS_STOP_MIGRATED); virSecurityManagerReleaseLabel(driver->securityManager, vm->def); /* Clear out dynamically assigned labels */ diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index ce59215..d13778d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -61,7 +61,8 @@ int qemuProcessStart(virConnectPtr conn, unsigned int flags); typedef enum { - VIR_QEMU_PROCESS_STOP_MIGRATED = 1 << 0, + VIR_QEMU_PROCESS_STOP_MIGRATED = 1 << 0, + VIR_QEMU_PROCESS_STOP_NO_RELABEL = 1 << 1, } qemuProcessStopFlags; void qemuProcessStop(struct qemud_driver *driver, -- 1.7.8.5
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. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
participants (2)
-
Eric Blake -
Michal Privoznik