3:28 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=638285 - when migrating
a guest, it was very easy to provoke a race where an application
could query block information on a VM that had just been migrated
away. Any time qemu code obtains a job lock, it must also check
that the VM was not taken down in the time where it was waiting
for the lock.
* src/qemu/qemu_driver.c (qemudDomainSetMemory)
(qemudDomainGetInfo, qemuDomainGetBlockInfo): Check that vm still
exists after obtaining job lock, before starting monitor action.
---
v2: incorporate danpb's suggestions, to minimize virDomainObjIsActive
calls and avoid use of goto for mid-function labels.
src/qemu/qemu_driver.c | 60 ++++++++++++++++++++++++-----------------------
1 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b119ca1..f1f8cdf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -466,7 +466,8 @@ static int ATTRIBUTE_RETURN_CHECK qemuDomainObjEndJob(virDomainObjPtr
obj)
* obj must be locked before calling, qemud_driver must be unlocked
*
* To be called immediately before any QEMU monitor API call
- * Must have alrady called qemuDomainObjBeginJob().
+ * Must have already called qemuDomainObjBeginJob(), and checked
+ * that the VM is still active.
*
* To be followed with qemuDomainObjExitMonitor() once complete
*/
@@ -507,7 +508,7 @@ static void qemuDomainObjExitMonitor(virDomainObjPtr obj)
* obj must be locked before calling, qemud_driver must be locked
*
* To be called immediately before any QEMU monitor API call
- * Must have alrady called qemuDomainObjBeginJob().
+ * Must have already called qemuDomainObjBeginJob().
*
* To be followed with qemuDomainObjExitMonitorWithDriver() once complete
*/
@@ -525,7 +526,7 @@ static void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver
*driver, vir
/* obj must NOT be locked before calling, qemud_driver must be unlocked,
* and will be locked after returning
*
- * Should be paired with an earlier qemuDomainObjEnterMonitor() call
+ * Should be paired with an earlier qemuDomainObjEnterMonitorWithDriver() call
*/
static void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver,
virDomainObjPtr obj)
{
@@ -5077,12 +5078,6 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long
newmem) {
goto cleanup;
}
- if (!virDomainObjIsActive(vm)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
- goto cleanup;
- }
-
if (newmem > vm->def->mem.max_balloon) {
qemuReportError(VIR_ERR_INVALID_ARG,
"%s", _("cannot set memory higher than max
memory"));
@@ -5092,6 +5087,12 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long
newmem) {
if (qemuDomainObjBeginJob(vm) < 0)
goto cleanup;
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
+
priv = vm->privateData;
qemuDomainObjEnterMonitor(vm);
r = qemuMonitorSetBalloon(priv->mon, newmem);
@@ -5158,26 +5159,25 @@ static int qemudDomainGetInfo(virDomainPtr dom,
} else if (!priv->jobActive) {
if (qemuDomainObjBeginJob(vm) < 0)
goto cleanup;
-
- qemuDomainObjEnterMonitor(vm);
- err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
- qemuDomainObjExitMonitor(vm);
- if (err < 0) {
- if (qemuDomainObjEndJob(vm) == 0)
- vm = NULL;
+ if (!virDomainObjIsActive(vm))
+ err = 0;
+ else {
+ qemuDomainObjEnterMonitor(vm);
+ err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
+ qemuDomainObjExitMonitor(vm);
+ }
+ if (qemuDomainObjEndJob(vm) == 0) {
+ vm = NULL;
goto cleanup;
}
+ if (err < 0)
+ goto cleanup;
if (err == 0)
/* Balloon not supported, so maxmem is always the allocation */
info->memory = vm->def->mem.max_balloon;
else
info->memory = balloon;
-
- if (qemuDomainObjEndJob(vm) == 0) {
- vm = NULL;
- goto cleanup;
- }
} else {
info->memory = vm->def->mem.cur_balloon;
}
@@ -10510,19 +10510,21 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
/* ..but if guest is running & not using raw
disk format and on a block device, then query
highest allocated extent from QEMU */
- if (virDomainObjIsActive(vm) &&
- disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
+ if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
format != VIR_STORAGE_FILE_RAW &&
S_ISBLK(sb.st_mode)) {
qemuDomainObjPrivatePtr priv = vm->privateData;
if (qemuDomainObjBeginJob(vm) < 0)
goto cleanup;
-
- qemuDomainObjEnterMonitor(vm);
- ret = qemuMonitorGetBlockExtent(priv->mon,
- disk->info.alias,
- &info->allocation);
- qemuDomainObjExitMonitor(vm);
+ if (!virDomainObjIsActive(vm))
+ ret = 0;
+ else {
+ qemuDomainObjEnterMonitor(vm);
+ ret = qemuMonitorGetBlockExtent(priv->mon,
+ disk->info.alias,
+ &info->allocation);
+ qemuDomainObjExitMonitor(vm);
+ }
if (qemuDomainObjEndJob(vm) == 0)
vm = NULL;
--
1.7.2.3