On Mon, Jan 21, 2013 at 12:22:20PM -0700, Jim Fehlig wrote:
> The libxl driver is racy in it's interactions with libxl and libvirt's
> event loop. The event loop can invoke callbacks after libxl has
> deregistered the event, and possibly access freed data associated with
> the event.
>
> This patch fixes the race by converting libxlDomainObjPrivate to a
> virObjectLockable, and locking it while executing libxl upcalls and
> libvirt event loop callbacks.
>
> Note that using the virDomainObj lock is not satisfactory since it may
> be desirable to hold the virDomainObj lock even when libxl events such
> as reading and writing to xenstore need processed.
> ---
> src/libxl/libxl_conf.h | 3 +
> src/libxl/libxl_driver.c | 140 ++++++++++++++++++++++++++++-------------------
> 2 files changed, 87 insertions(+), 56 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index a3cce08..3d5a461 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -35,6 +35,7 @@
> # include "capabilities.h"
> # include "configmake.h"
> # include "virportallocator.h"
> +# include "virobject.h"
>
>
> # define LIBXL_VNC_PORT_MIN 5900
> @@ -81,6 +82,8 @@ struct _libxlDriverPrivate {
> typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate;
> typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr;
> struct _libxlDomainObjPrivate {
> + virObjectLockable parent;
> +
> /* per domain libxl ctx */
> libxl_ctx *ctx;
> libxl_evgen_domain_death *deathW;
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d4f38e3..81c04ed 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -59,18 +59,16 @@
> /* Number of Xen scheduler parameters */
> #define XEN_SCHED_CREDIT_NPARAM 2
>
> -struct libxlOSEventHookFDInfo {
> - libxlDomainObjPrivatePtr priv;
> - void *xl_priv;
> - int watch;
> -};
> -
> -struct libxlOSEventHookTimerInfo {
> +/* Object used to store info related to libxl event registrations */
> +typedef struct _libxlEventHookInfo libxlEventHookInfo;
> +typedef libxlEventHookInfo *libxlEventHookInfoPtr;
> +struct _libxlEventHookInfo {
> libxlDomainObjPrivatePtr priv;
> void *xl_priv;
> int id;
> };
>
> +static virClassPtr libxlDomainObjPrivateClass;
> static libxlDriverPrivatePtr libxl_driver = NULL;
>
> /* Function declarations */
> @@ -84,6 +82,20 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
> bool start_paused, int restore_fd);
>
> /* Function definitions */
> +static int
> +libxlDomainObjPrivateOnceInit(void)
> +{
> + if (!(libxlDomainObjPrivateClass = virClassNew(virClassForObjectLockable(),
> + "libxlDomainObjPrivate",
> + sizeof(libxlDomainObjPrivate),
> + NULL)))
> + return -1;
> +
> + return 0;
> +}
> +
> +VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate)
> +
> static void
> libxlDriverLock(libxlDriverPrivatePtr driver)
> {
> @@ -97,14 +109,21 @@ libxlDriverUnlock(libxlDriverPrivatePtr driver)
> }
>
> static void
> +libxlEventHookInfoFree(void *obj)
> +{
> + VIR_FREE(obj);
> +}
> +
> +static void
> libxlFDEventCallback(int watch ATTRIBUTE_UNUSED,
> int fd,
> int vir_events,
> void *fd_info)
> {
> - struct libxlOSEventHookFDInfo *info = fd_info;
> + libxlEventHookInfoPtr info = fd_info;
> int events = 0;
>
> + virObjectLock(info->priv);
> if (vir_events & VIR_EVENT_HANDLE_READABLE)
> events |= POLLIN;
> if (vir_events & VIR_EVENT_HANDLE_WRITABLE)
> @@ -114,42 +133,37 @@ libxlFDEventCallback(int watch ATTRIBUTE_UNUSED,
> if (vir_events & VIR_EVENT_HANDLE_HANGUP)
> events |= POLLHUP;
>
> + virObjectUnlock(info->priv);
> libxl_osevent_occurred_fd(info->priv->ctx, info->xl_priv, fd, 0, events);
> }
>
> -static void
> -libxlFreeFDInfo(void *obj)
> -{
> - VIR_FREE(obj);
> -}
> -
> static int
> libxlFDRegisterEventHook(void *priv, int fd, void **hndp,
> short events, void *xl_priv)
> {
> int vir_events = VIR_EVENT_HANDLE_ERROR;
> - struct libxlOSEventHookFDInfo *fdinfo;
> + libxlEventHookInfoPtr info;
>
> - if (VIR_ALLOC(fdinfo) < 0) {
> + if (VIR_ALLOC(info) < 0) {
> virReportOOMError();
> return -1;
> }
>
> - fdinfo->priv = priv;
> - fdinfo->xl_priv = xl_priv;
> - *hndp = fdinfo;
> -
> if (events & POLLIN)
> vir_events |= VIR_EVENT_HANDLE_READABLE;
> if (events & POLLOUT)
> vir_events |= VIR_EVENT_HANDLE_WRITABLE;
> - fdinfo->watch = virEventAddHandle(fd, vir_events, libxlFDEventCallback,
> - fdinfo, libxlFreeFDInfo);
> - if (fdinfo->watch < 0) {
> - VIR_FREE(fdinfo);
> - return fdinfo->watch;
> + info->id = virEventAddHandle(fd, vir_events, libxlFDEventCallback,
> + info, libxlEventHookInfoFree);
> + if (info->id < 0) {
> + VIR_FREE(info);
> + return -1;
> }
>
> + info->priv = priv;
> + info->xl_priv = xl_priv;
> + *hndp = info;
> +
> return 0;
> }
>
> @@ -159,15 +173,18 @@ libxlFDModifyEventHook(void *priv ATTRIBUTE_UNUSED,
> void **hndp,
> short events)
> {
> - struct libxlOSEventHookFDInfo *fdinfo = *hndp;
> + libxlEventHookInfoPtr info = *hndp;
> int vir_events = VIR_EVENT_HANDLE_ERROR;
>
> + virObjectLock(info->priv);
> if (events & POLLIN)
> vir_events |= VIR_EVENT_HANDLE_READABLE;
> if (events & POLLOUT)
> vir_events |= VIR_EVENT_HANDLE_WRITABLE;
>
> - virEventUpdateHandle(fdinfo->watch, vir_events);
> + virEventUpdateHandle(info->id, vir_events);
> + virObjectUnlock(info->priv);
> +
> return 0;
> }
>
> @@ -176,29 +193,31 @@ libxlFDDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
> int fd ATTRIBUTE_UNUSED,
> void *hnd)
> {
> - struct libxlOSEventHookFDInfo *fdinfo = hnd;
> + libxlEventHookInfoPtr info = hnd;
> + libxlDomainObjPrivatePtr p = info->priv;
>
> - virEventRemoveHandle(fdinfo->watch);
> + virObjectLock(p);
> + virEventRemoveHandle(info->id);
> + virObjectUnlock(p);
> }
>
> static void
> libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info)
> {
> - struct libxlOSEventHookTimerInfo *info = timer_info;
> + libxlEventHookInfoPtr info = timer_info;
> + libxlDomainObjPrivatePtr p = info->priv;
>
> + virObjectLock(p);
> /* libxl expects the event to be deregistered when calling
> libxl_osevent_occurred_timeout, but we dont want the event info
> destroyed. Disable the timeout and only remove it after returning
> from libxl. */
> virEventUpdateTimeout(info->id, -1);
> - libxl_osevent_occurred_timeout(info->priv->ctx, info->xl_priv);
> + virObjectUnlock(p);
> + libxl_osevent_occurred_timeout(p->ctx, info->xl_priv);
> + virObjectLock(p);
> virEventRemoveTimeout(info->id);
> -}
> -
> -static void
> -libxlTimerInfoFree(void* obj)
> -{
> - VIR_FREE(obj);
> + virObjectUnlock(p);
> }
>
> static int
> @@ -207,13 +226,13 @@ libxlTimeoutRegisterEventHook(void *priv,
> struct timeval abs_t,
> void *xl_priv)
> {
> + libxlEventHookInfoPtr info;
> struct timeval now;
> struct timeval res;
> static struct timeval zero;
> - struct libxlOSEventHookTimerInfo *timer_info;
> - int timeout, timer_id;
> + int timeout;
>
> - if (VIR_ALLOC(timer_info) < 0) {
> + if (VIR_ALLOC(info) < 0) {
> virReportOOMError();
> return -1;
> }
> @@ -228,17 +247,17 @@ libxlTimeoutRegisterEventHook(void *priv,
> } else {
> timeout = res.tv_sec * 1000 + (res.tv_usec + 999) / 1000;
> }
> - timer_id = virEventAddTimeout(timeout, libxlTimerCallback,
> - timer_info, libxlTimerInfoFree);
> - if (timer_id < 0) {
> - VIR_FREE(timer_info);
> - return timer_id;
> + info->id = virEventAddTimeout(timeout, libxlTimerCallback,
> + info, libxlEventHookInfoFree);
> + if (info->id < 0) {
> + VIR_FREE(info);
> + return -1;
> }
>
> - timer_info->priv = priv;
> - timer_info->xl_priv = xl_priv;
> - timer_info->id = timer_id;
> - *hndp = timer_info;
> + info->priv = priv;
> + info->xl_priv = xl_priv;
> + *hndp = info;
> +
> return 0;
> }
>
> @@ -257,10 +276,13 @@ libxlTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED,
> void **hndp,
> struct timeval abs_t ATTRIBUTE_UNUSED)
> {
> - struct libxlOSEventHookTimerInfo *timer_info = *hndp;
> + libxlEventHookInfoPtr info = *hndp;
>
> + virObjectLock(info->priv);
> /* Make the timeout fire */
> - virEventUpdateTimeout(timer_info->id, 0);
> + virEventUpdateTimeout(info->id, 0);
> + virObjectUnlock(info->priv);
> +
> return 0;
> }
>
> @@ -268,9 +290,12 @@ static void
> libxlTimeoutDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
> void *hnd)
> {
> - struct libxlOSEventHookTimerInfo *timer_info = hnd;
> + libxlEventHookInfoPtr info = hnd;
> + libxlDomainObjPrivatePtr p = info->priv;
>
> - virEventRemoveTimeout(timer_info->id);
> + virObjectLock(p);
> + virEventRemoveTimeout(info->id);
> + virObjectUnlock(p);
> }
>
> static const libxl_osevent_hooks libxl_event_callbacks = {
> @@ -287,12 +312,15 @@ libxlDomainObjPrivateAlloc(void)
> {
> libxlDomainObjPrivatePtr priv;
>
> - if (VIR_ALLOC(priv) < 0)
> + if (libxlDomainObjPrivateInitialize() < 0)
> + return NULL;
> +
> + if (!(priv = virObjectLockableNew(libxlDomainObjPrivateClass)))
> return NULL;
>
> if (libxl_ctx_alloc(&priv->ctx, LIBXL_VERSION, 0, libxl_driver->logger)) {
> VIR_ERROR(_("Failed libxl context initialization"));
> - VIR_FREE(priv);
> + virObjectUnref(priv);
> return NULL;
> }
>
> @@ -310,7 +338,7 @@ libxlDomainObjPrivateFree(void *data)
> libxl_evdisable_domain_death(priv->ctx, priv->deathW);
>
> libxl_ctx_free(priv->ctx);
> - VIR_FREE(priv);
> + virObjectUnref(priv);
> }
>
> /* driver must be locked before calling */
ACK, adding a new lock allows to synchronize access, fine, i just hope
there is no risk of deadlock, we will see :-)
Daniel
--
Daniel Veillard | Open Source and Standards, Red Hat
veillard@xxxxxxxxxx | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list