[libvirt] [PATCH 0/3] Fix libvirtd crash when starting two managedsave operations on a single domain
We had an unfortunate code path where when you'd start two managedsave operations simultaneously libvirtd would crash. Fix it by adding a VM liveness check. Peter Krempa (3): qemu: managedsave: Check that VM is alive after entering async job security: selinux: Set saved state label only if it is available qemu: migration: Check domain live state after exitting the monitor src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_migration.c | 19 +++++++++++++++++-- src/security/security_selinux.c | 2 +- 3 files changed, 24 insertions(+), 3 deletions(-) -- 2.0.2
Saving a shutoff VM doesn't make sense (and has suboptimal effects on stability of the libvirt daemon). Check that the domain is alive after entering the save async job. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1129207 --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2c3f179..b6219ba 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3096,6 +3096,12 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, if (qemuDomainObjBeginAsyncJob(driver, vm, QEMU_ASYNC_JOB_SAVE) < 0) goto cleanup; + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto endjob; + } + memset(&priv->job.info, 0, sizeof(priv->job.info)); priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED; -- 2.0.2
On 08/12/2014 03:44 PM, Peter Krempa wrote:
Saving a shutoff VM doesn't make sense (and has suboptimal effects on stability of the libvirt daemon). Check that the domain is alive after
Saying it 'crashes' is more readable IMHO.
entering the save async job.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1129207 --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
ACK
Check that secdef->imagelabel exists before blindly applying it. --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c078cab..cf59d6c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1962,7 +1962,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || !secdef->relabel) + if (!secdef || !secdef->imagelabel || !secdef->relabel) return 0; return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel); -- 2.0.2
On 08/12/2014 03:44 PM, Peter Krempa wrote:
Check that secdef->imagelabel exists before blindly applying it. --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c078cab..cf59d6c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1962,7 +1962,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef;
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || !secdef->relabel) + if (!secdef || !secdef->imagelabel || !secdef->relabel) return 0;
return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel);
I think this is just covers up incorrect usage of this function on shutoff domains, because if a running domain has a selinux label, it should have the imagelabel generated. Jan
On 08/12/14 17:17, Ján Tomko wrote:
On 08/12/2014 03:44 PM, Peter Krempa wrote:
Check that secdef->imagelabel exists before blindly applying it. --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c078cab..cf59d6c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1962,7 +1962,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef;
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || !secdef->relabel) + if (!secdef || !secdef->imagelabel || !secdef->relabel) return 0;
return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel);
I think this is just covers up incorrect usage of this function on shutoff domains, because if a running domain has a selinux label, it should have the imagelabel generated.
Fair enough. I'll drop this one.
In qemuMigrationToFile we enter the monitor multiple times and don't check if the VM is still alive after returning form the monitor. Add the checks to skip pieces of code in case the VM crashes while saving it's state. --- src/qemu/qemu_migration.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 767d840..50a1eab 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4731,6 +4731,13 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjExitMonitor(driver, vm); } + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + /* nothing to tear down */ + return -1; + } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && (!compressor || pipe(pipeFD) == 0)) { /* All right! We can use fd migration, which means that qemu @@ -4818,6 +4825,12 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, } qemuDomainObjExitMonitor(driver, vm); + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + if (rc < 0) goto cleanup; @@ -4827,7 +4840,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, if (rc == -2) { orig_err = virSaveLastError(); virCommandAbort(cmd); - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { + if (virDomainObjIsActive(vm) && + qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { qemuMonitorMigrateCancel(priv->mon); qemuDomainObjExitMonitor(driver, vm); } @@ -4845,7 +4859,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, orig_err = virSaveLastError(); /* Restore max migration bandwidth */ - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { + if (virDomainObjIsActive(vm) && + qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { qemuMonitorSetMigrationSpeed(priv->mon, saveMigBandwidth); priv->migMaxBandwidth = saveMigBandwidth; qemuDomainObjExitMonitor(driver, vm); -- 2.0.2
On 08/12/2014 03:44 PM, Peter Krempa wrote:
In qemuMigrationToFile we enter the monitor multiple times and don't check if the VM is still alive after returning form the monitor. Add the checks to skip pieces of code in case the VM crashes while saving it's state. --- src/qemu/qemu_migration.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
ACK Jan
On 08/12/14 17:17, Ján Tomko wrote:
On 08/12/2014 03:44 PM, Peter Krempa wrote:
In qemuMigrationToFile we enter the monitor multiple times and don't check if the VM is still alive after returning form the monitor. Add the checks to skip pieces of code in case the VM crashes while saving it's state. --- src/qemu/qemu_migration.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
ACK
I've tweaked the commit message in 1/3 and pushed 1/3 and 3/3. Thanks. Peter
On 08/12/2014 07:44 AM, Peter Krempa wrote: s/exitting/exiting/ in the title
In qemuMigrationToFile we enter the monitor multiple times and don't check if the VM is still alive after returning form the monitor. Add the checks to skip pieces of code in case the VM crashes while saving it's
s/it's/its/ ("it's" is only correct if you can replace it with "it is")
state.
Oh well, I see this was already pushed; so we'll live with the typos :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa