On Mon, Jan 21, 2013 at 12:22:22PM -0700, Jim Fehlig wrote:
> I've noticed that libxl can invoke timeout reregister/modify hooks
> after returning from libxl_ctx_free. Explicitly remove the
> timeouts before freeing the libxl ctx to avoid executing hooks on
> stale objects.
> ---
> src/libxl/libxl_conf.h | 6 ++++
> src/libxl/libxl_driver.c | 71 +++++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 73 insertions(+), 4 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 3d5a461..f6167e6 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -79,6 +79,9 @@ struct _libxlDriverPrivate {
> char *saveDir;
> };
>
> +typedef struct _libxlEventHookInfo libxlEventHookInfo;
> +typedef libxlEventHookInfo *libxlEventHookInfoPtr;
> +
> typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate;
> typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr;
> struct _libxlDomainObjPrivate {
> @@ -87,6 +90,9 @@ struct _libxlDomainObjPrivate {
> /* per domain libxl ctx */
> libxl_ctx *ctx;
> libxl_evgen_domain_death *deathW;
> +
> + /* list of libxl timeout registrations */
> + libxlEventHookInfoPtr timerRegistrations;
> };
>
> # define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r"
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 530a17f..08dffd6 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -59,10 +59,39 @@
> /* Number of Xen scheduler parameters */
> #define XEN_SCHED_CREDIT_NPARAM 2
>
> +/* Append an event registration to the list of registrations */
> +#define LIBXL_EV_REG_APPEND(head, add) \
> + do { \
> + libxlEventHookInfoPtr temp; \
> + if (head) { \
> + temp = head; \
> + while (temp->next) \
> + temp = temp->next; \
> + temp->next = add; \
> + } else { \
> + head = add; \
> + } \
> + } while (0)
> +
> +/* Remove an event registration from the list of registrations */
> +#define LIBXL_EV_REG_REMOVE(head, del) \
> + do { \
> + libxlEventHookInfoPtr temp; \
> + if (head == del) { \
> + head = head->next; \
> + } else { \
> + temp = head; \
> + while (temp->next && temp->next != del) \
> + temp = temp->next; \
> + if (temp->next) { \
> + temp->next = del->next; \
> + } \
> + } \
> + } while (0)
> +
> /* Object used to store info related to libxl event registrations */
> -typedef struct _libxlEventHookInfo libxlEventHookInfo;
> -typedef libxlEventHookInfo *libxlEventHookInfoPtr;
> struct _libxlEventHookInfo {
> + libxlEventHookInfoPtr next;
> libxlDomainObjPrivatePtr priv;
> void *xl_priv;
> int id;
> @@ -225,7 +254,11 @@ libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info)
> virObjectUnlock(p);
> libxl_osevent_occurred_timeout(p->ctx, info->xl_priv);
> virObjectLock(p);
> - virEventRemoveTimeout(info->id);
> + /* timeout could have been freed while the lock was dropped.
> + Only remove it from the list if it still exists.
> + */
> + if (virEventRemoveTimeout(info->id) == 0)
> + LIBXL_EV_REG_REMOVE(p->timerRegistrations, info);
> virObjectUnlock(p);
> }
>
> @@ -269,6 +302,9 @@ libxlTimeoutRegisterEventHook(void *priv,
> event objects. */
> virObjectRef(info->priv);
>
> + virObjectLock(info->priv);
> + LIBXL_EV_REG_APPEND(info->priv->timerRegistrations, info);
> + virObjectUnlock(info->priv);
> info->xl_priv = xl_priv;
> *hndp = info;
>
> @@ -308,10 +344,35 @@ libxlTimeoutDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
> libxlDomainObjPrivatePtr p = info->priv;
>
> virObjectLock(p);
> - virEventRemoveTimeout(info->id);
> + /* Only remove the timeout from the list if removal from the
> + event loop is successful.
> + */
> + if (virEventRemoveTimeout(info->id) == 0)
> + LIBXL_EV_REG_REMOVE(p->timerRegistrations, info);
> virObjectUnlock(p);
> }
>
> +static void
> +libxlRegisteredTimeoutsCleanup(libxlDomainObjPrivatePtr priv)
> +{
> + libxlEventHookInfoPtr info;
> +
> + virObjectLock(priv);
> + info = priv->timerRegistrations;
> + while (info) {
> + /* 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(priv->ctx, info->xl_priv);
> + virEventRemoveTimeout(info->id);
> + info = info->next;
> + }
> + priv->timerRegistrations = NULL;
> + virObjectUnlock(priv);
> +}
> +
> static const libxl_osevent_hooks libxl_event_callbacks = {
> .fd_register = libxlFDRegisterEventHook,
> .fd_modify = libxlFDModifyEventHook,
> @@ -565,6 +626,8 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
> vm->def->id = -1;
> vm->newDef = NULL;
> }
> +
> + libxlRegisteredTimeoutsCleanup(priv);
> }
>
> /*
ACK, just one small nit, usually we format multi-line comments as
/*
* bla
* bla
*/
could you update your patches accordingly :-) ?
thanks !
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