[PATCH 0/5] qemu: Fix chardev hotplug and deny <console/> live detach
While trying to make live detach of <console/> work, I've accumulated couple of patches. The most reasonable ones made it into this patch set as paches 1-4. And after nearly losing my sanity, I've decided it's not worth supporting live detach of <console/> since <serial/> works just fine. And that's what patch 5 does. If somebody thinks it's too harsh, I'm more than happy to review their patches that fix the problem. What problem? Well, for starters: https://bugzilla.redhat.com/show_bug.cgi?id=2156300#c4 https://bugzilla.redhat.com/show_bug.cgi?id=2156300#c8 Michal Prívozník (5): qemuDomainChrInsertPreAlloced: Fix adding implicit console qemuDomainChrRemove: Don't leak vmdef->consoles[0] qemuAssignDeviceChrAlias: Fix a crasher during <console/> hotplug qemuDomainRemoveChrDevice: Deal with qemuDomainChrRemove() failure qemu_hotplug: Deny live detach of <console/> src/qemu/qemu_alias.c | 1 + src/qemu/qemu_hotplug.c | 43 +++++++++++++++++++++++++++++------------ 2 files changed, 32 insertions(+), 12 deletions(-) -- 2.39.2
When hotpluging a <serial/> device, we might need to add a <console/> device with it (because of some crazy backcompat). Now, hotplugging is done in several phases. In one of them, qemuDomainChrPreInsert() allocates space for both devices, and then qemuDomainChrInsertPreAlloced() actually inserts the device into domain definition and sets up the <console/> device with it. Except, the condition that checks whether to create the aliased <console/> is wrong as it compares nconsoles against 0. Surprisingly, qemuDomainChrInsertPreAllocCleanup() doesn't suffer from the same error. Fixes: daf51be5f1b0f7b41c0813d43d6b66edfbe4f6d9 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3f45a48393..f517646c55 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1875,9 +1875,8 @@ qemuDomainChrInsertPreAlloced(virDomainDef *vmdef, virDomainChrDef *chr) { virDomainChrInsertPreAlloced(vmdef, chr); - if (vmdef->nserials == 1 && vmdef->nconsoles == 0 && + if (vmdef->nserials == 1 && vmdef->nconsoles == 1 && 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; -- 2.39.2
On a Wednesday in 2023, Michal Privoznik wrote:
When hotpluging a <serial/> device, we might need to add a <console/> device with it (because of some crazy backcompat). Now, hotplugging is done in several phases. In one of them, qemuDomainChrPreInsert() allocates space for both devices, and then qemuDomainChrInsertPreAlloced() actually inserts the device into domain definition and sets up the <console/> device with it. Except, the condition that checks whether to create the aliased <console/> is wrong as it compares nconsoles against 0. Surprisingly, qemuDomainChrInsertPreAllocCleanup() doesn't suffer from the same error.
Fixes: daf51be5f1b0f7b41c0813d43d6b66edfbe4f6d9 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When removing the compat console from domain defintion, removing it from the vmdef->consoles array is good, but not sufficient. The console definition might have been fully allocated (after daemon restarted and reloaded the status XML). Use virDomainChrDefFree() to free also the definition. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f517646c55..a6407f074b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1937,8 +1937,10 @@ qemuDomainChrRemove(virDomainDef *vmdef, return NULL; } - if (removeCompat) + if (removeCompat) { + virDomainChrDefFree(vmdef->consoles[0]); VIR_DELETE_ELEMENT(vmdef->consoles, 0, vmdef->nconsoles); + } return ret; } -- 2.39.2
For a running guest, a <serial/> device can be hotunplugged. This will then remove also aliased <console/>. Trying to hotplug a <console/> device then, libvirtd crashed because it dereferences def->consoles while there's none. Fixes: 42d53ac799a1d7f1414737caa4deb73871876992 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_alias.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 85db7fbfe3..161d30cf72 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -95,6 +95,7 @@ qemuAssignDeviceChrAlias(virDomainDef *def, if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL && def->os.type == VIR_DOMAIN_OSTYPE_HVM && + def->nconsoles && def->consoles[0] == chr && def->nserials && def->serials[0]->info.alias) { -- 2.39.2
When cleaning up after removed device, qemuDomainChrRemove() is called. But this may fail, in which case we successfully ignore the failure and virDomainChrDefFree() the device anyway. While it decreases our memory consumption, it's a bit too far, especially if the next step is 'virsh dumpxml'. Then our memory consumption decreases all the way down to zero as we crash. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a6407f074b..d9a102191f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4695,6 +4695,7 @@ qemuDomainRemoveChrDevice(virQEMUDriver *driver, virObjectEvent *event; g_autofree char *charAlias = NULL; qemuDomainObjPrivate *priv = vm->privateData; + virDomainChrDef *chrRemoved = NULL; int rc = 0; VIR_DEBUG("Removing character device %s from domain %p %s", @@ -4728,17 +4729,24 @@ qemuDomainRemoveChrDevice(virQEMUDriver *driver, if (qemuDomainNamespaceTeardownChardev(vm, chr) < 0) VIR_WARN("Unable to remove chr device from /dev"); - qemuDomainReleaseDeviceAddress(vm, &chr->info); - qemuDomainChrRemove(vm->def, chr); + if (!(chrRemoved = qemuDomainChrRemove(vm->def, chr))) { + /* At this point, we only have bad options. The device + * was successfully removed from QEMU, denied in CGropus, + * etc. and yet, we failed to remove it from domain + * definition. */ + VIR_WARN("Unable to remove chr device from domain definition"); + } else { + qemuDomainReleaseDeviceAddress(vm, &chrRemoved->info); - /* The caller does not emit the event, so we must do it here. Note - * that the event should be reported only after all backend - * teardown is completed. - */ - event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); + /* The caller does not emit the event, so we must do it here. Note + * that the event should be reported only after all backend + * teardown is completed. + */ + event = virDomainEventDeviceRemovedNewFromObj(vm, chrRemoved->info.alias); + virObjectEventStateQueue(driver->domainEventState, event); - virDomainChrDefFree(chr); + virDomainChrDefFree(chrRemoved); + } return 0; } -- 2.39.2
I've tried, then I've tried even harder, but still wasn't able to make sense of our console backcompat code in all its fine details. Since I value my sanity, let's just forbid hotunplug of <console/>, especially since detaching of corresponding <serial/> works. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d9a102191f..5072798cb7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5693,6 +5693,16 @@ qemuDomainDetachDeviceChr(virQEMUDriver *driver, goto cleanup; } + if (vmdef->os.type == VIR_DOMAIN_OSTYPE_HVM && + tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + (tmpChr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL || + tmpChr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE)) { + /* Raise this limitation once device removal works. */ + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("detaching of <console/> is unsupported. Try corresponding <serial/> instead")); + goto cleanup; + } + /* guestfwd channels are not really -device rather than * -netdev. We need to treat them slightly differently. */ guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && -- 2.39.2
On a Wednesday in 2023, Michal Privoznik wrote:
While trying to make live detach of <console/> work, I've accumulated couple of patches. The most reasonable ones made it into this patch set as paches 1-4. And after nearly losing my sanity, I've decided it's not worth supporting live detach of <console/> since <serial/> works just fine. And that's what patch 5 does. If somebody thinks it's too harsh, I'm more than happy to review their patches that fix the problem.
What problem? Well, for starters:
https://bugzilla.redhat.com/show_bug.cgi?id=2156300#c4 https://bugzilla.redhat.com/show_bug.cgi?id=2156300#c8
Michal Prívozník (5): qemuDomainChrInsertPreAlloced: Fix adding implicit console qemuDomainChrRemove: Don't leak vmdef->consoles[0] qemuAssignDeviceChrAlias: Fix a crasher during <console/> hotplug qemuDomainRemoveChrDevice: Deal with qemuDomainChrRemove() failure qemu_hotplug: Deny live detach of <console/>
src/qemu/qemu_alias.c | 1 + src/qemu/qemu_hotplug.c | 43 +++++++++++++++++++++++++++++------------ 2 files changed, 32 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik