[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/6] libxl: Fix races in libxl event code



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 redhat com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]