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 <berrange@redhat.com>

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 695c0f9ff943070d2bb808c34e05c8dad34f3b32..149558fdc2fb036a7435604ef474d052de0e0c6c 100644 (file)
--- 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 ee3b9e1387447aa0e67c03f5553b63ee1093d817..bac38b5672a452dec7e8504bae75db17ec815f3d 100644 (file)
--- 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 9fbeee36afdc32c21976947162b76e7b5c8f9e47..8676904f6ecd88b2ed95a9bc28a7dd134a3aa8a4 100644 (file)
--- 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",