libxl: Fix races in libxl event code

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.

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index a3cce080a1e4826b432baafe0cbe7304f3a6c3ac..8617d312c7e63d292958a5b5d65ea86b37528b2f 100644 (file)
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -1,5 +1,5 @@
 /*---------------------------------------------------------------------------*/
-/*  Copyright (c) 2011 SUSE LINUX Products GmbH, Nuernberg, Germany.
+/*  Copyright (C) 2011-2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
  *  Copyright (C) 2011 Univention GmbH.
  *
  * This library is free software; you can redistribute it and/or
@@ -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 0e2c9cd87988cbf0d2f3690839f07ed50af90ba2..a34be51efe2cbd9ae08c500706be814bd8d01742 100644 (file)
--- 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)
 {
@@ -96,15 +108,22 @@ libxlDriverUnlock(libxlDriverPrivatePtr driver)
     virMutexUnlock(&driver->lock);
 }
 
+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,16 +193,21 @@ 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
@@ -193,14 +215,11 @@ libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info)
      * 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
@@ -209,13 +228,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;
     }
@@ -230,17 +249,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;
 }
 
@@ -259,10 +278,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;
 }
 
@@ -270,9 +292,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 = {
@@ -289,12 +314,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;
     }
 
@@ -312,7 +340,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 */