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>

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;

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;
         }
     }
-
+    ret = 0;
-    return 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,

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",