lib: Grab write lock when modifying list of domains

In some places where virDomainObjListForEach() is called the
passed callback calls virDomainObjListRemoveLocked(). Well, this
is unsafe, because the former only grabs a read lock but the
latter modifies the list.
I've identified the following unsafe calls:

- qemuProcessReconnectAll()
- libxlReconnectDomains()

The rest seem to be safe.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index e2c1b000807ea4fa4aff9ced1449ac64f1d6a45a..c52def7607eeabe806ec262ff8ed38ae89b35d56 100644 (file)
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -114,7 +114,7 @@ bhyveAutostartDomains(bhyveConnPtr driver)
 
     struct bhyveAutostartData data = { driver, conn };
 
-    virDomainObjListForEach(driver->domains, bhyveAutostartDomain, &data);
+    virDomainObjListForEach(driver->domains, false, bhyveAutostartDomain, &data);
 
     virObjectUnref(conn);
 }

diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 4dab6e5e5426ca9c3aa37015acf82b8eb5fcdef3..5fea2eb51c1692bb501ae842688a08fd92f57759 100644 (file)
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -466,7 +466,7 @@ virBhyveProcessReconnectAll(bhyveConnPtr driver)
     data.driver = driver;
     data.kd = kd;
 
-    virDomainObjListForEach(driver->domains, virBhyveProcessReconnect, &data);
+    virDomainObjListForEach(driver->domains, false, virBhyveProcessReconnect, &data);
 
     kvm_close(kd);
 }

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 11fd68745b0bc557867165d5097bac01da0d8220..2eff6768c20010ef1f79bc4fef99f4662168be42 100644 (file)
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -818,25 +818,33 @@ virDomainObjListHelper(void *payload,
 /**
  * virDomainObjListForEach:
  * @doms: Pointer to the domain object list
+ * @modify: Whether to lock @doms for modify operation
  * @callback: callback to run over each domain on the list
  * @opaque: opaque data to pass to @callback
  *
  * For every domain on the list (@doms) run @callback on it. If
  * @callback fails (i.e. returns a negative value), the iteration
- * carries still on until all domains are visited.
+ * carries still on until all domains are visited. Moreover, if
+ * @callback wants to modify the list of domains (@doms) then
+ * @modify must be set to true.
  *
  * Returns: 0 on success,
  *         -1 otherwise.
  */
 int
 virDomainObjListForEach(virDomainObjListPtr doms,
+                        bool modify,
                         virDomainObjListIterator callback,
                         void *opaque)
 {
     struct virDomainListIterData data = {
         callback, opaque, 0,
     };
-    virObjectRWLockRead(doms);
+
+    if (modify)
+        virObjectRWLockWrite(doms);
+    else
+        virObjectRWLockRead(doms);
     virHashForEach(doms->objs, virDomainObjListHelper, &data);
     virObjectRWUnlock(doms);
     return data.ret;

diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index 7d71bc54d08bea5a5b556402425a5388286d991c..da5ec8a57c16274e4a22a7fb0c2fe00c0b6fe748 100644 (file)
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -91,6 +91,7 @@ typedef int (*virDomainObjListIterator)(virDomainObjPtr dom,
                                         void *opaque);
 
 int virDomainObjListForEach(virDomainObjListPtr doms,
+                            bool modify,
                             virDomainObjListIterator callback,
                             void *opaque);
 

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 215471fa0d3351429ba1e7f1511d02c937ce14ae..fccc1f42e837ef434964d4edc19ec2705448d8f3 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -497,7 +497,7 @@ libxlReconnectDomain(virDomainObjPtr vm,
 static void
 libxlReconnectDomains(libxlDriverPrivatePtr driver)
 {
-    virDomainObjListForEach(driver->domains, libxlReconnectDomain, driver);
+    virDomainObjListForEach(driver->domains, true, libxlReconnectDomain, driver);
 }
 
 static int
@@ -800,10 +800,12 @@ libxlStateInitialize(bool privileged,
                                        NULL, NULL) < 0)
         goto error;
 
-    virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain,
+    virDomainObjListForEach(libxl_driver->domains, false,
+                            libxlAutostartDomain,
                             libxl_driver);
 
-    virDomainObjListForEach(libxl_driver->domains, libxlDomainManagedSaveLoad,
+    virDomainObjListForEach(libxl_driver->domains, false,
+                            libxlDomainManagedSaveLoad,
                             libxl_driver);
 
     return VIR_DRV_STATE_INIT_COMPLETE;
@@ -832,7 +834,8 @@ libxlStateReload(void)
                                    libxl_driver->xmlopt,
                                    NULL, libxl_driver);
 
-    virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain,
+    virDomainObjListForEach(libxl_driver->domains, false,
+                            libxlAutostartDomain,
                             libxl_driver);
 
     virObjectUnref(cfg);

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index cd65e7a0c0c9ba8e222d7a8d0ca49abeca551fac..cbdc7b1268973f6c322d2d0b18d57b44a86b79e5 100644 (file)
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1639,7 +1639,7 @@ virLXCProcessAutostartAll(virLXCDriverPtr driver)
 
     struct virLXCProcessAutostartData data = { driver, conn };
 
-    virDomainObjListForEach(driver->domains,
+    virDomainObjListForEach(driver->domains, false,
                             virLXCProcessAutostartDomain,
                             &data);
 
@@ -1760,6 +1760,6 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm,
 int virLXCProcessReconnectAll(virLXCDriverPtr driver,
                               virDomainObjListPtr doms)
 {
-    virDomainObjListForEach(doms, virLXCProcessReconnectDomain, driver);
+    virDomainObjListForEach(doms, false, virLXCProcessReconnectDomain, driver);
     return 0;
 }

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 73e1571dc007fa4966d671328ad034defa01f85d..b28a26c3d623fefe329cd2dfaff64b2e79078a72 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -304,7 +304,7 @@ qemuAutostartDomain(virDomainObjPtr vm,
 static void
 qemuAutostartDomains(virQEMUDriverPtr driver)
 {
-    virDomainObjListForEach(driver->domains, qemuAutostartDomain, driver);
+    virDomainObjListForEach(driver->domains, false, qemuAutostartDomain, driver);
 }
 
 
@@ -1055,10 +1055,12 @@ qemuStateInitialize(bool privileged,
      * the driver with. This is to avoid race between autostart and reconnect
      * threads */
     virDomainObjListForEach(qemu_driver->domains,
+                            false,
                             qemuDomainFindMaxID,
                             &qemu_driver->lastvmid);
 
     virDomainObjListForEach(qemu_driver->domains,
+                            false,
                             qemuDomainNetsRestart,
                             NULL);
 
@@ -1072,14 +1074,17 @@ qemuStateInitialize(bool privileged,
         goto error;
 
     virDomainObjListForEach(qemu_driver->domains,
+                            false,
                             qemuDomainSnapshotLoad,
                             cfg->snapshotDir);
 
     virDomainObjListForEach(qemu_driver->domains,
+                            false,
                             qemuDomainCheckpointLoad,
                             cfg->checkpointDir);
 
     virDomainObjListForEach(qemu_driver->domains,
+                            false,
                             qemuDomainManagedSaveLoad,
                             qemu_driver);
 

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1960f534665d49c7460ed5aa51f103c97c39ce2b..6a261786f976cd7f4690ca1cf7871a0037f5a9a5 100644 (file)
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8369,7 +8369,8 @@ void
 qemuProcessReconnectAll(virQEMUDriverPtr driver)
 {
     struct qemuProcessReconnectData data = {.driver = driver};
-    virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper, &data);
+    virDomainObjListForEach(driver->domains, true,
+                            qemuProcessReconnectHelper, &data);
 }
 
 

diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
index 1bc8a06c39f6d2f53caaec732830a2b92e7a35d9..a87af85916952df746e3e0fbf6f40705e9ad807b 100644 (file)
--- a/src/vmware/vmware_driver.c
+++ b/src/vmware/vmware_driver.c
@@ -990,7 +990,7 @@ static int vmwareDomainObjListUpdateDomain(virDomainObjPtr dom, void *data)
 static void
 vmwareDomainObjListUpdateAll(virDomainObjListPtr doms, struct vmware_driver *driver)
 {
-    virDomainObjListForEach(doms, vmwareDomainObjListUpdateDomain, driver);
+    virDomainObjListForEach(doms, false, vmwareDomainObjListUpdateDomain, driver);
 }
 
 static int