From 011cf7ad1029ec6d0c181914e54a06b1289bcae3 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 16 Jan 2013 12:09:58 +0000 Subject: [PATCH] Protect USB/PCI device list access in QEMU with dedicated locks Currently the activePciHostdevs, inactivePciHostdevsd and activeUsbHostdevs lists are all implicitly protected by the QEMU driver lock. Now that the lists all inherit from the virObjectLockable, we can make the locking explicit, removing the dependency on the QEMU driver lock for correctness. Signed-off-by: Daniel P. Berrange --- src/qemu/qemu_driver.c | 15 ++++++++++- src/qemu/qemu_hostdev.c | 57 ++++++++++++++++++++++++++++++----------- src/qemu/qemu_hotplug.c | 6 +++++ 3 files changed, 62 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 695c0f9ff9..149558fdc2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10034,6 +10034,8 @@ qemuNodeDeviceDettach(virNodeDevicePtr dev) return -1; qemuDriverLock(driver); + virObjectLock(driver->activePciHostdevs); + virObjectLock(driver->inactivePciHostdevs); in_inactive_list = virPCIDeviceListFind(driver->inactivePciHostdevs, pci); if (virPCIDeviceDetach(pci, driver->activePciHostdevs, @@ -10042,6 +10044,8 @@ qemuNodeDeviceDettach(virNodeDevicePtr dev) ret = 0; out: + virObjectUnlock(driver->inactivePciHostdevs); + virObjectUnlock(driver->activePciHostdevs); qemuDriverUnlock(driver); if (in_inactive_list) virPCIDeviceFree(pci); @@ -10064,6 +10068,9 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) if (!pci) return -1; + qemuDriverLock(driver); + virObjectLock(driver->activePciHostdevs); + virObjectLock(driver->inactivePciHostdevs); other = virPCIDeviceListFind(driver->activePciHostdevs, pci); if (other) { const char *other_name = virPCIDeviceGetUsedBy(other); @@ -10076,17 +10083,19 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) virReportError(VIR_ERR_OPERATION_INVALID, _("PCI device %s is still in use"), virPCIDeviceGetName(pci)); + goto out; } virPCIDeviceReattachInit(pci); - qemuDriverLock(driver); if (virPCIDeviceReattach(pci, driver->activePciHostdevs, driver->inactivePciHostdevs, "pci-stub") < 0) goto out; ret = 0; out: + virObjectUnlock(driver->inactivePciHostdevs); + virObjectUnlock(driver->activePciHostdevs); qemuDriverUnlock(driver); virPCIDeviceFree(pci); return ret; @@ -10108,6 +10117,8 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) return -1; qemuDriverLock(driver); + virObjectLock(driver->activePciHostdevs); + virObjectLock(driver->inactivePciHostdevs); if (virPCIDeviceReset(pci, driver->activePciHostdevs, driver->inactivePciHostdevs) < 0) @@ -10115,6 +10126,8 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) ret = 0; out: + virObjectUnlock(driver->inactivePciHostdevs); + virObjectUnlock(driver->activePciHostdevs); qemuDriverUnlock(driver); virPCIDeviceFree(pci); return ret; diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index ee3b9e1387..bac38b5672 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -72,6 +72,9 @@ qemuGetPciHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) return list; } +/* + * Pre-condition: driver->activePciHostdevs is locked + */ static virPCIDeviceListPtr qemuGetActivePciHostDeviceList(virQEMUDriverPtr driver, virDomainHostdevDefPtr *hostdevs, @@ -121,10 +124,14 @@ int qemuUpdateActivePciHostdevs(virQEMUDriverPtr driver, { virDomainHostdevDefPtr hostdev = NULL; int i; + int ret = -1; if (!def->nhostdevs) return 0; + virObjectLock(driver->activePciHostdevs); + virObjectLock(driver->inactivePciHostdevs); + for (i = 0; i < def->nhostdevs; i++) { virPCIDevicePtr dev = NULL; hostdev = def->hostdevs[i]; @@ -140,7 +147,7 @@ int qemuUpdateActivePciHostdevs(virQEMUDriverPtr driver, hostdev->source.subsys.u.pci.function); if (!dev) - return -1; + goto cleanup; virPCIDeviceSetManaged(dev, hostdev->managed); virPCIDeviceSetUsedBy(dev, def->name); @@ -152,11 +159,14 @@ int qemuUpdateActivePciHostdevs(virQEMUDriverPtr driver, if (virPCIDeviceListAdd(driver->activePciHostdevs, dev) < 0) { virPCIDeviceFree(dev); - return -1; + goto cleanup; } } - return 0; +cleanup: + virObjectUnlock(driver->activePciHostdevs); + virObjectUnlock(driver->inactivePciHostdevs); + return ret; } int @@ -165,10 +175,12 @@ qemuUpdateActiveUsbHostdevs(virQEMUDriverPtr driver, { virDomainHostdevDefPtr hostdev = NULL; int i; + int ret = -1; if (!def->nhostdevs) return 0; + virObjectLock(driver->activeUsbHostdevs); for (i = 0; i < def->nhostdevs; i++) { virUSBDevicePtr usb = NULL; hostdev = def->hostdevs[i]; @@ -193,11 +205,13 @@ qemuUpdateActiveUsbHostdevs(virQEMUDriverPtr driver, if (virUSBDeviceListAdd(driver->activeUsbHostdevs, usb) < 0) { virUSBDeviceFree(usb); - return -1; + goto cleanup; } } - - return 0; + ret = 0; +cleanup: + virObjectUnlock(driver->activeUsbHostdevs); + return ret; } static int @@ -412,6 +426,9 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, int ret = -1; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virObjectLock(driver->activePciHostdevs); + virObjectLock(driver->inactivePciHostdevs); + if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) goto cleanup; @@ -580,19 +597,13 @@ reattachdevs: } cleanup: + virObjectUnlock(driver->activePciHostdevs); + virObjectUnlock(driver->inactivePciHostdevs); virObjectUnref(pcidevs); virObjectUnref(cfg); return ret; } -static int -qemuPrepareHostPCIDevices(virQEMUDriverPtr driver, - virDomainDefPtr def) -{ - return qemuPrepareHostdevPCIDevices(driver, def->name, def->uuid, - def->hostdevs, def->nhostdevs); -} - int qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver, const char *name, @@ -602,6 +613,7 @@ qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver, unsigned int count; virUSBDevicePtr tmp; + virObjectLock(driver->activeUsbHostdevs); count = virUSBDeviceListCount(list); for (i = 0; i < count; i++) { @@ -631,6 +643,8 @@ qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver, if (virUSBDeviceListAdd(driver->activeUsbHostdevs, usb) < 0) goto error; } + + virObjectUnlock(driver->activeUsbHostdevs); return 0; error: @@ -638,6 +652,7 @@ error: tmp = virUSBDeviceListGet(list, i); virUSBDeviceListSteal(driver->activeUsbHostdevs, tmp); } + virObjectUnlock(driver->activeUsbHostdevs); return -1; } @@ -803,7 +818,8 @@ int qemuPrepareHostDevices(virQEMUDriverPtr driver, if (!def->nhostdevs) return 0; - if (qemuPrepareHostPCIDevices(driver, def) < 0) + if (qemuPrepareHostdevPCIDevices(driver, def->name, def->uuid, + def->hostdevs, def->nhostdevs) < 0) return -1; if (qemuPrepareHostUSBDevices(driver, def, coldBoot) < 0) @@ -813,6 +829,10 @@ int qemuPrepareHostDevices(virQEMUDriverPtr driver, } +/* + * Pre-condition: driver->inactivePciHostdevs & driver->activePciHostdevs + * are locked + */ void qemuReattachPciDevice(virPCIDevicePtr dev, virQEMUDriverPtr driver) { int retries = 100; @@ -852,6 +872,9 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver, int i; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virObjectLock(driver->activePciHostdevs); + virObjectLock(driver->inactivePciHostdevs); + if (!(pcidevs = qemuGetActivePciHostDeviceList(driver, hostdevs, nhostdevs))) { @@ -918,6 +941,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver, virObjectUnref(pcidevs); cleanup: + virObjectUnlock(driver->activePciHostdevs); + virObjectUnlock(driver->inactivePciHostdevs); virObjectUnref(cfg); } @@ -929,6 +954,7 @@ qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver, { int i; + virObjectLock(driver->activeUsbHostdevs); for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; virUSBDevicePtr usb, tmp; @@ -980,6 +1006,7 @@ qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver, virUSBDeviceListDel(driver->activeUsbHostdevs, tmp); } } + virObjectUnlock(driver->activeUsbHostdevs); } void qemuDomainReAttachHostDevices(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9fbeee36af..8676904f6e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2368,6 +2368,8 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver, if (detach->parent.data.net) qemuDomainHostdevNetConfigRestore(detach, cfg->stateDir); + virObjectLock(driver->activePciHostdevs); + virObjectLock(driver->inactivePciHostdevs); pci = virPCIDeviceNew(subsys->u.pci.domain, subsys->u.pci.bus, subsys->u.pci.slot, subsys->u.pci.function); if (pci) { @@ -2383,6 +2385,8 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver, } virPCIDeviceFree(pci); } + virObjectUnlock(driver->activePciHostdevs); + virObjectUnlock(driver->inactivePciHostdevs); if (qemuCapsGet(priv->caps, QEMU_CAPS_DEVICE) && qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, @@ -2425,7 +2429,9 @@ qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, usb = virUSBDeviceNew(subsys->u.usb.bus, subsys->u.usb.device, NULL); if (usb) { + virObjectLock(driver->activeUsbHostdevs); virUSBDeviceListDel(driver->activeUsbHostdevs, usb); + virObjectUnlock(driver->activeUsbHostdevs); virUSBDeviceFree(usb); } else { VIR_WARN("Unable to find device %03d.%03d in list of used USB devices",