From d301bc8d08084272cde2e210e6b257b05d005d02 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Fri, 6 Sep 2019 13:59:59 +0200 Subject: [PATCH] lib: Grab write lock when modifying list of domains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ján Tomko --- src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/conf/virdomainobjlist.c | 12 ++++++++++-- src/conf/virdomainobjlist.h | 1 + src/libxl/libxl_driver.c | 11 +++++++---- src/lxc/lxc_process.c | 4 ++-- src/qemu/qemu_driver.c | 7 ++++++- src/qemu/qemu_process.c | 3 ++- src/vmware/vmware_driver.c | 2 +- 9 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index e2c1b00080..c52def7607 100644 --- 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 4dab6e5e54..5fea2eb51c 100644 --- 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 11fd68745b..2eff6768c2 100644 --- 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 7d71bc54d0..da5ec8a57c 100644 --- 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 215471fa0d..fccc1f42e8 100644 --- 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 cd65e7a0c0..cbdc7b1268 100644 --- 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 73e1571dc0..b28a26c3d6 100644 --- 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 1960f53466..6a261786f9 100644 --- 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 1bc8a06c39..a87af85916 100644 --- 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 -- GitLab