External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-12-26 23:55:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

libxl: find virDomainObj in libxlDomainShutdownThread

Browse Source 
libxl events are delivered to libvirt via the libxlDomainEventHandler
callback registered with libxl. Documenation in
$xensrc/tools/libxl/libxl_event.h states that the callback "may occur
on any thread in which the application calls libxl". This can result
in deadlock since many of the libvirt callees of libxl hold a lock on
the virDomainObj they are working on. When the callback is invoked, it
attempts to find a virDomainObj corresponding to the domain ID provided
by libxl. Searching the domain obj list results in locking each obj
before checking if it is active, and its ID equals the requested ID.
Deadlock is possible when attempting to lock an obj that is already
locked further up the call stack. Indeed, Max Ustermann reported an
instance of this deadlock

https://www.redhat.com/archives/libvir-list/2015-November/msg00130.html

Guido Rossmueller also recently stumbled across it

https://www.redhat.com/archives/libvir-list/2016-September/msg00287.html

Fix the deadlock by moving the lookup of virDomainObj to the
libxlDomainShutdownThread. After this patch, libxl events are
enqueued on the libvirt side and processed by dedicated thread,
avoiding the described deadlock.

Reported-by: Max Ustermann <ustermann78@web.de>
Reported-by: Guido Rossmueller <Guido.Rossmueller@gdata.de>
This commit is contained in:
Jim Fehlig 2016-09-21 15:02:34 -06:00
parent 934c4dbea8
commit 7b3cf84bbb
1 changed files with 8 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
src/libxl/libxl_domain.c
View File

@ -423,7 +423,6 @@ virDomainDefParserConfig libxlDomainDefParserConfig = {
struct libxlShutdownThreadInfo struct libxlShutdownThreadInfo
{ {
libxlDriverPrivatePtr driver; libxlDriverPrivatePtr driver;
virDomainObjPtr vm;
libxl_event *event; libxl_event *event;
}; };
@ -432,7 +431,7 @@ static void
libxlDomainShutdownThread(void *opaque) libxlDomainShutdownThread(void *opaque)
{ {
struct libxlShutdownThreadInfo *shutdown_info = opaque; struct libxlShutdownThreadInfo *shutdown_info = opaque;
virDomainObjPtr vm = shutdown_info->vm; virDomainObjPtr vm = NULL;
libxl_event *ev = shutdown_info->event; libxl_event *ev = shutdown_info->event;
libxlDriverPrivatePtr driver = shutdown_info->driver; libxlDriverPrivatePtr driver = shutdown_info->driver;
virObjectEventPtr dom_event = NULL; virObjectEventPtr dom_event = NULL;
@ -441,6 +440,12 @@ libxlDomainShutdownThread(void *opaque)
cfg = libxlDriverConfigGet(driver); cfg = libxlDriverConfigGet(driver);
vm = virDomainObjListFindByIDRef(driver->domains, ev->domid);
if (!vm) {
VIR_INFO("Received event for unknown domain ID %d", ev->domid);
goto cleanup;
}
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
goto cleanup; goto cleanup;
@ -549,7 +554,6 @@ void
libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
{ {
libxlDriverPrivatePtr driver = data; libxlDriverPrivatePtr driver = data;
virDomainObjPtr vm = NULL;
libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason; libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
struct libxlShutdownThreadInfo *shutdown_info = NULL; struct libxlShutdownThreadInfo *shutdown_info = NULL;
virThread thread; virThread thread;
@ -567,12 +571,6 @@ 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 = virDomainObjListFindByIDRef(driver->domains, event->domid);
if (!vm) {
VIR_INFO("Received event for unknown domain ID %d", event->domid);
goto error;
}
/* /*
* Start a thread to handle shutdown. We don't want to be tying up * Start a thread to handle shutdown. We don't want to be tying up
* libxl's event machinery by doing a potentially lengthy shutdown. * libxl's event machinery by doing a potentially lengthy shutdown.
@ -581,7 +579,6 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
goto error; goto error;
shutdown_info->driver = driver; shutdown_info->driver = driver;
shutdown_info->vm = vm;
shutdown_info->event = (libxl_event *)event; shutdown_info->event = (libxl_event *)event;
if (virThreadCreate(&thread, false, libxlDomainShutdownThread, if (virThreadCreate(&thread, false, libxlDomainShutdownThread,
shutdown_info) < 0) { shutdown_info) < 0) {
@ -593,7 +590,7 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
} }
/* /*
* VM is unlocked and libxl_event freed in shutdown thread * libxlShutdownThreadInfo and libxl_event are freed in shutdown thread
*/ */
return; return;
@ -602,7 +599,6 @@ 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);
virDomainObjEndAPI(&vm);
VIR_FREE(shutdown_info); VIR_FREE(shutdown_info);
} }