[libvirt] [PATCHv2] qemu: Validate address type when attaching a disk device.
https://bugzilla.redhat.com/show_bug.cgi?id=1257844 Attach-device can hotplug a virtio disk device with any address type now, it need to validate the address type before the attachment. Coldplug a scsi disk device without checking the address type in current version, this patch also fix the scsi disk problem. --- src/qemu/qemu_driver.c | 8 ++++++++ src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 91eb661..af926fc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) if (virDomainDefAddImplicitControllers(vmdef) < 0) return -1; + /* scsi disk should have an address type of driver */ + if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && + (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("scsi disk cannot have an address of type '%s'"), + virDomainDeviceAddressTypeToString(disk->info.type)); + return -1; + } if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; break; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 63fafa6..4226650 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, priv->qemuCaps, disk->dst)) goto cleanup; + + /* virtio device should either have a ccw or pci address */ + if (qemuDomainMachineIsS390CCW(vm->def) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { + if (!virDomainDeviceAddressIsValid(&disk->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device cannot be attached without a valid CCW address")); + goto cleanup; + } + } else { + if (!virDomainDeviceAddressIsValid(&disk->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device cannot be attached without a valid PCI address")); + goto cleaup; + } + } } for (i = 0; i < vm->def->ndisks; i++) { -- 2.4.3
On Mon, Sep 07, 2015 at 07:22:01PM +0800, Ruifeng Bian wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Attach-device can hotplug a virtio disk device with any address type now, it need to validate the address type before the attachment.
Coldplug a scsi disk device without checking the address type in current version, this patch also fix the scsi disk problem. --- src/qemu/qemu_driver.c | 8 ++++++++ src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 91eb661..af926fc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) if (virDomainDefAddImplicitControllers(vmdef) < 0) return -1; + /* scsi disk should have an address type of driver */ + if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && + (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("scsi disk cannot have an address of type '%s'"), + virDomainDeviceAddressTypeToString(disk->info.type)); + return -1; + }
This is the check for DISK_BUS_SCSI, qemuDomainAssignAddresses does the check for DISK_BUS_VIRTIO. Should we check other bus types as well? I think the most important part of the bug is that we allow hotplugging a device with broken configuration, then fail to unplug it because it is broken. Here detach-device works with --config even with an incorrect address type.
if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; break; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 63fafa6..4226650 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, priv->qemuCaps, disk->dst)) goto cleanup; + + /* virtio device should either have a ccw or pci address */ + if (qemuDomainMachineIsS390CCW(vm->def) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { + if (!virDomainDeviceAddressIsValid(&disk->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device cannot be attached without a valid CCW address")); + goto cleanup; + }
Is hotplugging a device with a S390 type address impossible on a s390-ccw machine?
+ } else { + if (!virDomainDeviceAddressIsValid(&disk->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device cannot be attached without a valid PCI address"));
These are not the only two options. At this point, the address type can also be VIRTIO_S390 for non-CCW S390 machines, and ADDRESS_TYPE_NONE (and a PCI address will be generated by virDomainPCIAddressEnsureAddr). The checks seem to be copied from qemuDomainDetachVirtioDiskDevice. For ancient QEMUs which did not support -device, we use the PCI address to uniquely identify the device. In that case, detaching a virtio disk with no address is not possible. For all reasonable versions of QEMU, the device alias is used, so detaching devices with other address types is possible. But we explicitly require a PCI or CCW address, to make sure we will unplug only the device requested by user. S390 seems to be missing here, but that might be on purpose. Allowing S390 addresses on hotplug without allowing them on hotunplug won't fix the linked bug for S390 machines, but attaching a device with no PCI address is a valid use case - libvirt will just generate one.
+ goto cleaup;
Please make sure 'make check' and 'make syntax-check' pass before sending a patch to the list, as described on our hacking page: http://libvirt.org/hacking.html Jan
----- Original Message -----
From: "Ján Tomko" <jtomko@redhat.com> To: "Ruifeng Bian" <rbian@redhat.com> Cc: libvir-list@redhat.com Sent: Tuesday, September 8, 2015 8:07:21 PM Subject: Re: [libvirt] [PATCHv2] qemu: Validate address type when attaching a disk device.
On Mon, Sep 07, 2015 at 07:22:01PM +0800, Ruifeng Bian wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Attach-device can hotplug a virtio disk device with any address type now, it need to validate the address type before the attachment.
Coldplug a scsi disk device without checking the address type in current version, this patch also fix the scsi disk problem. --- src/qemu/qemu_driver.c | 8 ++++++++ src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 91eb661..af926fc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) if (virDomainDefAddImplicitControllers(vmdef) < 0) return -1; + /* scsi disk should have an address type of driver */ + if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && + (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("scsi disk cannot have an address of type '%s'"), + virDomainDeviceAddressTypeToString(disk->info.type)); + return -1; + }
This is the check for DISK_BUS_SCSI, qemuDomainAssignAddresses does the check for DISK_BUS_VIRTIO.
Should we check other bus types as well?
Yes, other bus types still have problem, I will collect all types.
I think the most important part of the bug is that we allow hotplugging a device with broken configuration, then fail to unplug it because it is broken. Here detach-device works with --config even with an incorrect address type.
Detach-device should also check address types, I will add it.
if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; break; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 63fafa6..4226650 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, priv->qemuCaps, disk->dst)) goto cleanup; + + /* virtio device should either have a ccw or pci address */ + if (qemuDomainMachineIsS390CCW(vm->def) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { + if (!virDomainDeviceAddressIsValid(&disk->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device cannot be attached without a valid CCW address")); + goto cleanup; + }
Is hotplugging a device with a S390 type address impossible on a s390-ccw machine?
+ } else { + if (!virDomainDeviceAddressIsValid(&disk->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device cannot be attached without a valid PCI address"));
These are not the only two options. At this point, the address type can also be VIRTIO_S390 for non-CCW S390 machines, and ADDRESS_TYPE_NONE (and a PCI address will be generated by virDomainPCIAddressEnsureAddr).
ADDRESS_TYPE_NONE won't come here, since disk->info.type is 0.
The checks seem to be copied from qemuDomainDetachVirtioDiskDevice. For ancient QEMUs which did not support -device, we use the PCI address to uniquely identify the device. In that case, detaching a virtio disk with no address is not possible.
For all reasonable versions of QEMU, the device alias is used, so detaching devices with other address types is possible. But we explicitly require a PCI or CCW address, to make sure we will unplug only the device requested by user. S390 seems to be missing here, but that might be on purpose.
Allowing S390 addresses on hotplug without allowing them on hotunplug won't fix the linked bug for S390 machines, but attaching a device with no PCI address is a valid use case - libvirt will just generate one.
Yes, the check code from qemuDomainDetachVirtioDiskDevice. I thought the code should work both for attach and detach. If user provide virtio device address for attachment, the address type should be PCI, CCW or S390. We can check machine type to see if CCW address should be used. But how to check S390 address should be used? Could I use virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)? In all other cases, PCI address should be used. Right?
+ goto cleaup;
Please make sure 'make check' and 'make syntax-check' pass before sending a patch to the list, as described on our hacking page: http://libvirt.org/hacking.html
Jan
participants (2)
-
Ján Tomko -
Ruifeng Bian