[libvirt PATCH] qemu: prevent attempts to detach a device on a controller with hotplug='off'
Although the original patches to support controllers with hotplug='off' were checking during hotplug/attach requests that the device was being plugged into a PCI controller that didn't have hotplug disabled, but I forgot to do the same for device detach (the main impetus for adding the feature was to prevent unplugs originating from within the guest, so it slipped my mind). So although the guest OS was ultimately unable to honor the unplug request, libvirt could still be used to make such a request, and since device attach/detach are asynchronous operations, the caller to libvirt would receive a success status back (the device would stubbornly/correctly remain in the domain status XML however) This patch remedies that, by looking at the controller for the device in the detach request, and immediately failing the operation if that controller has hotplug=off. r changes. Lines starting Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ab5a7aef84..5fe125b1a7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5891,6 +5891,33 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return -1; } + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + int controllerIdx = virDomainControllerFind(vm->def, + VIR_DOMAIN_CONTROLLER_TYPE_PCI, + info->addr.pci.bus); + if (controllerIdx < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot hot unplug %s device with PCI guest address: " + VIR_PCI_DEVICE_ADDRESS_FMT + " - controller not found"), + virDomainDeviceTypeToString(detach.type), + info->addr.pci.domain, info->addr.pci.bus, + info->addr.pci.slot, info->addr.pci.function); + return -1; + } + + if (vm->def->controllers[controllerIdx]->opts.pciopts.hotplug + == VIR_TRISTATE_SWITCH_OFF) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot hot unplug %s device with PCI guest address: " + VIR_PCI_DEVICE_ADDRESS_FMT + " - not allowed by controller"), + virDomainDeviceTypeToString(detach.type), + info->addr.pci.domain, info->addr.pci.bus, + info->addr.pci.slot, info->addr.pci.function); + return -1; + } + } /* * Issue the qemu monitor command to delete the device (based on * its alias), and optionally wait a short time in case the -- 2.25.4
On Thu, May 14, 2020 at 02:12:38PM -0400, Laine Stump wrote:
Although the original patches to support controllers with hotplug='off' were checking during hotplug/attach requests that the device was being plugged into a PCI controller that didn't have hotplug disabled, but I forgot to do the same for device detach (the main impetus for adding the feature was to prevent unplugs originating from within the guest, so it slipped my mind). So although the guest OS was ultimately unable to honor the unplug request, libvirt could still be used to make such a request, and since device attach/detach are asynchronous operations, the caller to libvirt would receive a success status back (the device would stubbornly/correctly remain in the domain status XML however)
This patch remedies that, by looking at the controller for the device in the detach request, and immediately failing the operation if that controller has hotplug=off.
r changes. Lines starting
^This looks like some accidental copy-paste :)
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ab5a7aef84..5fe125b1a7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5891,6 +5891,33 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return -1; }
+ if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + int controllerIdx = virDomainControllerFind(vm->def, + VIR_DOMAIN_CONTROLLER_TYPE_PCI, + info->addr.pci.bus); + if (controllerIdx < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot hot unplug %s device with PCI guest address: " + VIR_PCI_DEVICE_ADDRESS_FMT + " - controller not found"), + virDomainDeviceTypeToString(detach.type), + info->addr.pci.domain, info->addr.pci.bus, + info->addr.pci.slot, info->addr.pci.function); + return -1; + } +
To enhance readability, use a temporary variable: virDomainControllerDefPtr controller = vm->def->controllers[controllerIdx]; if (controller->opts.pciopts.hotplug == VIR_TRISTATE_SWITCH_OFF) {...}
+ if (vm->def->controllers[controllerIdx]->opts.pciopts.hotplug + == VIR_TRISTATE_SWITCH_OFF) { + virReportError(VIR_ERR_OPERATION_FAILED,
So, VIR_ERR_OPERATION_FAILED makes perfect sense here, but it feels quite inconsistent with what error we return in the opposite scenario when the flags are compared in virDomainPCIAddressFlagsCompatible, where we'd report internal error on hotplug. Just my humble opinion, I agree with ^this proposal.
+ _("cannot hot unplug %s device with PCI guest address: " + VIR_PCI_DEVICE_ADDRESS_FMT + " - not allowed by controller"), + virDomainDeviceTypeToString(detach.type), + info->addr.pci.domain, info->addr.pci.bus, + info->addr.pci.slot, info->addr.pci.function); + return -1; + } + }
Add an extra line please.
/* * Issue the qemu monitor command to delete the device (based on * its alias), and optionally wait a short time in case the
Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 5/15/20 3:58 AM, Erik Skultety wrote:
On Thu, May 14, 2020 at 02:12:38PM -0400, Laine Stump wrote:
Although the original patches to support controllers with hotplug='off' were checking during hotplug/attach requests that the device was being plugged into a PCI controller that didn't have hotplug disabled, but I forgot to do the same for device detach (the main impetus for adding the feature was to prevent unplugs originating from within the guest, so it slipped my mind). So although the guest OS was ultimately unable to honor the unplug request, libvirt could still be used to make such a request, and since device attach/detach are asynchronous operations, the caller to libvirt would receive a success status back (the device would stubbornly/correctly remain in the domain status XML however)
This patch remedies that, by looking at the controller for the device in the detach request, and immediately failing the operation if that controller has hotplug=off.
r changes. Lines starting ^This looks like some accidental copy-paste :)
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ab5a7aef84..5fe125b1a7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5891,6 +5891,33 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return -1; }
+ if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + int controllerIdx = virDomainControllerFind(vm->def, + VIR_DOMAIN_CONTROLLER_TYPE_PCI, + info->addr.pci.bus); + if (controllerIdx < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot hot unplug %s device with PCI guest address: " + VIR_PCI_DEVICE_ADDRESS_FMT + " - controller not found"), + virDomainDeviceTypeToString(detach.type), + info->addr.pci.domain, info->addr.pci.bus, + info->addr.pci.slot, info->addr.pci.function); + return -1; + } + To enhance readability, use a temporary variable:
virDomainControllerDefPtr controller = vm->def->controllers[controllerIdx]; if (controller->opts.pciopts.hotplug == VIR_TRISTATE_SWITCH_OFF) {...}
I suppose it does put the two sides of the == on a single line. Other than that it doesn't do much, just takes up an extra line because the definition has to be on a separate line from its assignment (you can only use controllerIdx if it's >= 0, so it could be invalid up in the block where automatics need to be defined), and it's only used once (if it was used more than once I would have already done it). Anyway, since you asked, I did it now before pushing. (NB: I also would have made a temporary variable to point directly at info->addr.pci, but there was existing usage of it as is, and I would have felt compelled to change that usage to use a temporary as well, but didn't want to restructure that much. I'll send a followup patch for that)
+ if (vm->def->controllers[controllerIdx]->opts.pciopts.hotplug + == VIR_TRISTATE_SWITCH_OFF) { + virReportError(VIR_ERR_OPERATION_FAILED, So, VIR_ERR_OPERATION_FAILED makes perfect sense here, but it feels quite inconsistent with what error we return in the opposite scenario when the flags are compared in virDomainPCIAddressFlagsCompatible, where we'd report internal error on hotplug.
That depends on whether the address was generated automatically by libvirt, or provided by the user. virDomainPCIAddressFlagsCompatible will give an INTERNAL_ERROR if libvirt generated the address (shouldn't ever happen - libvirt should instead say that it can't find an appropriate slot at an earlier time). If the user provides an address, then it gives XML_ERROR. I guess at the time that code was written, we were thinking more about the fact that the address came from XML written by the user rather than what was being done with that XML. Anyway, in the attach case, the user is saying "put this device *here*, and libvirt is saying "*here* isn't a proper place to put this device". In the detach case, the user is saying "unplug this device" and libvirt is saying "you can't unplug this device", which is a bit of a different thing from "this device can't be put here". But that's just nitpicking. The more important thing is that I'm just following whatever error types were already there for similar things (and in a lot of cases the decision of which error type to use is inaccurate and subjective, because the same function may be called from different places that would suggest different error types - point in case is that virDomainPCIAddressFlagsCompatible tries to use a different error type depending on the context; if we try to go too far down that road the code gets ugly and hairy for no real gain - in the end it's only important to inform what is broken, and the user can decide themselves how exactly to classify that breakage)
Just my humble opinion, I agree with ^this proposal.
+ _("cannot hot unplug %s device with PCI guest address: " + VIR_PCI_DEVICE_ADDRESS_FMT + " - not allowed by controller"), + virDomainDeviceTypeToString(detach.type), + info->addr.pci.domain, info->addr.pci.bus, + info->addr.pci.slot, info->addr.pci.function); + return -1; + } + } Add an extra line please.
/* * Issue the qemu monitor command to delete the device (based on * its alias), and optionally wait a short time in case the
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Thanks!
participants (3)
-
Erik Skultety -
Laine Stump -
Laine Stump