[libvirt] [PATCH 0/2] Deny live device alias change
*** BLURB HERE *** Michal Privoznik (2): qemuDomainUpdateDeviceFlags: Parse device as live if needed conf: Forbid device alias change on device-update src/conf/domain_conf.c | 12 +++++++++++- src/conf/domain_conf.h | 3 ++- src/lxc/lxc_driver.c | 6 +++--- src/qemu/qemu_driver.c | 28 ++++++++++++++++------------ 4 files changed, 32 insertions(+), 17 deletions(-) -- 2.16.4
When updating device it's worth parsing live info too as users might want to update it as well. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f0fb806fcd..ab5cc6ea31 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8608,7 +8608,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, int ret = -1; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = 0; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -8630,15 +8630,19 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, parse_flags); if (dev == NULL) goto endjob; - if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) - goto endjob; - if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { /* If we are affecting both CONFIG and LIVE -- 2.16.4
On 06/12/2018 11:04 AM, Michal Privoznik wrote:
When updating device it's worth parsing live info too as users might want to update it as well.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
So this follows what DetachLiveAndConfig has done with altering parse_flags to add the inactive mainly because it "makes sense" to attempt to match whatever "live/active" device may exist to the fullest possible extent including fields that wouldn't normally be parsed for an inactive guest. Took a bit to come to that conclusion ;-) because the commit message above didn't give me that! Reviewed-by: John Ferlan <jferlan@redhat.com> John
https://bugzilla.redhat.com/show_bug.cgi?id=1585108 When updating a live device users might pass different alias than the one the device has. Currently, this is silently ignored which goes against our behaviour for other parts of the device where we explicitly allow only certain changes and error out loudly on anything else. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 12 +++++++++++- src/conf/domain_conf.h | 3 ++- src/lxc/lxc_driver.c | 6 +++--- src/qemu/qemu_driver.c | 16 ++++++++-------- 4 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85f07af46e..91cac75c0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28195,7 +28195,8 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED, int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev, - virDomainDeviceDefPtr oldDev) + virDomainDeviceDefPtr oldDev, + bool live) { virDomainCompatibleDeviceData data = { .newInfo = virDomainDeviceGetInfo(dev), @@ -28205,6 +28206,15 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, if (oldDev) data.oldInfo = virDomainDeviceGetInfo(oldDev); + if (live && + ((!!data.newInfo != !!data.oldInfo) || + (data.newInfo && data.oldInfo && + STRNEQ(data.newInfo->alias, data.oldInfo->alias)))) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("changing device alias is not allowed")); + return -1; + } + if (!virDomainDefHasUSB(def) && def->os.type != VIR_DOMAIN_OSTYPE_EXE && virDomainDeviceIsUSB(dev)) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ea8ddb2b39..c03028efb9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3107,7 +3107,8 @@ typedef enum { int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev, - virDomainDeviceDefPtr oldDev); + virDomainDeviceDefPtr oldDev, + bool live); void virDomainRNGDefFree(virDomainRNGDefPtr def); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index cfb431488d..fb7c8135e0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3549,7 +3549,7 @@ lxcDomainUpdateDeviceConfig(virDomainDefPtr vmdef, goto cleanup; oldDev.data.net = vmdef->nets[idx]; - if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev, false) < 0) return -1; virDomainNetDefFree(vmdef->nets[idx]); @@ -4785,7 +4785,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, if (!vmdef) goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev, NULL) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, NULL, false) < 0) goto endjob; if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0) @@ -4793,7 +4793,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL, true) < 0) goto endjob; if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) < 0) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ab5cc6ea31..2d4e777b47 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7860,7 +7860,7 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm, } oldDev.data.disk = orig_disk; - if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev, true) < 0) goto cleanup; if (!qemuDomainDiskChangeSupported(disk, orig_disk)) @@ -7918,7 +7918,7 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_GRAPHICS: if ((idx = qemuDomainFindGraphicsIndex(vm->def, dev->data.graphics)) >= 0) { oldDev.data.graphics = vm->def->graphics[idx]; - if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev, true) < 0) return -1; } @@ -7928,7 +7928,7 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_NET: if ((idx = virDomainNetFindIdx(vm->def, dev->data.net)) >= 0) { oldDev.data.net = vm->def->nets[idx]; - if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev, true) < 0) return -1; } @@ -8383,7 +8383,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, } oldDev.data.disk = vmdef->disks[pos]; - if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev, false) < 0) return -1; virDomainDiskDefFree(vmdef->disks[pos]); @@ -8402,7 +8402,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, } oldDev.data.graphics = vmdef->graphics[pos]; - if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev, false) < 0) return -1; virDomainGraphicsDefFree(vmdef->graphics[pos]); @@ -8416,7 +8416,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, return -1; oldDev.data.net = vmdef->nets[pos]; - if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev, false) < 0) return -1; virDomainNetDefFree(vmdef->nets[pos]); @@ -8507,7 +8507,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, if (!vmdef) goto cleanup; - if (virDomainDefCompatibleDevice(vmdef, dev, NULL) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, NULL, false) < 0) goto cleanup; if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps, parse_flags, @@ -8516,7 +8516,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL, true) < 0) goto cleanup; if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0) -- 2.16.4
On 06/12/2018 11:04 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1585108
When updating a live device users might pass different alias than the one the device has. Currently, this is silently ignored which goes against our behaviour for other parts of the device where we explicitly allow only certain changes and error out loudly on anything else.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 12 +++++++++++- src/conf/domain_conf.h | 3 ++- src/lxc/lxc_driver.c | 6 +++--- src/qemu/qemu_driver.c | 16 ++++++++-------- 4 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85f07af46e..91cac75c0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28195,7 +28195,8 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED, int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev, - virDomainDeviceDefPtr oldDev) + virDomainDeviceDefPtr oldDev, + bool live) { virDomainCompatibleDeviceData data = { .newInfo = virDomainDeviceGetInfo(dev), @@ -28205,6 +28206,15 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, if (oldDev) data.oldInfo = virDomainDeviceGetInfo(oldDev);
+ if (live && + ((!!data.newInfo != !!data.oldInfo) || + (data.newInfo && data.oldInfo && + STRNEQ(data.newInfo->alias, data.oldInfo->alias)))) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("changing device alias is not allowed")); + return -1; + } +
Does this work for attach-device from the bz? I just tried it and it doesn't - probably because oldInfo would be NULL for attach. Additionally, if the update/detach doesn't include an alias, then would this fail too? By comparison qemuDomainDiskChangeSupported only cares that the updated disk definition has an alias before making check and of course that would now be duplicitous to this. Then there's qemuDomainChangeNet which at first I didn't understand why it wasn't throwing up since it does check the alias. Then I noticed something from the case, for the updated interface XML - if you include the <address> from the live guest, but change the "<alias>", then you get the error. However, if you remove the <address> from the update XML things then succeed. That's because of this gem in the code: if (!virDomainDeviceAddressIsValid(&newdev->info, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) { goto cleanup; } where virDomainDeviceInfoCopy essentially *overwrites* the provided/changed newdev->info.alias. So I think if you fix this part of the code, then you don't have the need for the rest of this patch. Yes, that does possibly leave the graphics devices as not checking alias updates, but that would seemingly be fixable separately if it's considered a problem or not checked as I didn't follow all the graphics update code. John NB: Another inconsistency in the bz is that the add interface added uses target dev='vnet0', but the live guest shows target dev='vnet12'. Then the updated xml doesn't change target dev='vnet0', but does change the alias and it works? For me it failed with: error: Operation not supported: cannot modify network device tap name until I changed it to match the live xml target dev... [...]
On 06/15/2018 02:38 PM, John Ferlan wrote:
On 06/12/2018 11:04 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1585108
When updating a live device users might pass different alias than the one the device has. Currently, this is silently ignored which goes against our behaviour for other parts of the device where we explicitly allow only certain changes and error out loudly on anything else.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 12 +++++++++++- src/conf/domain_conf.h | 3 ++- src/lxc/lxc_driver.c | 6 +++--- src/qemu/qemu_driver.c | 16 ++++++++-------- 4 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85f07af46e..91cac75c0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28195,7 +28195,8 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED, int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev, - virDomainDeviceDefPtr oldDev) + virDomainDeviceDefPtr oldDev, + bool live) { virDomainCompatibleDeviceData data = { .newInfo = virDomainDeviceGetInfo(dev), @@ -28205,6 +28206,15 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, if (oldDev) data.oldInfo = virDomainDeviceGetInfo(oldDev);
+ if (live && + ((!!data.newInfo != !!data.oldInfo) || + (data.newInfo && data.oldInfo && + STRNEQ(data.newInfo->alias, data.oldInfo->alias)))) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("changing device alias is not allowed")); + return -1; + } +
Does this work for attach-device from the bz? I just tried it and it doesn't - probably because oldInfo would be NULL for attach.
Huh, okay. I guess I need to rename the argument to say "checkAlias" and pass false from everywhere but qemuDomainUpdateDeviceLive(). Would you agree with this solution?
Additionally, if the update/detach doesn't include an alias, then would this fail too?
That's the idea. In general, one can argue that when detaching a device, only partial device XML could be parsed (the minimum needed to uniquely identify the device). But with update device the semantic is changed. Not only we use the XML to find the device we also use it to perform changes: what's new is set, what's left out is unset. For instance, imagine an <interface/> with QoS set. Then, call to update device with the very same <interface/> with QoS left out must be treated as "user wants QoS to be removed". Or vice versa. Because of this reason, leaving out <alias/> is viewed as "unset alias" (which of course is not possible for live XMLs, but is possible for inactive XMLs). Similarly, providing different <alias/> is viewed as "change alias".
By comparison qemuDomainDiskChangeSupported only cares that the updated disk definition has an alias before making check and of course that would now be duplicitous to this.
Then there's qemuDomainChangeNet which at first I didn't understand why it wasn't throwing up since it does check the alias. Then I noticed something from the case, for the updated interface XML - if you include the <address> from the live guest, but change the "<alias>", then you get the error. However, if you remove the <address> from the update XML things then succeed. That's because of this gem in the code:
if (!virDomainDeviceAddressIsValid(&newdev->info,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) { goto cleanup; }
where virDomainDeviceInfoCopy essentially *overwrites* the provided/changed newdev->info.alias.
So I think if you fix this part of the code, then you don't have the need for the rest of this patch. Yes, that does possibly leave the graphics devices as not checking alias updates, but that would seemingly be fixable separately if it's considered a problem or not checked as I didn't follow all the graphics update code.
Yep, this definitely deserves fixing (actually, the lines should be removed, not providing <address/> should be viewed as request for its removal which is of course impossible). But I still think we need one place where we would check for the alias change. Michal
participants (2)
-
John Ferlan -
Michal Privoznik