libxl: fix vm lock overwritten bug

In libxl driver we do virObjectRef in libxlDomainObjBeginJob,
If virCondWaitUntil failed, it goes to error, do virObjectUnref,
There's a chance that someone undefine the vm at the same time,
and refs unref to zero, vm is freed in libxlDomainObjBeginJob.
But the vm outside function is not Null, we do virObjectUnlock(vm).
That's how we overwrite the vm memory after it's freed. I fix it.

Signed-off-by: Wang Yufei <james.wangyufei@huawei.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This commit is contained in:
Wang Yufei 2016-06-12 18:30:00 +08:00 committed by Michal Privoznik
parent fc1b2428ac
commit 9ac9450780
7 changed files with 87 additions and 112 deletions

View File

@ -112,24 +112,45 @@ static int virDomainObjListSearchID(const void *payload,
return want; return want;
} }
static virDomainObjPtr
virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms, virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
int id) int id,
bool ref)
{ {
virDomainObjPtr obj; virDomainObjPtr obj;
virObjectLock(doms); virObjectLock(doms);
obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id); obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id);
if (ref) {
virObjectRef(obj);
virObjectUnlock(doms);
}
if (obj) { if (obj) {
virObjectLock(obj); virObjectLock(obj);
if (obj->removing) { if (obj->removing) {
virObjectUnlock(obj); virObjectUnlock(obj);
if (ref)
virObjectUnref(obj);
obj = NULL; obj = NULL;
} }
} }
virObjectUnlock(doms); if (!ref)
virObjectUnlock(doms);
return obj; return obj;
} }
virDomainObjPtr
virDomainObjListFindByID(virDomainObjListPtr doms,
int id)
{
return virDomainObjListFindByIDInternal(doms, id, false);
}
virDomainObjPtr
virDomainObjListFindByIDRef(virDomainObjListPtr doms,
int id)
{
return virDomainObjListFindByIDInternal(doms, id, true);
}
static virDomainObjPtr static virDomainObjPtr
virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,

View File

@ -34,6 +34,8 @@ virDomainObjListPtr virDomainObjListNew(void);
virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms, virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
int id); int id);
virDomainObjPtr virDomainObjListFindByIDRef(virDomainObjListPtr doms,
int id);
virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms, virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms,
const unsigned char *uuid); const unsigned char *uuid);
virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms, virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,

View File

@ -888,6 +888,7 @@ virDomainObjListCollect;
virDomainObjListConvert; virDomainObjListConvert;
virDomainObjListExport; virDomainObjListExport;
virDomainObjListFindByID; virDomainObjListFindByID;
virDomainObjListFindByIDRef;
virDomainObjListFindByName; virDomainObjListFindByName;
virDomainObjListFindByUUID; virDomainObjListFindByUUID;
virDomainObjListFindByUUIDRef; virDomainObjListFindByUUIDRef;

View File

@ -123,8 +123,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
return -1; return -1;
then = now + LIBXL_JOB_WAIT_TIME; then = now + LIBXL_JOB_WAIT_TIME;
virObjectRef(obj);
while (priv->job.active) { while (priv->job.active) {
VIR_DEBUG("Wait normal job condition for starting job: %s", VIR_DEBUG("Wait normal job condition for starting job: %s",
libxlDomainJobTypeToString(job)); libxlDomainJobTypeToString(job));
@ -157,7 +155,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
virReportSystemError(errno, virReportSystemError(errno,
"%s", _("cannot acquire job mutex")); "%s", _("cannot acquire job mutex"));
virObjectUnref(obj);
return -1; return -1;
} }
@ -171,7 +168,7 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
* non-zero, false if the reference count has dropped to zero * non-zero, false if the reference count has dropped to zero
* and obj is disposed. * and obj is disposed.
*/ */
bool void
libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
virDomainObjPtr obj) virDomainObjPtr obj)
{ {
@ -183,8 +180,6 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
libxlDomainObjResetJob(priv); libxlDomainObjResetJob(priv);
virCondSignal(&priv->job.cond); virCondSignal(&priv->job.cond);
return virObjectUnref(obj);
} }
int int
@ -532,12 +527,10 @@ libxlDomainShutdownThread(void *opaque)
} }
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
if (dom_event) if (dom_event)
libxlDomainEventQueue(driver, dom_event); libxlDomainEventQueue(driver, dom_event);
libxl_event_free(cfg->ctx, ev); libxl_event_free(cfg->ctx, ev);
@ -570,7 +563,7 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND) if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
goto error; goto error;
vm = virDomainObjListFindByID(driver->domains, event->domid); vm = virDomainObjListFindByIDRef(driver->domains, event->domid);
if (!vm) { if (!vm) {
VIR_INFO("Received event for unknown domain ID %d", event->domid); VIR_INFO("Received event for unknown domain ID %d", event->domid);
goto error; goto error;
@ -605,8 +598,7 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
/* Cast away any const */ /* Cast away any const */
libxl_event_free(cfg->ctx, (libxl_event *)event); libxl_event_free(cfg->ctx, (libxl_event *)event);
virObjectUnref(cfg); virObjectUnref(cfg);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
VIR_FREE(shutdown_info); VIR_FREE(shutdown_info);
} }

