[libvirt] [PATCHv2 0/2] Really fix the crash on qemu crash on chardev hotplug
An alternative solution that only adds the chardev to the definition after the command succeeded. Ján Tomko (2): Split qemuDomainChrInsert into two parts hotplug: only add a chardev to vmdef after monitor call src/conf/domain_conf.c | 18 ++++++++-- src/conf/domain_conf.h | 7 ++-- src/libvirt_private.syms | 3 +- src/qemu/qemu_hotplug.c | 88 +++++++++++++++++++++++++++++++----------------- 4 files changed, 79 insertions(+), 37 deletions(-) -- 2.0.5
Do the allocation first, then add the actual device. The second part should never fail. This is good for live hotplug where we don't want to remove the device on OOM after the monitor command succeeded. The only change in behavior is that on failure, the vmdef->consoles array is freed, not just the first console. --- src/conf/domain_conf.c | 18 +++++++++++++--- src/conf/domain_conf.h | 7 +++++-- src/libvirt_private.syms | 3 ++- src/qemu/qemu_hotplug.c | 54 +++++++++++++++++++++++++++++++++++++++++------- 4 files changed, 68 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10dbabd..3c6b77d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12027,15 +12027,27 @@ virDomainChrGetDomainPtrs(const virDomainDef *vmdef, int -virDomainChrInsert(virDomainDefPtr vmdef, - virDomainChrDefPtr chr) +virDomainChrPreAlloc(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrDefPtr **arrPtr = NULL; + size_t *cntPtr = NULL; + + virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); + + return VIR_REALLOC_N(*arrPtr, *cntPtr + 1); +} + +void +virDomainChrInsertPreAlloced(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) { virDomainChrDefPtr **arrPtr = NULL; size_t *cntPtr = NULL; virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); - return VIR_APPEND_ELEMENT(*arrPtr, *cntPtr, chr); + ignore_value(VIR_APPEND_ELEMENT_INPLACE(*arrPtr, *cntPtr, chr)); } virDomainChrDefPtr diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 563c18b..93f2314 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2652,8 +2652,11 @@ bool virDomainChrEquals(virDomainChrDefPtr src, virDomainChrDefPtr tgt); int -virDomainChrInsert(virDomainDefPtr vmdef, - virDomainChrDefPtr chr); +virDomainChrPreAlloc(virDomainDefPtr vmdef, + virDomainChrDefPtr chr); +void +virDomainChrInsertPreAlloced(virDomainDefPtr vmdef, + virDomainChrDefPtr chr); virDomainChrDefPtr virDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 75a6d83..bd7870f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -153,7 +153,8 @@ virDomainChrDefNew; virDomainChrEquals; virDomainChrFind; virDomainChrGetDomainPtrs; -virDomainChrInsert; +virDomainChrInsertPreAlloced; +virDomainChrPreAlloc; virDomainChrRemove; virDomainChrSerialTargetTypeFromString; virDomainChrSerialTargetTypeToString; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1dbcc5d..2ea30f5 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1403,9 +1403,9 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, } -int -qemuDomainChrInsert(virDomainDefPtr vmdef, - virDomainChrDefPtr chr) +static int +qemuDomainChrPreInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) { if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) { @@ -1420,25 +1420,63 @@ qemuDomainChrInsert(virDomainDefPtr vmdef, return -1; } - if (virDomainChrInsert(vmdef, chr) < 0) + if (virDomainChrPreAlloc(vmdef, chr) < 0) return -1; /* Due to some crazy backcompat stuff, the first serial device is an alias * to the first console too. If this is the case, the definition must be * duplicated as first console device. */ - if (vmdef->nserials == 1 && vmdef->nconsoles == 0) { - if ((!vmdef->consoles && VIR_ALLOC(vmdef->consoles) < 0) || - VIR_ALLOC(vmdef->consoles[0]) < 0) { - virDomainChrRemove(vmdef, chr); + if (vmdef->nserials == 0 && vmdef->nconsoles == 0 && + chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL) { + if (!vmdef->consoles && VIR_ALLOC(vmdef->consoles) < 0) + return -1; + + if (VIR_ALLOC(vmdef->consoles[0]) < 0) { + VIR_FREE(vmdef->consoles); return -1; } + vmdef->nconsoles++; + } + return 0; +} + +static void +qemuDomainChrInsertPreAlloced(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrInsertPreAlloced(vmdef, chr); + if (vmdef->nserials == 1 && vmdef->nconsoles == 0 && + chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL) { vmdef->nconsoles = 1; /* Create an console alias for the serial port */ vmdef->consoles[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; vmdef->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL; } +} + +static void +qemuDomainChrInsertPreAllocCleanup(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + /* Remove the stub console added by qemuDomainChrPreInsert */ + if (vmdef->nserials == 0 && vmdef->nconsoles == 1 && + chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL) { + VIR_FREE(vmdef->consoles[0]); + VIR_FREE(vmdef->consoles); + vmdef->nconsoles = 0; + } +} +int +qemuDomainChrInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + if (qemuDomainChrPreInsert(vmdef, chr) < 0) { + qemuDomainChrInsertPreAllocCleanup(vmdef, chr); + return -1; + } + qemuDomainChrInsertPreAlloced(vmdef, chr); return 0; } -- 2.0.5
https://bugzilla.redhat.com/show_bug.cgi?id=1161024 This way the device is in vmdef only if ret = 0 and the caller (qemuDomainAttachDeviceFlags) does not free it. Otherwise it might get double freed by qemuProcessStop and qemuDomainAttachDeviceFlags if the domain crashed in monitor after we've added it to vm->def. --- qemuDomainChrInsertPreAllocCleanup is always called, not just when qemuDomainChrPreInsert was called before. But unless I missed something, the configuration where nserials == 0, nconsoles == 1 should not happen after qemu's PostParse callback. src/qemu/qemu_hotplug.c | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2ea30f5..033b281 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1523,59 +1523,47 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; char *devstr = NULL; char *charAlias = NULL; - bool need_remove = false; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("qemu does not support -device")); - return ret; + goto cleanup; } if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) - return ret; + goto cleanup; if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) - return ret; + goto cleanup; if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) goto cleanup; - if (qemuDomainChrInsert(vmdef, chr) < 0) + if (qemuDomainChrPreInsert(vmdef, chr) < 0) goto cleanup; - need_remove = true; qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto audit; } if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { /* detach associated chardev on error */ qemuMonitorDetachCharDev(priv->mon, charAlias); - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto audit; } - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto audit; + qemuDomainChrInsertPreAlloced(vm->def, chr); ret = 0; audit: virDomainAuditChardev(vm, NULL, chr, "attach", ret == 0); cleanup: - if (ret < 0 && need_remove) - qemuDomainChrRemove(vmdef, chr); + if (ret < 0 && virDomainObjIsActive(vm)) + qemuDomainChrInsertPreAllocCleanup(vm->def, chr); VIR_FREE(charAlias); VIR_FREE(devstr); return ret; -- 2.0.5
On 28.01.2015 10:14, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
This way the device is in vmdef only if ret = 0 and the caller (qemuDomainAttachDeviceFlags) does not free it.
Otherwise it might get double freed by qemuProcessStop and qemuDomainAttachDeviceFlags if the domain crashed in monitor after we've added it to vm->def. --- qemuDomainChrInsertPreAllocCleanup is always called, not just when qemuDomainChrPreInsert was called before. But unless I missed something, the configuration where nserials == 0, nconsoles == 1 should not happen after qemu's PostParse callback.
src/qemu/qemu_hotplug.c | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2ea30f5..033b281 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1523,59 +1523,47 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; char *devstr = NULL; char *charAlias = NULL; - bool need_remove = false;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("qemu does not support -device")); - return ret; + goto cleanup; }
if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) - return ret; + goto cleanup;
if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) - return ret; + goto cleanup;
if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) goto cleanup;
- if (qemuDomainChrInsert(vmdef, chr) < 0) + if (qemuDomainChrPreInsert(vmdef, chr) < 0) goto cleanup; - need_remove = true;
qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto audit; }
if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { /* detach associated chardev on error */ qemuMonitorDetachCharDev(priv->mon, charAlias); - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto audit; } - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto audit;
+ qemuDomainChrInsertPreAlloced(vm->def, chr); ret = 0; audit: virDomainAuditChardev(vm, NULL, chr, "attach", ret == 0); cleanup: - if (ret < 0 && need_remove) - qemuDomainChrRemove(vmdef, chr); + if (ret < 0 && virDomainObjIsActive(vm)) + qemuDomainChrInsertPreAllocCleanup(vm->def, chr);
It took me a while to see if this is safe. We can jump here even if vm->def hasn't been touched at all, e.g. if qemu is missing the DEVICE capability. However, if that's the case, there's currently no way for vm->def to contain one console but no serial line.
VIR_FREE(charAlias); VIR_FREE(devstr); return ret;
Michal
On 28.01.2015 10:14, Ján Tomko wrote:
An alternative solution that only adds the chardev to the definition after the command succeeded.
Ján Tomko (2): Split qemuDomainChrInsert into two parts hotplug: only add a chardev to vmdef after monitor call
src/conf/domain_conf.c | 18 ++++++++-- src/conf/domain_conf.h | 7 ++-- src/libvirt_private.syms | 3 +- src/qemu/qemu_hotplug.c | 88 +++++++++++++++++++++++++++++++----------------- 4 files changed, 79 insertions(+), 37 deletions(-)
ACK to both Michal
participants (2)
-
Ján Tomko -
Michal Privoznik