[libvirt] [PATCH 0/3] Couple of user alias fixes
The deeper into the rabbit hole we go the more I think it was a mistake to let users specify device aliases. Michal Privoznik (3): virDomainDeviceValidateAliasForHotplug: Use correct domain defintion conf: Check for user aliases duplicates only qemu: Forbid user aliases for USB controllers src/conf/domain_conf.c | 6 +++--- src/qemu/qemu_domain.c | 20 +++++++++++++++++++- tests/qemuxml2argvdata/user-aliases-usb.xml | 19 +++++++++++++++++++ tests/qemuxml2argvdata/user-aliases.xml | 1 - tests/qemuxml2argvtest.c | 1 + 5 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/user-aliases-usb.xml -- 2.16.1
https://bugzilla.redhat.com/show_bug.cgi?id=1553075 For some weird reason this function is getting live and persistent def for domain but then accesses vm->def and vm->newDef directly. This is rather unsafe as we can be accessing NULL pointer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70b19311b..b98b1ca42 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5679,11 +5679,11 @@ virDomainDeviceValidateAliasForHotplug(virDomainObjPtr vm, return -1; if (persDef && - virDomainDeviceValidateAliasImpl(vm->def, dev) < 0) + virDomainDeviceValidateAliasImpl(persDef, dev) < 0) return -1; if (liveDef && - virDomainDeviceValidateAliasImpl(vm->newDef, dev) < 0) + virDomainDeviceValidateAliasImpl(liveDef, dev) < 0) return -1; return 0; -- 2.16.1
On Fri, Mar 09, 2018 at 12:56:11 +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1553075
For some weird reason this function is getting live and persistent def for domain but then accesses vm->def and vm->newDef directly. This is rather unsafe as we can be accessing NULL pointer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
https://bugzilla.redhat.com/show_bug.cgi?id=1553162 When validating a device XML config we check if user provided alias is unique. We do this by maintaining a hash table of device aliases as we iterated over all devices defined for the domain. However, it may happen that what appears as two devices in domain XML is in fact just one interface in hypervisor. For instance in qemu driver this is true for uhci/ehci controllers. In that case an error is reported even though it is not actually an error. At any rate, we can assume libvirt generated aliases to be unique and thus really check user provided ones only. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b98b1ca42..04a6ee77a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5573,7 +5573,7 @@ virDomainDeviceDefValidateAliasesIterator(virDomainDefPtr def, struct virDomainDefValidateAliasesData *data = opaque; const char *alias = info->alias; - if (!alias) + if (!alias || !virDomainDeviceAliasIsUserAlias(alias)) return 0; /* Some crazy backcompat for consoles. */ -- 2.16.1
On Fri, Mar 09, 2018 at 12:56:12 +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1553162
When validating a device XML config we check if user provided alias is unique. We do this by maintaining a hash table of device aliases as we iterated over all devices defined for the domain. However, it may happen that what appears as two devices in domain XML is in fact just one interface in hypervisor. For instance in qemu driver this is true for uhci/ehci controllers. In that case an error is reported even though it is not actually an error. At any rate, we can assume libvirt generated aliases to be unique and thus really check user provided ones only.
So I find some parts of this description slightly misleading. I agree that we should only validate that the user-provided aliases are unique, since libvirt should do the proper thing otherwise and the user-aliases are in it's own namespace. I think though that the hint to the usb controller requiring multiple devices from the libvirt view map to a single device in qemu, or any other reason why two libvirt entries should have the same alias is not future-proof enough. With this commit you make it possible to use it with the default alias, but it still might be possible to use with a user alias. ACK to this patch, since I think it's the correct thing to do, but please delete the irrelevant blurb about uhci/ehci in qemu.
https://bugzilla.redhat.com/show_bug.cgi?id=1552127 These are bit different than other devices. Their alias also specify the name of the bus. For instance, if there are these 'joined' USB devices: <controller type='usb' index='0' model='ich9-ehci1'> <alias name='ua-myUSB'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/> </controller> <controller type='usb' index='0' model='ich9-uhci1'> <master startport='0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/> </controller> which translates to cmd line as: -device ich9-usb-ehci1,id=ua-myUSB,bus=pci.0,addr=0x4.0x7 -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4 The problem is that UHCI is still trying to serve 'usb.0' bus. Rather than trying to come up with some complicated algorithm to make everything work, lets forbid user aliases for USB controllers. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 20 +++++++++++++++++++- tests/qemuxml2argvdata/user-aliases-usb.xml | 19 +++++++++++++++++++ tests/qemuxml2argvdata/user-aliases.xml | 1 - tests/qemuxml2argvtest.c | 1 + 4 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/user-aliases-usb.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b55013de6..67c145493 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4565,6 +4565,21 @@ qemuDomainDeviceDefValidateControllerSATA(const virDomainControllerDef *controll } +static int +qemuDomainDeviceDefValidateControllerUSB(const virDomainControllerDef *controller) +{ + if (controller->info.alias && + virDomainDeviceAliasIsUserAlias(controller->info.alias)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Providing user alias to USB " + "controllers is not supported yet")); + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, const virDomainDef *def, @@ -4602,10 +4617,13 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, qemuCaps); break; + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + ret = qemuDomainDeviceDefValidateControllerUSB(controller); + break; + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: case VIR_DOMAIN_CONTROLLER_TYPE_CCID: - case VIR_DOMAIN_CONTROLLER_TYPE_USB: case VIR_DOMAIN_CONTROLLER_TYPE_LAST: break; } diff --git a/tests/qemuxml2argvdata/user-aliases-usb.xml b/tests/qemuxml2argvdata/user-aliases-usb.xml new file mode 100644 index 000000000..6dade4572 --- /dev/null +++ b/tests/qemuxml2argvdata/user-aliases-usb.xml @@ -0,0 +1,19 @@ +<domain type='kvm'> + <name>gentoo</name> + <uuid>a75aca4b-a02f-2bcb-4a91-c93cd848c34b</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-1.4'>hvm</type> + <boot dev='hd'/> + <boot dev='cdrom'/> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <alias name='ua-SomeWeirdController'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/user-aliases.xml b/tests/qemuxml2argvdata/user-aliases.xml index 52132a82d..6f1fed636 100644 --- a/tests/qemuxml2argvdata/user-aliases.xml +++ b/tests/qemuxml2argvdata/user-aliases.xml @@ -71,7 +71,6 @@ <address type='drive' controller='0' bus='1' target='0' unit='0'/> </disk> <controller type='usb' index='0'> - <alias name='ua-SomeWeirdController'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> <controller type='pci' index='0' model='pci-root'> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 688846b9b..5758811b8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2965,6 +2965,7 @@ mymain(void) QEMU_CAPS_DEVICE_ISA_SERIAL, QEMU_CAPS_HDA_DUPLEX); DO_TEST("user-aliases2", QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_ICH9_AHCI); + DO_TEST_PARSE_ERROR("user-aliases-usb", QEMU_CAPS_KVM); if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); -- 2.16.1
On Fri, Mar 09, 2018 at 12:56:13 +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1552127
These are bit different than other devices. Their alias also specify the name of the bus. For instance, if there are these 'joined' USB devices:
<controller type='usb' index='0' model='ich9-ehci1'> <alias name='ua-myUSB'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/> </controller> <controller type='usb' index='0' model='ich9-uhci1'> <master startport='0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/> </controller>
which translates to cmd line as:
-device ich9-usb-ehci1,id=ua-myUSB,bus=pci.0,addr=0x4.0x7 -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4
The problem is that UHCI is still trying to serve 'usb.0' bus. Rather than trying to come up with some complicated algorithm to make everything work, lets forbid user aliases for USB controllers.
I'm not a fan of this. This creates situations where the user is not able to know which devices support user aliases and which don't. If there isn't a technical problem preventing this from working we should not forbid it.
On 03/12/2018 10:04 AM, Peter Krempa wrote:
On Fri, Mar 09, 2018 at 12:56:13 +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1552127
These are bit different than other devices. Their alias also specify the name of the bus. For instance, if there are these 'joined' USB devices:
<controller type='usb' index='0' model='ich9-ehci1'> <alias name='ua-myUSB'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/> </controller> <controller type='usb' index='0' model='ich9-uhci1'> <master startport='0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/> </controller>
which translates to cmd line as:
-device ich9-usb-ehci1,id=ua-myUSB,bus=pci.0,addr=0x4.0x7 -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4
The problem is that UHCI is still trying to serve 'usb.0' bus. Rather than trying to come up with some complicated algorithm to make everything work, lets forbid user aliases for USB controllers.
I'm not a fan of this. This creates situations where the user is not able to know which devices support user aliases and which don't. If there isn't a technical problem preventing this from working we should not forbid it.
Thing is I think we are facing technical problem. Let me give you an example: <controller type='usb' index='0' model='ich9-ehci1'> <alias name='ua-myUSB1'/> <address type='pci' domain='0' bus='0' slot='4' function='7'/> </controller> <controller type='usb' index='0' model='ich9-uhci1'> <alias name='ua-myUSB2'/> <master startport='0'/> <address type='pci' domain='0' bus='0' slot='4' function='0' multifunction='on'/> </controller> <controller type='usb' index='0' model='ich9-uhci2'> <alias name='ua-myUSB3'/> <master startport='2'/> <address type='pci' domain='0' bus='0' slot='4' function='1'/> </controller> <controller type='usb' index='0' model='ich9-uhci3'> <alias name='ua-myUSB3'/> <master startport='4'/> <address type='pci' domain='0' bus='0' slot='4' function='2'/> </controller> What should the generated command line look like? To my knowledge, these ich9-XhciN devices are tricky and their id= attribute gives also name to the bus they are creating. Michal
On Mon, Mar 12, 2018 at 13:12:27 +0100, Michal Privoznik wrote:
On 03/12/2018 10:04 AM, Peter Krempa wrote:
On Fri, Mar 09, 2018 at 12:56:13 +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1552127
These are bit different than other devices. Their alias also specify the name of the bus. For instance, if there are these 'joined' USB devices:
<controller type='usb' index='0' model='ich9-ehci1'> <alias name='ua-myUSB'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/> </controller> <controller type='usb' index='0' model='ich9-uhci1'> <master startport='0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/> </controller>
which translates to cmd line as:
-device ich9-usb-ehci1,id=ua-myUSB,bus=pci.0,addr=0x4.0x7 -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4
The problem is that UHCI is still trying to serve 'usb.0' bus. Rather than trying to come up with some complicated algorithm to make everything work, lets forbid user aliases for USB controllers.
I'm not a fan of this. This creates situations where the user is not able to know which devices support user aliases and which don't. If there isn't a technical problem preventing this from working we should not forbid it.
Thing is I think we are facing technical problem. Let me give you an example:
<controller type='usb' index='0' model='ich9-ehci1'> <alias name='ua-myUSB1'/> <address type='pci' domain='0' bus='0' slot='4' function='7'/> </controller> <controller type='usb' index='0' model='ich9-uhci1'> <alias name='ua-myUSB2'/> <master startport='0'/> <address type='pci' domain='0' bus='0' slot='4' function='0' multifunction='on'/> </controller> <controller type='usb' index='0' model='ich9-uhci2'> <alias name='ua-myUSB3'/> <master startport='2'/> <address type='pci' domain='0' bus='0' slot='4' function='1'/> </controller> <controller type='usb' index='0' model='ich9-uhci3'> <alias name='ua-myUSB3'/> <master startport='4'/> <address type='pci' domain='0' bus='0' slot='4' function='2'/> </controller>
What should the generated command line look like? To my knowledge, these ich9-XhciN devices are tricky and their id= attribute gives also name to the bus they are creating.
I don't have deep technical knowledge of how the USB sub-controllers work, but allowing an user alias for the master controller should be possible even now, only the code generating the commandline/alias for the sub-controller needs to be fixed to use the proper one. For the issue of using different alias for controllers which need to have the same, you could forbid that configuration. Another option would be to allow the same alias, but that would not be really different from the situation where users need to know which support duplicate aliases, so I'd not go that way. Yet another option is to treat the alias of the sub-controllers as an opaque string and use the proper master alias on the commandline. This will turn the alias of the sub-controllers into a unused user-provided identifier, but would not break the assumptions of unique aliases and would allow the controllers to work properly. Peter P.S.: The best option would be not to pass the user alias on the commandline at all and reat it opaque strings. But it's too late for that.
participants (2)
-
Michal Privoznik -
Peter Krempa