|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] libxl: fix stale fd event callback race
Ian Jackson wrote:
> Because there is not necessarily any lock held at the point the
> application (eg, libvirt) calls libxl_osevent_occurred_timeout and
> ..._fd, in a multithreaded program those calls may be arbitrarily
> delayed in relation to other activities within the program.
>
> libxl therefore needs to be prepared to receive very old event
> callbacks. Arrange for this to be the case for fd callbacks.
>
> This requires a new layer of indirection through a "hook nexus" struct
> which can outlive the libxl__ev_foo. Allocation and deallocation of
> these nexi is mostly handled in the OSEVENT macros which wrap up
> the application's callbacks.
>
> Document the problem and the solution in a comment in libxl_event.c
> just before the definition of struct libxl__osevent_hook_nexus.
>
> There is still a race relating to libxl__osevent_occurred_timeout;
> this will be addressed in the following patch.
>
> Reported-by: Bamvor Jian Zhang <bjzhang@xxxxxxxx>
> Cc: Bamvor Jian Zhang <bjzhang@xxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>
Hi Ian,
Thanks for the patches! I've found some time to test them in the context
of Xen 4.2 and have some comments. For this patch, only a few nits below.
> --
> v2:
> - Prepare for fixing timeout race too
> - Break out osevent_release_nexus()
> - nexusop argument to OSEVENT* macros
> - Clarify OSEVENT* nexusop hooks
> - osevent_ev_from_hook_nexus takes a libxl__osevent_hook_nexus*
> ---
> tools/libxl/libxl_event.c | 184 +++++++++++++++++++++++++++++++++++------
> tools/libxl/libxl_internal.h | 8 ++-
> 2 files changed, 163 insertions(+), 29 deletions(-)
>
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 72cb723..f1fe425 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -38,23 +38,131 @@
> * The application's registration hooks should be called ONLY via
> * these macros, with the ctx locked. Likewise all the "occurred"
> * entrypoints from the application should assert(!in_hook);
> + *
> + * During the hook call - including while the arguments are being
> + * evaluated - ev->nexus is guaranteed to be valid and refer to the
> + * nexus which is being used for this event registration. The
> + * arguments should specify ev->nexus for the for_libxl argument and
> + * ev->nexus->for_app_reg (or a pointer to it) for for_app_reg.
> */
> -#define OSEVENT_HOOK_INTERN(retval, hookname, ...) do {
> \
> - if (CTX->osevent_hooks) {
> \
> - CTX->osevent_in_hook++;
> \
> - retval CTX->osevent_hooks->hookname(CTX->osevent_user, __VA_ARGS__);
> \
> - CTX->osevent_in_hook--;
> \
> - }
> \
> +#define OSEVENT_HOOK_INTERN(retval, failedp, evkind, hookop, nexusop, ...)
> do { \
> + if (CTX->osevent_hooks) { \
> + CTX->osevent_in_hook++; \
> + libxl__osevent_hook_nexi *nexi = &CTX->hook_##evkind##_nexi_idle; \
> + osevent_hook_pre_##nexusop(gc, ev, nexi, &ev->nexus); \
> + retval CTX->osevent_hooks->evkind##_##hookop \
> + (CTX->osevent_user, __VA_ARGS__); \
> + if ((failedp)) \
> + osevent_hook_failed_##nexusop(gc, ev, nexi, &ev->nexus); \
> + CTX->osevent_in_hook--; \
> + } \
> } while (0)
>
> -#define OSEVENT_HOOK(hookname, ...) ({
> \
> - int osevent_hook_rc = 0;
> \
> - OSEVENT_HOOK_INTERN(osevent_hook_rc = , hookname, __VA_ARGS__);
> \
> - osevent_hook_rc;
> \
> +#define OSEVENT_HOOK(evkind, hookop, nexusop, ...) ({ \
> + int osevent_hook_rc = 0; \
> + OSEVENT_HOOK_INTERN(osevent_hook_rc =, !!osevent_hook_rc, \
> + evkind, hookop, nexusop, __VA_ARGS__); \
> + osevent_hook_rc; \
> })
>
> -#define OSEVENT_HOOK_VOID(hookname, ...) \
> - OSEVENT_HOOK_INTERN(/* void */, hookname, __VA_ARGS__)
> +#define OSEVENT_HOOK_VOID(evkind, hookop, nexusop, ...)
> \
> + OSEVENT_HOOK_INTERN(/* void */, 0, evkind, hookop, nexusop, __VA_ARGS__)
> +
> +/*
> + * The application's calls to libxl_osevent_occurred_... may be
> + * indefinitely delayed with respect to the rest of the program (since
> + * they are not necessarily called with any lock held). So the
> + * for_libxl value we receive may be (almost) arbitrarily old. All we
> + * know is that it came from this ctx.
> + *
> + * Therefore we may not free the object referred to by any for_libxl
> + * value until we free the whole libxl_ctx. And if we reuse it we
> + * must be able to tell when an old use turns up, and discard the
> + * stale event.
> + *
> + * Thus we cannot use the ev directly as the for_libxl value - we need
> + * a layer of indirection.
> + *
> + * We do this by keeping a pool of libxl__osevent_hook_nexus structs,
> + * and use pointers to them as for_libxl values. In fact, there are
> + * two pools: one for fds and one for timeouts. This ensures that we
> + * don't risk a type error when we upcast nexus->ev. In each nexus
> + * the ev is either null or points to a valid libxl__ev_time or
> + * libxl__ev_fd, as applicable.
> + *
> + * We /do/ allow ourselves to reassociate an old nexus with a new ev
> + * as otherwise we would have to leak nexi. (This reassociation
> + * might, of course, be an old ev being reused for a new purpose so
> + * simply comparing the ev pointer is not sufficient.) Thus the
> + * libxl_osevent_occurred functions need to check that the condition
> + * allegedly signalled by this event actually exists.
> + *
> + * The nexi and the lists are all protected by the ctx lock.
> + */
> +
> +struct libxl__osevent_hook_nexus {
> + void *ev;
> + void *for_app_reg;
> + LIBXL_SLIST_ENTRY(libxl__osevent_hook_nexus) next;
> +};
> +
> +static void *osevent_ev_from_hook_nexus(libxl_ctx *ctx,
> + libxl__osevent_hook_nexus *nexus /* pass void *for_libxl */)
> +{
> + return nexus->ev;
> +}
> +
> +static void osevent_release_nexus(libxl__gc *gc,
> + libxl__osevent_hook_nexi *nexi_idle,
> + libxl__osevent_hook_nexus *nexus)
> +{
> + nexus->ev = 0;
> + LIBXL_SLIST_INSERT_HEAD(nexi_idle, nexus, next);
> +}
> +
> +/*----- OSEVENT* hook functions for nexusop "alloc" -----*/
> +static void osevent_hook_pre_alloc(libxl__gc *gc, void *ev,
> + libxl__osevent_hook_nexi *nexi_idle,
> + libxl__osevent_hook_nexus **nexus_r)
> +{
> + libxl__osevent_hook_nexus *nexus = LIBXL_SLIST_FIRST(nexi_idle);
> + if (nexus) {
> + LIBXL_SLIST_REMOVE_HEAD(nexi_idle, next);
> + } else {
> + nexus = libxl__zalloc(NOGC, sizeof(*nexus));
> + }
> + nexus->ev = ev;
> + *nexus_r = nexus;
> +}
> +static void osevent_hook_failed_alloc(libxl__gc *gc, void *ev,
> + libxl__osevent_hook_nexi *nexi_idle,
> + libxl__osevent_hook_nexus **nexus)
> +{
> + osevent_release_nexus(gc, nexi_idle, *nexus);
> +}
> +
> +/*----- OSEVENT* hook functions for nexusop "release" -----*/
> +static void osevent_hook_pre_release(libxl__gc *gc, void *ev,
> + libxl__osevent_hook_nexi *nexi_idle,
> + libxl__osevent_hook_nexus **nexus)
> +{
> + osevent_release_nexus(gc, nexi_idle, *nexus);
> +}
> +static void osevent_hook_failed_release(libxl__gc *gc, void *ev,
> + libxl__osevent_hook_nexi *nexi_idle,
> + libxl__osevent_hook_nexus **nexus)
> +{
> + abort();
> +}
> +
> +/*----- OSEVENT* hook functions for nexusop "noop" -----*/
> +static void osevent_hook_pre_noop(libxl__gc *gc, void *ev,
> + libxl__osevent_hook_nexi *nexi_idle,
> + libxl__osevent_hook_nexus **nexus) { }
> +static void osevent_hook_failed_noop(libxl__gc *gc, void *ev,
> + libxl__osevent_hook_nexi *nexi_idle,
> + libxl__osevent_hook_nexus **nexus) { }
> +
>
> /*
> * fd events
> @@ -72,7 +180,8 @@ int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev,
>
> DBG("ev_fd=%p register fd=%d events=%x", ev, fd, events);
>
> - rc = OSEVENT_HOOK(fd_register, fd, &ev->for_app_reg, events, ev);
> + rc = OSEVENT_HOOK(fd,register, alloc, fd, &ev->nexus->for_app_reg,
>
Nit, should there be a space between 'fd,' and 'register'? Also, not
that gcc complained, but register is a keyword.
> + events, ev->nexus);
> if (rc) goto out;
>
> ev->fd = fd;
> @@ -97,7 +206,7 @@ int libxl__ev_fd_modify(libxl__gc *gc, libxl__ev_fd *ev,
> short events)
>
> DBG("ev_fd=%p modify fd=%d events=%x", ev, ev->fd, events);
>
> - rc = OSEVENT_HOOK(fd_modify, ev->fd, &ev->for_app_reg, events);
> + rc = OSEVENT_HOOK(fd,modify, noop, ev->fd, &ev->nexus->for_app_reg,
> events);
>
Same nit here, and in a few other uses of OSEVENT_HOOK* below.
Regards,
Jim
> if (rc) goto out;
>
> ev->events = events;
> @@ -119,7 +228,7 @@ void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd
> *ev)
>
> DBG("ev_fd=%p deregister fd=%d", ev, ev->fd);
>
> - OSEVENT_HOOK_VOID(fd_deregister, ev->fd, ev->for_app_reg);
> + OSEVENT_HOOK_VOID(fd,deregister, release, ev->fd,
> ev->nexus->for_app_reg);
> LIBXL_LIST_REMOVE(ev, entry);
> ev->fd = -1;
>
> @@ -171,7 +280,8 @@ static int time_register_finite(libxl__gc *gc,
> libxl__ev_time *ev,
> {
> int rc;
>
> - rc = OSEVENT_HOOK(timeout_register, &ev->for_app_reg, absolute, ev);
> + rc = OSEVENT_HOOK(timeout,register, alloc, &ev->nexus->for_app_reg,
> + absolute, ev->nexus);
> if (rc) return rc;
>
> ev->infinite = 0;
> @@ -184,7 +294,7 @@ static int time_register_finite(libxl__gc *gc,
> libxl__ev_time *ev,
> static void time_deregister(libxl__gc *gc, libxl__ev_time *ev)
> {
> if (!ev->infinite) {
> - OSEVENT_HOOK_VOID(timeout_deregister, ev->for_app_reg);
> + OSEVENT_HOOK_VOID(timeout,deregister, release,
> ev->nexus->for_app_reg);
> LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
> }
> }
> @@ -270,7 +380,8 @@ int libxl__ev_time_modify_abs(libxl__gc *gc,
> libxl__ev_time *ev,
> rc = time_register_finite(gc, ev, absolute);
> if (rc) goto out;
> } else {
> - rc = OSEVENT_HOOK(timeout_modify, &ev->for_app_reg, absolute);
> + rc = OSEVENT_HOOK(timeout,modify, noop,
> + &ev->nexus->for_app_reg, absolute);
> if (rc) goto out;
>
> LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
> @@ -1010,35 +1121,54 @@ void libxl_osevent_register_hooks(libxl_ctx *ctx,
>
>
> void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
> - int fd, short events, short revents)
> + int fd, short events_ign, short revents_ign)
> {
> - libxl__ev_fd *ev = for_libxl;
> -
> EGC_INIT(ctx);
> CTX_LOCK;
> assert(!CTX->osevent_in_hook);
>
> - assert(fd == ev->fd);
> - revents &= ev->events;
> - if (revents)
> - ev->func(egc, ev, fd, ev->events, revents);
> + libxl__ev_fd *ev = osevent_ev_from_hook_nexus(ctx, for_libxl);
> + if (!ev) goto out;
> + if (ev->fd != fd) goto out;
>
> + struct pollfd check;
> + for (;;) {
> + check.fd = fd;
> + check.events = ev->events;
> + int r = poll(&check, 1, 0);
> + if (!r)
> + goto out;
> + if (r==1)
> + break;
> + assert(r<0);
> + if (errno != EINTR) {
> + LIBXL__EVENT_DISASTER(egc, "failed poll to check for fd", errno,
> 0);
> + goto out;
> + }
> + }
> +
> + if (check.revents)
> + ev->func(egc, ev, fd, ev->events, check.revents);
> +
> + out:
> CTX_UNLOCK;
> EGC_FREE;
> }
>
> void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl)
> {
> - libxl__ev_time *ev = for_libxl;
> -
> EGC_INIT(ctx);
> CTX_LOCK;
> assert(!CTX->osevent_in_hook);
>
> + libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, for_libxl);
> + if (!ev) goto out;
> assert(!ev->infinite);
> +
> LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
> ev->func(egc, ev, &ev->abs);
>
> + out:
> CTX_UNLOCK;
> EGC_FREE;
> }
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index cba3616..6484bcb 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -136,6 +136,8 @@ typedef struct libxl__gc libxl__gc;
> typedef struct libxl__egc libxl__egc;
> typedef struct libxl__ao libxl__ao;
> typedef struct libxl__aop_occurred libxl__aop_occurred;
> +typedef struct libxl__osevent_hook_nexus libxl__osevent_hook_nexus;
> +typedef struct libxl__osevent_hook_nexi libxl__osevent_hook_nexi;
>
> _hidden void libxl__alloc_failed(libxl_ctx *, const char *func,
> size_t nmemb, size_t size)
> __attribute__((noreturn));
> @@ -163,7 +165,7 @@ struct libxl__ev_fd {
> libxl__ev_fd_callback *func;
> /* remainder is private for libxl__ev_fd... */
> LIBXL_LIST_ENTRY(libxl__ev_fd) entry;
> - void *for_app_reg;
> + libxl__osevent_hook_nexus *nexus;
> };
>
>
> @@ -178,7 +180,7 @@ struct libxl__ev_time {
> int infinite; /* not registered in list or with app if infinite */
> LIBXL_TAILQ_ENTRY(libxl__ev_time) entry;
> struct timeval abs;
> - void *for_app_reg;
> + libxl__osevent_hook_nexus *nexus;
> };
>
> typedef struct libxl__ev_xswatch libxl__ev_xswatch;
> @@ -329,6 +331,8 @@ struct libxl__ctx {
> libxl__poller poller_app; /* libxl_osevent_beforepoll and _afterpoll */
> LIBXL_LIST_HEAD(, libxl__poller) pollers_event, pollers_idle;
>
> + LIBXL_SLIST_HEAD(libxl__osevent_hook_nexi, libxl__osevent_hook_nexus)
> + hook_fd_nexi_idle, hook_timeout_nexi_idle;
> LIBXL_LIST_HEAD(, libxl__ev_fd) efds;
> LIBXL_TAILQ_HEAD(, libxl__ev_time) etimes;
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |