Re: [libvirt] PATCH: 0/28: Thread safety for libvirtd daemon and drivers

On Wed, Dec 03, 2008 at 06:52:32PM +0000, Daniel P. Berrange wrote: I was having so much fun I wrote more checks for incorrect locking order and data access while unlocked, and improved existing detection. A bunch more bugs found and fixed ! lxc_driver.c | 23 +++++++++++++++++++---- network_driver.c | 3 --- qemu_driver.c | 28 +++++++++++++++++----------- storage_driver.c | 10 ++++++++-- test.c | 3 +-- uml_driver.c | 18 ++++++++++++++---- 6 files changed, 59 insertions(+), 26 deletions(-) Daniel diff --git a/src/lxc_driver.c b/src/lxc_driver.c --- a/src/lxc_driver.c +++ b/src/lxc_driver.c @@ -682,24 +682,32 @@ static void lxcMonitorEvent(int watch, virDomainObjPtr vm = NULL; unsigned int i; + lxcDriverLock(driver); for (i = 0 ; i < driver->domains.count ; i++) { + virDomainObjLock(driver->domains.objs[i]); if (driver->domains.objs[i]->monitorWatch == watch) { vm = driver->domains.objs[i]; break; } + virDomainObjUnlock(driver->domains.objs[i]); } if (!vm) { virEventRemoveHandle(watch); - return; + goto cleanup; } if (vm->monitor != fd) { virEventRemoveHandle(watch); - return; + goto cleanup; } if (lxcVmTerminate(NULL, driver, vm, SIGINT) < 0) virEventRemoveHandle(watch); + +cleanup: + if (vm) + virDomainObjUnlock(vm); + lxcDriverUnlock(driver); } @@ -1153,20 +1161,26 @@ static int lxcStartup(void) char *config = NULL; virDomainDefPtr tmp; int rc; - if ((vm->monitor = lxcMonitorClient(NULL, lxc_driver, vm)) < 0) + virDomainObjLock(vm); + if ((vm->monitor = lxcMonitorClient(NULL, lxc_driver, vm)) < 0) { + virDomainObjUnlock(vm); continue; + } /* Read pid from controller */ if ((rc = virFileReadPid(lxc_driver->stateDir, vm->def->name, &vm->pid)) != 0) { close(vm->monitor); vm->monitor = -1; + virDomainObjUnlock(vm); continue; } if ((config = virDomainConfigFile(NULL, lxc_driver->stateDir, - vm->def->name)) == NULL) + vm->def->name)) == NULL) { + virDomainObjUnlock(vm); continue; + } /* Try and load the live config */ tmp = virDomainDefParseFile(NULL, lxc_driver->caps, config); @@ -1184,6 +1198,7 @@ static int lxcStartup(void) close(vm->monitor); vm->monitor = -1; } + virDomainObjUnlock(vm); } lxcDriverUnlock(lxc_driver); diff --git a/src/network_driver.c b/src/network_driver.c --- a/src/network_driver.c +++ b/src/network_driver.c @@ -1005,7 +1005,6 @@ static virNetworkPtr networkCreate(virCo def = NULL; if (networkStartNetworkDaemon(conn, driver, network) < 0) { - virNetworkObjUnlock(network); virNetworkRemoveInactive(&driver->networks, network); network = NULL; @@ -1043,7 +1042,6 @@ static virNetworkPtr networkDefine(virCo driver->networkConfigDir, driver->networkAutostartDir, network) < 0) { - virNetworkObjUnlock(network); virNetworkRemoveInactive(&driver->networks, network); network = NULL; @@ -1083,7 +1081,6 @@ static int networkUndefine(virNetworkPtr if (virNetworkDeleteConfig(net->conn, network) < 0) goto cleanup; - virNetworkObjUnlock(network); virNetworkRemoveInactive(&driver->networks, network); network = NULL; diff --git a/src/qemu_driver.c b/src/qemu_driver.c --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -243,8 +243,7 @@ qemudStartup(void) { goto out_of_memory; if (qemudLoadDriverConfig(qemu_driver, driverConf) < 0) { - qemudShutdown(); - return -1; + goto error; } if (virDomainLoadAllConfigs(NULL, @@ -327,9 +326,13 @@ qemudActive(void) { return 0; qemuDriverLock(qemu_driver); - for (i = 0 ; i < qemu_driver->domains.count ; i++) - if (virDomainIsActive(qemu_driver->domains.objs[i])) + for (i = 0 ; i < qemu_driver->domains.count ; i++) { + virDomainObjPtr vm = qemu_driver->domains.objs[i]; + virDomainObjLock(vm); + if (virDomainIsActive(vm)) active = 1; + virDomainObjUnlock(vm); + } qemuDriverUnlock(qemu_driver); return active; @@ -1088,12 +1091,15 @@ qemudDispatchVMEvent(int watch, int fd, qemuDriverLock(driver); for (i = 0 ; i < driver->domains.count ; i++) { - if (virDomainIsActive(driver->domains.objs[i]) && - (driver->domains.objs[i]->stdout_watch == watch || - driver->domains.objs[i]->stderr_watch == watch)) { - vm = driver->domains.objs[i]; - break; - } + virDomainObjPtr tmpvm = driver->domains.objs[i]; + virDomainObjLock(vm); + if (virDomainIsActive(tmpvm) && + (tmpvm->stdout_watch == watch || + tmpvm->stderr_watch == watch)) { + vm = tmpvm; + break; + } + virDomainObjUnlock(vm); } if (!vm) @@ -2128,7 +2134,7 @@ static int qemudDomainSave(virDomainPtr if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("failed to write save header")); - return -1; + goto cleanup; } if (safewrite(fd, xml, header.xml_len) != header.xml_len) { diff --git a/src/storage_driver.c b/src/storage_driver.c --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -62,13 +62,14 @@ storageDriverAutostart(virStorageDriverS for (i = 0 ; i < driver->pools.count ; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; - + virStoragePoolObjLock(pool); if (pool->autostart && !virStoragePoolObjIsActive(pool)) { virStorageBackendPtr backend; if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { storageLog("Missing backend %d", pool->def->type); + virStoragePoolObjUnlock(pool); continue; } @@ -77,6 +78,7 @@ storageDriverAutostart(virStorageDriverS virErrorPtr err = virGetLastError(); storageLog("Failed to autostart storage pool '%s': %s", pool->def->name, err ? err->message : NULL); + virStoragePoolObjUnlock(pool); continue; } @@ -86,10 +88,12 @@ storageDriverAutostart(virStorageDriverS backend->stopPool(NULL, pool); storageLog("Failed to autostart storage pool '%s': %s", pool->def->name, err ? err->message : NULL); + virStoragePoolObjUnlock(pool); continue; } pool->active = 1; } + virStoragePoolObjUnlock(pool); } } @@ -237,11 +241,12 @@ storageDriverShutdown(void) { /* shutdown active pools */ for (i = 0 ; i < driverState->pools.count ; i++) { virStoragePoolObjPtr pool = driverState->pools.objs[i]; - + virStoragePoolObjLock(pool); if (virStoragePoolObjIsActive(pool)) { virStorageBackendPtr backend; if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { storageLog("Missing backend"); + virStoragePoolObjUnlock(pool); continue; } @@ -253,6 +258,7 @@ storageDriverShutdown(void) { } virStoragePoolObjClearVols(pool); } + virStoragePoolObjUnlock(pool); } /* free inactive pools */ diff --git a/src/test.c b/src/test.c --- a/src/test.c +++ b/src/test.c @@ -1592,7 +1592,6 @@ static int testDomainUndefine(virDomainP } privdom->state = VIR_DOMAIN_SHUTOFF; - virDomainObjUnlock(privdom); virDomainRemoveInactive(&privconn->domains, privdom); privdom = NULL; @@ -2393,7 +2392,7 @@ testStoragePoolCreate(virConnectPtr conn } if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) { - return NULL; + goto cleanup; } def = NULL; diff --git a/src/uml_driver.c b/src/uml_driver.c --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -215,28 +215,29 @@ umlInotifyEvent(int watch, struct uml_driver *driver = data; virDomainObjPtr dom; + umlDriverLock(driver); if (watch != driver->inotifyWatch) - return; + goto cleanup; reread: got = read(fd, buf, sizeof(buf)); if (got == -1) { if (errno == EINTR) goto reread; - return; + goto cleanup; } tmp = buf; while (got) { if (got < sizeof(struct inotify_event)) - return; /* bad */ + goto cleanup; /* bad */ e = (struct inotify_event *)tmp; tmp += sizeof(struct inotify_event); got -= sizeof(struct inotify_event); if (got < e->len) - return; + goto cleanup; tmp += e->len; got -= e->len; @@ -251,6 +252,7 @@ reread: if (e->mask & IN_DELETE) { if (!virDomainIsActive(dom)) { + virDomainObjUnlock(dom); continue; } @@ -263,10 +265,12 @@ reread: dom->state = VIR_DOMAIN_SHUTOFF; } else if (e->mask & (IN_CREATE | IN_MODIFY)) { if (virDomainIsActive(dom)) { + virDomainObjUnlock(dom); continue; } if (umlReadPidFile(NULL, driver, dom) < 0) { + virDomainObjUnlock(dom); continue; } @@ -279,7 +283,11 @@ reread: if (umlIdentifyChrPTY(NULL, driver, dom) < 0) umlShutdownVMDaemon(NULL, driver, dom); } + virDomainObjUnlock(dom); } + +cleanup: + umlDriverUnlock(driver); } /** @@ -465,8 +473,10 @@ umlShutdown(void) { /* shutdown active VMs */ for (i = 0 ; i < uml_driver->domains.count ; i++) { virDomainObjPtr dom = uml_driver->domains.objs[i]; + virDomainObjLock(dom); if (virDomainIsActive(dom)) umlShutdownVMDaemon(NULL, uml_driver, dom); + virDomainObjUnlock(dom); } virDomainObjListFree(&uml_driver->domains); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|