View File

@ -85,10 +85,9 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
enum libxlDomainJob job) enum libxlDomainJob job)
ATTRIBUTE_RETURN_CHECK; ATTRIBUTE_RETURN_CHECK;
bool void
libxlDomainObjEndJob(libxlDriverPrivatePtr driver, libxlDomainObjEndJob(libxlDriverPrivatePtr driver,
virDomainObjPtr obj) virDomainObjPtr obj);
ATTRIBUTE_RETURN_CHECK;
int int
libxlDomainJobUpdateTime(struct libxlDomainJobObj *job) libxlDomainJobUpdateTime(struct libxlDomainJobObj *job)

View File

@ -290,7 +290,7 @@ libxlDomObjFromDomain(virDomainPtr dom)
libxlDriverPrivatePtr driver = dom->conn->privateData; libxlDriverPrivatePtr driver = dom->conn->privateData;
char uuidstr[VIR_UUID_STRING_BUFLEN]; char uuidstr[VIR_UUID_STRING_BUFLEN];
vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); vm = virDomainObjListFindByUUIDRef(driver->domains, dom->uuid);
if (!vm) { if (!vm) {
virUUIDFormat(dom->uuid, uuidstr); virUUIDFormat(dom->uuid, uuidstr);
virReportError(VIR_ERR_NO_DOMAIN, virReportError(VIR_ERR_NO_DOMAIN,
@ -309,13 +309,12 @@ libxlAutostartDomain(virDomainObjPtr vm,
libxlDriverPrivatePtr driver = opaque; libxlDriverPrivatePtr driver = opaque;
int ret = -1; int ret = -1;
virObjectRef(vm);
virObjectLock(vm); virObjectLock(vm);
virResetLastError(); virResetLastError();
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
virObjectUnlock(vm); goto cleanup;
return ret;
}
if (vm->autostart && !virDomainObjIsActive(vm) && if (vm->autostart && !virDomainObjIsActive(vm) &&
libxlDomainStartNew(driver, vm, false) < 0) { libxlDomainStartNew(driver, vm, false) < 0) {
@ -328,8 +327,9 @@ libxlAutostartDomain(virDomainObjPtr vm,
ret = 0; ret = 0;
endjob: endjob:
if (libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
virObjectUnlock(vm); cleanup:
virDomainObjEndAPI(&vm);
return ret; return ret;
} }
@ -983,6 +983,7 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL))) NULL)))
goto cleanup; goto cleanup;
virObjectRef(vm);
def = NULL; def = NULL;
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@ -1008,13 +1009,11 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
dom->id = vm->def->id; dom->id = vm->def->id;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
virDomainDefFree(def); virDomainDefFree(def);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virObjectUnref(cfg); virObjectUnref(cfg);
return dom; return dom;
} }
@ -1141,12 +1140,10 @@ libxlDomainSuspend(virDomainPtr dom)
ret = 0; ret = 0;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
if (event) if (event)
libxlDomainEventQueue(driver, event); libxlDomainEventQueue(driver, event);
virObjectUnref(cfg); virObjectUnref(cfg);
@ -1200,12 +1197,10 @@ libxlDomainResume(virDomainPtr dom)
ret = 0; ret = 0;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
if (event) if (event)
libxlDomainEventQueue(driver, event); libxlDomainEventQueue(driver, event);
virObjectUnref(cfg); virObjectUnref(cfg);
@ -1373,12 +1368,10 @@ libxlDomainDestroyFlags(virDomainPtr dom,
ret = 0; ret = 0;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
if (event) if (event)
libxlDomainEventQueue(driver, event); libxlDomainEventQueue(driver, event);
virObjectUnref(cfg); virObjectUnref(cfg);
@ -1517,12 +1510,10 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
ret = 0; ret = 0;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virObjectUnref(cfg); virObjectUnref(cfg);
return ret; return ret;
} }
@ -1746,16 +1737,14 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml,
ret = 0; ret = 0;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (remove_dom && vm) { if (remove_dom && vm) {
virDomainObjListRemove(driver->domains, vm); virDomainObjListRemove(driver->domains, vm);
vm = NULL; vm = NULL;
} }
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
return ret; return ret;
} }
@ -1802,7 +1791,7 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL))) NULL)))
goto cleanup; goto cleanup;
virObjectRef(vm);
def = NULL; def = NULL;
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@ -1819,15 +1808,13 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
if (ret < 0 && !vm->persistent) if (ret < 0 && !vm->persistent)
virDomainObjListRemove(driver->domains, vm); virDomainObjListRemove(driver->domains, vm);
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (VIR_CLOSE(fd) < 0) if (VIR_CLOSE(fd) < 0)
virReportSystemError(errno, "%s", _("cannot close file")); virReportSystemError(errno, "%s", _("cannot close file"));
virDomainDefFree(def); virDomainDefFree(def);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virObjectUnref(cfg); virObjectUnref(cfg);
return ret; return ret;
} }
@ -1923,16 +1910,14 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
} }
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (remove_dom && vm) { if (remove_dom && vm) {
virDomainObjListRemove(driver->domains, vm); virDomainObjListRemove(driver->domains, vm);
vm = NULL; vm = NULL;
} }
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
if (event) if (event)
libxlDomainEventQueue(driver, event); libxlDomainEventQueue(driver, event);
virObjectUnref(cfg); virObjectUnref(cfg);
@ -1986,16 +1971,14 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags)
ret = 0; ret = 0;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (remove_dom && vm) { if (remove_dom && vm) {
virDomainObjListRemove(driver->domains, vm); virDomainObjListRemove(driver->domains, vm);
vm = NULL; vm = NULL;
} }
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
VIR_FREE(name); VIR_FREE(name);
return ret; return ret;
} }
@ -2212,14 +2195,12 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
} }
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
VIR_FREE(bitmask); VIR_FREE(bitmask);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm); virObjectUnref(cfg);
virObjectUnref(cfg);
return ret; return ret;
} }
@ -2357,12 +2338,10 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu,
} }
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virBitmapFree(pcpumap); virBitmapFree(pcpumap);
virObjectUnref(cfg); virObjectUnref(cfg);
return ret; return ret;
@ -2685,12 +2664,10 @@ libxlDomainCreateWithFlags(virDomainPtr dom,
dom->id = vm->def->id; dom->id = vm->def->id;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
return ret; return ret;
} }
@ -3695,14 +3672,12 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
} }
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
virDomainDefFree(vmdef); virDomainDefFree(vmdef);
virDomainDeviceDefFree(dev); virDomainDeviceDefFree(dev);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virObjectUnref(cfg); virObjectUnref(cfg);
return ret; return ret;
} }
@ -3788,14 +3763,12 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
} }
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
virDomainDefFree(vmdef); virDomainDefFree(vmdef);
virDomainDeviceDefFree(dev); virDomainDeviceDefFree(dev);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virObjectUnref(cfg); virObjectUnref(cfg);
return ret; return ret;
} }
@ -4076,14 +4049,12 @@ libxlDomainSetAutostart(virDomainPtr dom, int autostart)
ret = 0; ret = 0;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
VIR_FREE(configFile); VIR_FREE(configFile);
VIR_FREE(autostartLink); VIR_FREE(autostartLink);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virObjectUnref(cfg); virObjectUnref(cfg);
return ret; return ret;
} }
@ -4288,12 +4259,10 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom,
ret = 0; ret = 0;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virObjectUnref(cfg); virObjectUnref(cfg);
return ret; return ret;
} }
@ -4609,12 +4578,10 @@ libxlDomainInterfaceStats(virDomainPtr dom,
_("'%s' is not a known interface"), path); _("'%s' is not a known interface"), path);
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
return ret; return ret;
} }
@ -4791,13 +4758,11 @@ libxlDomainMemoryStats(virDomainPtr dom,
ret = i; ret = i;
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
libxl_dominfo_dispose(&d_info); libxl_dominfo_dispose(&d_info);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virObjectUnref(cfg); virObjectUnref(cfg);
return ret; return ret;
} }
@ -5364,8 +5329,7 @@ libxlDomainMigrateFinish3Params(virConnectPtr dconn,
ret = libxlDomainMigrationFinish(dconn, vm, flags, cancelled); ret = libxlDomainMigrationFinish(dconn, vm, flags, cancelled);
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
virDomainObjEndAPI(&vm); virDomainObjEndAPI(&vm);

View File

@ -266,6 +266,7 @@ libxlDoMigrateReceive(void *opaque)
int ret; int ret;
bool remove_dom = 0; bool remove_dom = 0;
virObjectRef(vm);
virObjectLock(vm); virObjectLock(vm);
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
goto cleanup; goto cleanup;
@ -291,16 +292,14 @@ libxlDoMigrateReceive(void *opaque)
VIR_FORCE_CLOSE(recvfd); VIR_FORCE_CLOSE(recvfd);
virObjectUnref(args); virObjectUnref(args);
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
if (remove_dom && vm) { if (remove_dom && vm) {
virDomainObjListRemove(driver->domains, vm); virDomainObjListRemove(driver->domains, vm);
vm = NULL; vm = NULL;
} }
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
} }
@ -437,14 +436,11 @@ libxlDomainMigrationBegin(virConnectPtr conn,
xml = virDomainDefFormat(def, cfg->caps, VIR_DOMAIN_DEF_FORMAT_SECURE); xml = virDomainDefFormat(def, cfg->caps, VIR_DOMAIN_DEF_FORMAT_SECURE);
endjob: endjob:
if (!libxlDomainObjEndJob(driver, vm)) libxlDomainObjEndJob(driver, vm);
vm = NULL;
cleanup: cleanup:
libxlMigrationCookieFree(mig); libxlMigrationCookieFree(mig);
if (vm) virDomainObjEndAPI(&vm);
virObjectUnlock(vm);
virDomainDefFree(tmpdef); virDomainDefFree(tmpdef);
virObjectUnref(cfg); virObjectUnref(cfg);
return xml; return xml;