[libvirt] [PATCH] qemu: Call qemuSetupHostdevCGroup later during hotplug

https://bugzilla.redhat.com/show_bug.cgi?id=1025108 So far qemuSetupHostdevCGroup was called very early during hotplug, even before we knew the device we were about to hotplug was actually available. By calling the function later, we make sure QEMU won't be allowed to access devices used by other domains. Another important effect of this change is that hopluging USB devices specified by vendor and product (but not by their USB address) works again. This was broken since v1.0.5-171-g7d763ac, when the call to qemuFindHostdevUSBDevice was moved after the call to qemuSetupHostdevCGroup, which then used an uninitialized USB address. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_hotplug.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6eb483c..0d9a3aa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1135,6 +1135,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, int configfd = -1; char *configfd_name = NULL; bool releaseaddr = false; + bool teardowncgroup = false; int backend = hostdev->source.subsys.u.pci.backend; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) @@ -1171,6 +1172,10 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, break; } + if (qemuSetupHostdevCGroup(vm, hostdev) < 0) + goto error; + teardowncgroup = true; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto error; @@ -1226,6 +1231,9 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, return 0; error: + if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); @@ -1418,6 +1426,7 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, virUSBDevicePtr usb = NULL; char *devstr = NULL; bool added = false; + bool teardowncgroup = false; int ret = -1; if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) @@ -1435,6 +1444,10 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, added = true; virUSBDeviceListSteal(list, usb); + if (qemuSetupHostdevCGroup(vm, hostdev) < 0) + goto cleanup; + teardowncgroup = true; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto cleanup; @@ -1461,6 +1474,10 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, ret = 0; cleanup: + if (ret < 0 && + teardowncgroup && + qemuTeardownHostdevCgroup(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); if (added) virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); virUSBDeviceFree(usb); @@ -1478,6 +1495,7 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; char *devstr = NULL; char *drvstr = NULL; + bool teardowncgroup = false; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE) || !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) || @@ -1498,6 +1516,10 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, return -1; } + if (qemuSetupHostdevCGroup(vm, hostdev) < 0) + goto cleanup; + teardowncgroup = true; + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto cleanup; @@ -1535,8 +1557,11 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, ret = 0; cleanup: - if (ret < 0) + if (ret < 0) { qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &hostdev, 1); + if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + } VIR_FREE(drvstr); VIR_FREE(devstr); return ret; @@ -1553,12 +1578,9 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, return -1; } - if (qemuSetupHostdevCGroup(vm, hostdev) < 0) - return -1; - if (virSecurityManagerSetHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) - goto cleanup; + return -1; switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: @@ -1592,10 +1614,6 @@ error: if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); - -cleanup: - if (qemuTeardownHostdevCgroup(vm, hostdev) < 0) - VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); return -1; } -- 1.8.4.3

On Thu, Nov 14, 2013 at 03:04:13PM +0100, Jiri Denemark wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1025108
So far qemuSetupHostdevCGroup was called very early during hotplug, even before we knew the device we were about to hotplug was actually available. By calling the function later, we make sure QEMU won't be allowed to access devices used by other domains.
Another important effect of this change is that hopluging USB devices specified by vendor and product (but not by their USB address) works again. This was broken since v1.0.5-171-g7d763ac, when the call to qemuFindHostdevUSBDevice was moved after the call to qemuSetupHostdevCGroup, which then used an uninitialized USB address.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_hotplug.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)
I won't nack the patch for this, but wanted to point out that we should expand the qemuhotplugtest.c file to use an LD_PRELOAD to mock out the cgroups filesystem. Then we can validate that the cgroups setup is being done correctly for each hotplug operation we test. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 11/14/13 15:04, Jiri Denemark wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1025108
So far qemuSetupHostdevCGroup was called very early during hotplug, even before we knew the device we were about to hotplug was actually available. By calling the function later, we make sure QEMU won't be allowed to access devices used by other domains.
Another important effect of this change is that hopluging USB devices specified by vendor and product (but not by their USB address) works again. This was broken since v1.0.5-171-g7d763ac, when the call to qemuFindHostdevUSBDevice was moved after the call to qemuSetupHostdevCGroup, which then used an uninitialized USB address.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_hotplug.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)
Insisting on doing the test with LD_PRELOAD as Dan suggested would be kind of madness in this case when considering the complexity of the test. ACK to the code changes. Peter

On Fri, Nov 15, 2013 at 11:34:33 +0100, Peter Krempa wrote:
On 11/14/13 15:04, Jiri Denemark wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1025108
So far qemuSetupHostdevCGroup was called very early during hotplug, even before we knew the device we were about to hotplug was actually available. By calling the function later, we make sure QEMU won't be allowed to access devices used by other domains.
Another important effect of this change is that hopluging USB devices specified by vendor and product (but not by their USB address) works again. This was broken since v1.0.5-171-g7d763ac, when the call to qemuFindHostdevUSBDevice was moved after the call to qemuSetupHostdevCGroup, which then used an uninitialized USB address.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_hotplug.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)
Insisting on doing the test with LD_PRELOAD as Dan suggested would be kind of madness in this case when considering the complexity of the test.
Yeah, since I already did quite a lot of hacking in the hotplug test, I may eventually improve it with mocking cgroups and other stuff but it's going to take some time for sure.
ACK to the code changes.
Thanks and pushed. Jirka
participants (3)
-
Daniel P. Berrange
-
Jiri Denemark
-
Peter Krempa