Bamvor Jian Zhang
2012-Nov-20 07:16 UTC
[PATCH] fix race condition between libvirtd event handling and libxl fd deregister
the race condition may be encounted at the following senaro:
(1), xenlight will remove fd handle just after the transfer is done according to
the buffer pointer. This action will first mark fd handle deleted in libvirtd
then remove fd handler from list in libxl. To mark the fd deleted in libvirtd,
the libvirt event loop mutex must be acquired.
(2), meanwhile in the libvirt event dispatch process, libvirt will check the fd
deleted flag while holding the event loop mutex. At this time, "(1)"
may be
blocked from marking the deleted flag. Then libvirt release its mutex temperary
to run the dispatcher callback. But this callback need xenlight lock(CTX_LOCK)
which is held by xenlight fd deregister function. So, libvirtd will continue to
run this callback after fd deregister exit which means xenlight has been marked
deleted flag, removed this fd handler and set ev->fd as -1. after
libxl__ev_fd_deregister exit, it is time for callback running. but
unfortunately, this callback has been removed as I mentioned above.
reference the following graph:
libvirt event dispatch xenlight transfer done
| enter libxl__ev_fd_deregister
| CTX_LOCK
| |
| |
| enter osevent_fd_deregister
| |
| enter virEventRemoveHandle
| waiting virMutexLock
check handler delete flag |
virMutexUnlock |
| virMutexLock
enter libxl_osevent_occurred_fd |
waiting CTX_LOCK mark delete flag
| virMutexUnlock
| |
| exit virEventRemoveHandle
| exit osevent_fd_deregister
| |
| remove fd handler from list
| set ev->fd as -1
| CTX_UNLOCK
CTX_LOCK
assert(fd==ev->fd) //lead to crash
call back in libxl
CTX_UNLOCK
exit libxl_osevent_occurred_fd
at the same time, i found the times of file handler register is less than the
times of file handler deregister. is that right? seems that it will be better if
the register and deregister is paired.
Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
diff -r 321f8487379b -r 0451e6041bdd tools/libxl/libxl_event.c
--- a/tools/libxl/libxl_event.c Thu Nov 15 10:25:29 2012 +0000
+++ b/tools/libxl/libxl_event.c Tue Nov 20 14:56:04 2012 +0800
@@ -1018,11 +1018,15 @@ void libxl_osevent_occurred_fd(libxl_ctx
CTX_LOCK;
assert(!CTX->osevent_in_hook);
+ if (!libxl__ev_fd_isregistered(ev)) {
+ DBG("ev_fd=%p deregister unregistered",ev);
+ goto out;
+ }
assert(fd == ev->fd);
revents &= ev->events;
if (revents)
ev->func(egc, ev, fd, ev->events, revents);
-
+out:
CTX_UNLOCK;
EGC_FREE;
}
Ian Campbell
2012-Nov-23 09:43 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
On Tue, 2012-11-20 at 07:16 +0000, Bamvor Jian Zhang wrote:> the race condition may be encounted at the following senaro: > (1), xenlight will remove fd handle just after the transfer is done according to > the buffer pointer. This action will first mark fd handle deleted in libvirtd > then remove fd handler from list in libxl. To mark the fd deleted in libvirtd, > the libvirt event loop mutex must be acquired. > > (2), meanwhile in the libvirt event dispatch process, libvirt will check the fd > deleted flag while holding the event loop mutex. At this time, "(1)" may be > blocked from marking the deleted flag. Then libvirt release its mutex temperary > to run the dispatcher callback. But this callback need xenlight lock(CTX_LOCK) > which is held by xenlight fd deregister function. So, libvirtd will continue to > run this callback after fd deregister exit which means xenlight has been marked > deleted flag, removed this fd handler and set ev->fd as -1. after > libxl__ev_fd_deregister exit, it is time for callback running. but > unfortunately, this callback has been removed as I mentioned above. > > reference the following graph: > libvirt event dispatch xenlight transfer done > | enter libxl__ev_fd_deregister > | CTX_LOCK > | | > | | > | enter osevent_fd_deregister > | | > | enter virEventRemoveHandle > | waiting virMutexLock > check handler delete flag | > virMutexUnlock | > | virMutexLock > enter libxl_osevent_occurred_fd | > waiting CTX_LOCK mark delete flag > | virMutexUnlock > | | > | exit virEventRemoveHandle > | exit osevent_fd_deregister > | | > | remove fd handler from list > | set ev->fd as -1 > | CTX_UNLOCK > CTX_LOCK > assert(fd==ev->fd) //lead to crash > call back in libxl > CTX_UNLOCK > exit libxl_osevent_occurred_fdThis all seems pretty plausible to me, but I''d like to have an Ack from Ian J before I commit.> at the same time, i found the times of file handler register is less > than the times of file handler deregister. is that right? seems that > it will be better if the register and deregister is paired.How many extra file handles are we talking about? Presumably some long lived file handles will stay around until you call libxl_ctx_free for the ctx. Or are you doing this and saying you are seeing leaked fd''s?> Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>> > diff -r 321f8487379b -r 0451e6041bdd tools/libxl/libxl_event.c > --- a/tools/libxl/libxl_event.c Thu Nov 15 10:25:29 2012 +0000 > +++ b/tools/libxl/libxl_event.c Tue Nov 20 14:56:04 2012 +0800 > @@ -1018,11 +1018,15 @@ void libxl_osevent_occurred_fd(libxl_ctx > CTX_LOCK; > assert(!CTX->osevent_in_hook); > > + if (!libxl__ev_fd_isregistered(ev)) { > + DBG("ev_fd=%p deregister unregistered",ev); > + goto out; > + } > assert(fd == ev->fd); > revents &= ev->events; > if (revents) > ev->func(egc, ev, fd, ev->events, revents); > - > +out: > CTX_UNLOCK; > EGC_FREE; > } > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Jim Fehlig
2012-Nov-26 23:22 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Ian Campbell wrote:> On Tue, 2012-11-20 at 07:16 +0000, Bamvor Jian Zhang wrote: > >> the race condition may be encounted at the following senaro: >> (1), xenlight will remove fd handle just after the transfer is done according to >> the buffer pointer. This action will first mark fd handle deleted in libvirtd >> then remove fd handler from list in libxl. To mark the fd deleted in libvirtd, >> the libvirt event loop mutex must be acquired. >> >> (2), meanwhile in the libvirt event dispatch process, libvirt will check the fd >> deleted flag while holding the event loop mutex. At this time, "(1)" may be >> blocked from marking the deleted flag. Then libvirt release its mutex temperary >> to run the dispatcher callback. But this callback need xenlight lock(CTX_LOCK) >> which is held by xenlight fd deregister function. So, libvirtd will continue to >> run this callback after fd deregister exit which means xenlight has been marked >> deleted flag, removed this fd handler and set ev->fd as -1. after >> libxl__ev_fd_deregister exit, it is time for callback running. but >> unfortunately, this callback has been removed as I mentioned above. >> >> reference the following graph: >> libvirt event dispatch xenlight transfer done >> | enter libxl__ev_fd_deregister >> | CTX_LOCK >> | | >> | | >> | enter osevent_fd_deregister >> | | >> | enter virEventRemoveHandle >> | waiting virMutexLock >> check handler delete flag | >> virMutexUnlock | >> | virMutexLock >> enter libxl_osevent_occurred_fd | >> waiting CTX_LOCK mark delete flag >> | virMutexUnlock >> | | >> | exit virEventRemoveHandle >> | exit osevent_fd_deregister >> | | >> | remove fd handler from list >> | set ev->fd as -1 >> | CTX_UNLOCK >> CTX_LOCK >> assert(fd==ev->fd) //lead to crash >> call back in libxl >> CTX_UNLOCK >> exit libxl_osevent_occurred_fd >> > > This all seems pretty plausible to me, but I''d like to have an Ack from > Ian J before I commit. >FYI, I hit the race again today on a test machine without this patch, and no longer encountered the race after applying the patch. So Tested-by: Jim Fehlig <jfehlig@suse.com> If ACK''ed by Ian J., can this also be added to 4.2-testing? Thanks! Jim> >> at the same time, i found the times of file handler register is less >> than the times of file handler deregister. is that right? seems that >> it will be better if the register and deregister is paired. >> > > How many extra file handles are we talking about? > > Presumably some long lived file handles will stay around until you call > libxl_ctx_free for the ctx. Or are you doing this and saying you are > seeing leaked fd''s? > > >> Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com> >> > > >> diff -r 321f8487379b -r 0451e6041bdd tools/libxl/libxl_event.c >> --- a/tools/libxl/libxl_event.c Thu Nov 15 10:25:29 2012 +0000 >> +++ b/tools/libxl/libxl_event.c Tue Nov 20 14:56:04 2012 +0800 >> @@ -1018,11 +1018,15 @@ void libxl_osevent_occurred_fd(libxl_ctx >> CTX_LOCK; >> assert(!CTX->osevent_in_hook); >> >> + if (!libxl__ev_fd_isregistered(ev)) { >> + DBG("ev_fd=%p deregister unregistered",ev); >> + goto out; >> + } >> assert(fd == ev->fd); >> revents &= ev->events; >> if (revents) >> ev->func(egc, ev, fd, ev->events, revents); >> - >> +out: >> CTX_UNLOCK; >> EGC_FREE; >> } >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel >> > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > >
Ian Campbell
2012-Nov-27 10:03 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
On Mon, 2012-11-26 at 23:22 +0000, Jim Fehlig wrote:> FYI, I hit the race again today on a test machine without this patch, > and no longer encountered the race after applying the patch. So > > Tested-by: Jim Fehlig <jfehlig@suse.com> > > If ACK''ed by Ian J., can this also be added to 4.2-testing?IMHO, Yes, absolutely. Ian.
Ian Jackson
2012-Nov-27 19:08 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Bamvor Jian Zhang writes ("[PATCH] fix race condition between libvirtd
event handling and libxl fd deregister"):> the race condition may be encounted at the following senaro:
Thanks for this report. You are correct that there is a race here.
Unfortunately it''s more complicated than your analysis reveals and I
think your proposed fix is not sufficient.
I have worked up a patch - see below - which I think fixes most of the
problem. Please see the commit message and the big new comment just
after the definition of OSEVENT_HOOK_VOID for details.
But I think there may be one serious problem remaining. It seems to
me that entry to libxl__osevent_occurred_timeout can race with the
hook timeout_deregister. Specifically, the API presented by libxl
does not allow libxl to tell whether a call to
libxl__osevent_occurred_timeout is one which was entered before a call
to libxl__ev_time_deregister and thence timeout_deregister.
Let us suppose that the previous timeout registration is similar to
the new one, but has a different for_app_reg value.
If the call to libxl__osevent_occurred_timeout was entered a long time
ago then it refers to the old timeout registration and the current
timeout registration is not affected, and must still be deregistered if
necessary. Not deregistering it would leak memory.
If the call was entered "recently" it refers to the current timeout
registration. That means that the current timeout for_app_reg value
(the libvirt event request) has been freed and deregistering it would
be a memory management error.
Bamvor, do you agree with this analysis ? If so, what change to the
libxl API syntax or semantics would fix the problem ?
I see that the notion that a timeout event registration by libxl with
the application is automatic deregistered is not expressed clearly in
libxl.h. So perhaps one fix would be to decree that libxl is supposed
to call timeout_deregister from within
libxl__osevent_occurred_timeout if it wasn''t called before.
Thanks,
Ian.
From: Ian Jackson <ian.jackson@eu.citrix.com>
Subject: [PATCH] RFC: libxl: fix stale fd/timeout event callback race
DO NOT APPLY
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.
Document the problem and the solution in a comment in libxl_event.c
just before the definition of struct libxl__osevent_hook_nexus.
Reported-by: Bamvor Jian Zhang <bjzhang@suse.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_event.c | 188 +++++++++++++++++++++++++++++++++++------
tools/libxl/libxl_internal.h | 8 ++-
2 files changed, 166 insertions(+), 30 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 72cb723..7c8719d 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -38,23 +38,123 @@
* 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, ...) do { \
+ if (CTX->osevent_hooks) { \
+ CTX->osevent_in_hook++; \
+ libxl__osevent_hook_nexi *nexi =
&CTX->hook_##evkind##_nexi_idle; \
+ osevent_hook_pre_##hookop(gc, ev, nexi, &ev->nexus);
\
+ retval CTX->osevent_hooks->evkind##_##hookop \
+ (CTX->osevent_user, __VA_ARGS__); \
+ if ((failedp)) \
+ osevent_hook_failed_##hookop(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, ...) ({ \
+ int osevent_hook_rc = 0; \
+ OSEVENT_HOOK_INTERN(osevent_hook_rc =, !!osevent_hook_rc, \
+ evkind, hookop, __VA_ARGS__); \
+ osevent_hook_rc; \
})
-#define OSEVENT_HOOK_VOID(hookname, ...) \
- OSEVENT_HOOK_INTERN(/* void */, hookname, __VA_ARGS__)
+#define OSEVENT_HOOK_VOID(evkind, hookop, ...) \
+ OSEVENT_HOOK_INTERN(/* void */, 0, evkind, hookop, __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_hook_pre_register(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_pre_deregister(libxl__gc *gc, void *ev,
+ libxl__osevent_hook_nexi *nexi_idle,
+ libxl__osevent_hook_nexus **nexus)
+{
+ (*nexus)->ev = 0;
+ LIBXL_SLIST_INSERT_HEAD(nexi_idle, *nexus, next);
+}
+static void osevent_hook_pre_modify(libxl__gc *gc, void *ev,
+ libxl__osevent_hook_nexi *nexi_idle,
+ libxl__osevent_hook_nexus **nexus)
+{
+}
+
+static void osevent_hook_failed_register(libxl__gc *gc, void *ev,
+ libxl__osevent_hook_nexi *nexi_idle,
+ libxl__osevent_hook_nexus **nexus)
+{
+ osevent_hook_pre_deregister(gc, ev, nexi_idle, nexus);
+}
+static void osevent_hook_failed_deregister(libxl__gc *gc, void *ev,
+ libxl__osevent_hook_nexi *nexi_idle,
+ libxl__osevent_hook_nexus **nexus)
+{
+ abort();
+}
+static void osevent_hook_failed_modify(libxl__gc *gc, void *ev,
+ libxl__osevent_hook_nexi *nexi_idle,
+ libxl__osevent_hook_nexus **nexus)
+{
+}
+
+static void *osevent_ev_from_hook_nexus(libxl_ctx *ctx, void *for_libxl)
+{
+ libxl__osevent_hook_nexus *nexus = for_libxl;
+ return nexus->ev;
+}
/*
* fd events
@@ -72,7 +172,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, fd, &ev->nexus->for_app_reg,
+ events, ev->nexus);
if (rc) goto out;
ev->fd = fd;
@@ -97,7 +198,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, ev->fd, &ev->nexus->for_app_reg,
events);
if (rc) goto out;
ev->events = events;
@@ -119,7 +220,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, ev->fd, ev->nexus->for_app_reg);
LIBXL_LIST_REMOVE(ev, entry);
ev->fd = -1;
@@ -171,7 +272,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, &ev->nexus->for_app_reg,
+ absolute, ev->nexus);
if (rc) return rc;
ev->infinite = 0;
@@ -184,7 +286,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, ev->nexus->for_app_reg);
LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
}
}
@@ -270,7 +372,7 @@ 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, &ev->nexus->for_app_reg,
absolute);
if (rc) goto out;
LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
@@ -1010,35 +1112,65 @@ 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);
- assert(!ev->infinite);
+ libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, for_libxl);
+ if (!ev) goto out;
+ if (ev->infinite) goto out;
+
+ struct timeval now;
+ int r = libxl__gettimeofday(gc, &now);
+ if (r)
+ /* probably(?) safer to risk a race causing a time event to
+ * happen early than to ignore the event and call DISASTER */
+ goto out;
+
+ if (timercmp(&ev->abs, &now, >))
+ /* lost the race - this is a stale timer event callback */
+ goto out;
+
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;
--
tg: (bfe7025..) t/xen/xl.eventfix2.fdreg.indirect (depends on: base)
Ian Campbell
2012-Nov-28 11:25 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
On Tue, 2012-11-27 at 19:08 +0000, Ian Jackson wrote:> what change to the libxl API syntax or semantics would fix the problem ?Mostly just thinking out loud here... Can we provide, or (more likely) require the application to provide, a lock (perhaps per-event or, again more likely, per-event-loop) which must be held while processing callbacks and also while events are being registered/unregistered with the application''s event handling subsystem? With such a lock in place the application would be able to guarantee that having returned from the deregister hook any further events would be seen as spurious events by its own event processing loop. The other scheme which springs to mind is to do reference counting, with the application holding a reference whenever the event is present in its event loop (such that there is any chance of the event being generated) and libxl holding a reference while it considers the event to be active (which roughly corresponds to the existing register/unregister hook calls, I think). libxl would drop its reference as part of calling the deregister hook while the application would hold its reference until it was certain the event would no longer be generated e.g. in a pass at the top of its event handling loop where it includes or excludes the relevant fd from its select()/poll(). Last half brained idea would be to split the deregistration into two. libxl calls up to the app saying "please deregister" and the app calls back to libxl to say "I am no longer watching for this event and guarantee that I won''t deliver it any more". (Presumably this would be implemented by the application via some combination of the above). This could be done in a somewhat compatible way by allowing the deregister hook to return "PENDING". Ian.
Bamvor Jian Zhang
2012-Dec-06 16:06 UTC
答复： Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Hi, Ian Thanks your comments, I do not think it deeply before it. I am not sure about your "remaining problem". I guess this problem should not happen, Because the different registration should refer to different ev(lixl__ev_time, lixl__ev_fd). About the race condition, I still not understand why it occurred. Libvirt should not got the event while libxl thought such event is useless. Currently, fd is deregister twice, the first time is just after transfer done and second time is during cleanup function such as bootloader cleanup. If we only do it in the cleanup functions, this race should not happen.>>> Ian Jackson <Ian.Jackson@eu.citrix.com> 12年11月28日 上午 3:08 >>>Bamvor Jian Zhang writes ("[PATCH] fix race condition between libvirtd event handling and libxl fd deregister"):> the race condition may be encounted at the following senaro:Thanks for this report. You are correct that there is a race here. Unfortunately it's more complicated than your analysis reveals and I think your proposed fix is not sufficient. I have worked up a patch - see below - which I think fixes most of the problem. Please see the commit message and the big new comment just after the definition of OSEVENT_HOOK_VOID for details. But I think there may be one serious problem remaining. It seems to me that entry to libxl__osevent_occurred_timeout can race with the hook timeout_deregister. Specifically, the API presented by libxl does not allow libxl to tell whether a call to libxl__osevent_occurred_timeout is one which was entered before a call to libxl__ev_time_deregister and thence timeout_deregister. Let us suppose that the previous timeout registration is similar to the new one, but has a different for_app_reg value. If the call to libxl__osevent_occurred_timeout was entered a long time ago then it refers to the old timeout registration and the current timeout registration is not affected, and must still be deregistered if necessary. Not deregistering it would leak memory. If the call was entered "recently" it refers to the current timeout registration. That means that the current timeout for_app_reg value (the libvirt event request) has been freed and deregistering it would be a memory management error. Bamvor, do you agree with this analysis ? If so, what change to the libxl API syntax or semantics would fix the problem ? I see that the notion that a timeout event registration by libxl with the application is automatic deregistered is not expressed clearly in libxl.h. So perhaps one fix would be to decree that libxl is supposed to call timeout_deregister from within libxl__osevent_occurred_timeout if it wasn't called before. Thanks, Ian. From: Ian Jackson <ian.jackson@eu.citrix.com> Subject: [PATCH] RFC: libxl: fix stale fd/timeout event callback race DO NOT APPLY 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. Document the problem and the solution in a comment in libxl_event.c just before the definition of struct libxl__osevent_hook_nexus. Reported-by: Bamvor Jian Zhang <bjzhang@suse.com> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> --- tools/libxl/libxl_event.c | 188 +++++++++++++++++++++++++++++++++++------ tools/libxl/libxl_internal.h | 8 ++- 2 files changed, 166 insertions(+), 30 deletions(-) diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c index 72cb723..7c8719d 100644 --- a/tools/libxl/libxl_event.c +++ b/tools/libxl/libxl_event.c @@ -38,23 +38,123 @@ * 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); + * + * Dur+ * 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, ...) do { \ + if (CTX->osevent_hooks) { \ + CTX->osevent_in_hook++; \ + libxl__osevent_hook_nexi *nexi = &CTX->hook_##evkind##_nexi_idle; \ + osevent_hook_pre_##hookop(gc, ev, nexi, &ev->nexus); \ + retval CTX->osevent_hooks->evkind##_##hookop \ + (CTX->osevent_user, __VA_ARGS__); \ + if ((failedp)) \ + osevent_hook_failed_##hookop(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, ...) ({ \ + int osevent_hook_rc = 0; \ + OSEVENT_HOOK_INTERN(osevent_hook_rc =, !!osevent_hook_rc, \ + evkind, hookop, __VA_ARGS__); \ + osevent_hook_rc; \ }) -#define OSEVENT_HOOK_VOID(hookname, ...) \ - OSEVENT_HOOK_INTERN(/* void */, hookname, __VA_ARGS__) +#define OSEVENT_HOOK_VOID(evkind, hookop, ...) \ + OSEVENT_HOOK_INTERN(/* void */, 0, evkind, hookop, __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 { + LIBXL_SLIST_ENTRY(libxl__osevent_hook_nexus) next; +}; + +static void osevent_hook_pre_register(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_pre_deregister(libxl__gc *gc, void *ev, + libxl__osevent_hook_nexi *nexi_idle, + libxl__osevent_hook_nexus **nexus) +{ + (*nexus)->ev = 0; + LIBXL_SLIST_INSERT_HEAD(nexi_idle, *nexus, next); +} +static void osevent_hook_pre_modify(libxl__gc *gc, void *ev, + libxl__osevent_hook_nexi *nexi_idle, + libxl__osevent_hook_nexus **nexus) +{ +} + +static void osevent_hook_failed_register(libxl__gc *gc, void *ev, + libxl__osevent_hook_nexi *nexi_idle, + libxl__osevent_hook_nexus **nexus) +{ + osevent_hook_pre_deregister(gc, ev, nexi_idle, nexus); +} +static void osevent_hook_failed_deregister(libxl__gc *gc, void *ev, + libxl__osevent_hook_nexi *nexi_idle, + libxl__osevent_hook_nexus **nexus) +{ + abort(); +} +static void osevent_hook_failed_modify(libxl__gc *gc, void *ev, + libxl__osevent_hook_nexi *nexi_idle, + libxl__osevent_hook_nexus **nexus) +{ +} + +static void *osevent_ev_from_hook_nexus(libxl_ctx *ctx, void *for_libxl) +{ + libxl__osevent_hook_nexus *nexus = for_libxl; + return nexus->ev; +} /* * fd events @@ -72,7 +172,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, fd, &ev->nexus->for_app_reg, + events, ev->nexus); if (rc) goto out; ev->fd = fd; @@ -97,7 +198,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, ev->fd, &ev->nexus->for_app_reg, events); if (rc) goto out; ev->events = events; @@ -119,7 +220,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, ev->fd, ev->nexus->for_app_reg); LIBXL_LIST_REMOVE(ev, entry); ev->fd = -1; @@ -171,7 +272,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, &ev->nexus->for_app_reg, + absolute, ev->nexus); if (rc) return rc; ev->infinite = 0; @@ -184,7 +286,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, ev->nexus->for_app_reg); LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry); } } @@ -270,7 +372,7 @@ 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( LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry); @@ -1010,35 +1112,65 @@ 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); - assert(!ev->infinite); + libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, for_libxl); + if (!ev) goto out; + if (ev->infinite) goto out; + + struct timeval now; + int r = libxl__gettimeofday(gc, &now); + if (r) + /* probably(?) safer to risk a race causing a time event to + * happen early than to ignore the event and call DISASTER */ + goto out; + + if (timercmp(&ev->abs, &now, >)) + /* lost the race - this is a stale timer event callback */ + goto out; + 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; -- tg: (bfe7025..) t/xen/xl.eventfix2.fdreg.indirect (depends on: base) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Bamvor Jian Zhang
2012-Dec-06 16:11 UTC
答复： Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
The first menthol is already provided by libvirt though libvirt mutex which is a per-event lock. And because this lock release before libxl callback entering, libxl got the chance to remove handles.>>> Ian Campbell <Ian.Campbell@citrix.com> 12年11月28日 下午 19:25 >>>On Tue, 2012-11-27 at 19:08 +0000, Ian Jackson wrote:> what change to the libxl API syntax or semantics would fix the problem ?Mostly just thinking out loud here... Can we provide, or (more likely) require the application to provide, a lock (perhaps per-event or, again more likely, per-event-loop) which must be held while processing callbacks and also while events are being registered/unregistered with the application's event handling subsystem? With such a lock in place the application would be able to guarantee that having returned from the deregister hook any further events would be seen as spurious events by its own event processing loop. The other scheme which springs to mind is to do reference counting, with the application holding a reference whenever the event is present in its event loop (such that there is any chance of the event being generated) and libxl holding a reference while it considers the event to be active (which roughly corresponds to the existing register/unregister hook calls, I think). libxl would drop its reference as part of calling the deregister hook while the application would hold its reference until it was certain the event would no longer be generated e.g. in a pass at the top of its event handling loop where it includes or excludes the relevant fd from its select()/poll(). Last half brained idea would be to split the deregistration into two. libxl calls up to the app saying "please deregister" and the app calls back to libxl to say "I am no longer watching for this event and guarantee that I won't deliver it any more". (Presumably this would be implemented by the application via some combination of the above). This could be done in a somewhat compatible way by allowing the deregister hook to return "PENDING". Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Jackson
2012-Dec-07 19:11 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Ian Campbell writes ("Re: [PATCH] fix race condition between libvirtd event
handling and libxl fd deregister"):> Can we provide, or (more likely) require the application to provide, a
> lock (perhaps per-event or, again more likely, per-event-loop) which
> must be held while processing callbacks and also while events are being
> registered/unregistered with the application''s event handling
subsystem?
> With such a lock in place the application would be able to guarantee
> that having returned from the deregister hook any further events would
> be seen as spurious events by its own event processing loop.
I think this might be difficult to get right without deadlocks.
...> Last half brained idea would be to split the deregistration into two.
> libxl calls up to the app saying "please deregister" and the app
calls
> back to libxl to say "I am no longer watching for this event and
> guarantee that I won''t deliver it any more". (Presumably this
would be
> implemented by the application via some combination of the above). This
> could be done in a somewhat compatible way by allowing the deregister
> hook to return "PENDING".
This is in fact straightforward and is a subset of the existing API.
If we have libxl always call timeout_modify with abs={0,0}, it will
get timeout_occurred when the application''s event loop has dealt with
it. We can simply never call timeout_deregister.
I have implemented this in the 2-patch RFD series I''m about to send.
NB this series has been complied but not (as yet) executed by me...
Ian.
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@suse.com>
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
--
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,
+ 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);
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;
--
1.7.2.5
Ian Jackson
2012-Dec-07 19:15 UTC
[PATCH 2/2] libxl: fix stale timeout event callback race
Because there is not necessarily any lock held at the point the
application (eg, libvirt) calls libxl_osevent_occurred_timeout, in a
multithreaded program those calls may be arbitrarily delayed in
relation to other activities within the program.
Specifically this means when ->timeout_deregister returns, libxl does
not know whether it can safely dispose of the for_libxl value or
whether it needs to retain it in case of an in-progress call to
_occurred_timeout.
The interface could be fixed by requiring the application to make a
new call into libxl to say that the deregistration was complete.
However that new call would have to be threaded through the
application''s event loop; this is complicated and some application
authors are likely not to implement it properly. Furthermore the
easiest way to implement this facility in most event loops is to queue
up a time event for "now".
Shortcut all of this by having libxl always call timeout_modify
setting abs={0,0} (ie, ASAP) instead of timeout_deregister. This will
cause the application to call _occurred_timeout. When processing this
calldown we see that we were no longer actually interested and simply
throw it away.
Additionally, there is a race between _occurred_timeout and
->timeout_modify. If libxl ever adjusts the deadline for a timeout
the application may already be in the process of calling _occurred, in
which case the situation with for_app''s lifetime becomes very
complicated. Therefore abolish libxl__ev_time_modify_{abs,rel} (which
have no callers) and promise to the application only ever to call
->timeout_modify with abs=={0,0}. The application still needs to cope
with ->timeout_modify racing with its internal function which calls
_occurred_timeout. Document this.
This is a forwards-compatible change for applications using the libxl
API, and will hopefully eliminate these races in callback-supplying
applications (such as libvirt) without the need for corresponding
changes to the application.
For clarity, fold the body of time_register_finite into its one
remaining call site. This makes the semantics of ev->infinite
slightly clearer.
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_event.c | 88 +++++++--------------------------------------
tools/libxl/libxl_event.h | 17 ++++++++-
2 files changed, 28 insertions(+), 77 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index f1fe425..65c34da 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -267,18 +267,11 @@ static int time_rel_to_abs(libxl__gc *gc, int ms, struct
timeval *abs_out)
return 0;
}
-static void time_insert_finite(libxl__gc *gc, libxl__ev_time *ev)
-{
- libxl__ev_time *evsearch;
- LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch,
/*empty*/,
- timercmp(&ev->abs, &evsearch->abs,
>));
- ev->infinite = 0;
-}
-
static int time_register_finite(libxl__gc *gc, libxl__ev_time *ev,
struct timeval absolute)
{
int rc;
+ libxl__ev_time *evsearch;
rc = OSEVENT_HOOK(timeout,register, alloc,
&ev->nexus->for_app_reg,
absolute, ev->nexus);
@@ -286,7 +279,8 @@ static int time_register_finite(libxl__gc *gc,
libxl__ev_time *ev,
ev->infinite = 0;
ev->abs = absolute;
- time_insert_finite(gc, ev);
+ LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch,
/*empty*/,
+ timercmp(&ev->abs, &evsearch->abs,
>));
return 0;
}
@@ -294,7 +288,11 @@ 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, release,
ev->nexus->for_app_reg);
+ struct timeval right_away = { 0, 0 };
+ ev->nexus->ev = 0;
+ OSEVENT_HOOK_VOID(timeout,modify,
+ noop /* release nexus in _occurred_ */,
+ ev->nexus->for_app_reg, right_away);
LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
}
}
@@ -364,70 +362,6 @@ int libxl__ev_time_register_rel(libxl__gc *gc,
libxl__ev_time *ev,
return rc;
}
-int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev,
- struct timeval absolute)
-{
- int rc;
-
- CTX_LOCK;
-
- DBG("ev_time=%p modify abs==%lu.%06lu",
- ev, (unsigned long)absolute.tv_sec, (unsigned long)absolute.tv_usec);
-
- assert(libxl__ev_time_isregistered(ev));
-
- if (ev->infinite) {
- rc = time_register_finite(gc, ev, absolute);
- if (rc) goto out;
- } else {
- rc = OSEVENT_HOOK(timeout,modify, noop,
- &ev->nexus->for_app_reg, absolute);
- if (rc) goto out;
-
- LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
- ev->abs = absolute;
- time_insert_finite(gc, ev);
- }
-
- rc = 0;
- out:
- time_done_debug(gc,__func__,ev,rc);
- CTX_UNLOCK;
- return rc;
-}
-
-int libxl__ev_time_modify_rel(libxl__gc *gc, libxl__ev_time *ev,
- int milliseconds)
-{
- struct timeval absolute;
- int rc;
-
- CTX_LOCK;
-
- DBG("ev_time=%p modify ms=%d", ev, milliseconds);
-
- assert(libxl__ev_time_isregistered(ev));
-
- if (milliseconds < 0) {
- time_deregister(gc, ev);
- ev->infinite = 1;
- rc = 0;
- goto out;
- }
-
- rc = time_rel_to_abs(gc, milliseconds, &absolute);
- if (rc) goto out;
-
- rc = libxl__ev_time_modify_abs(gc, ev, absolute);
- if (rc) goto out;
-
- rc = 0;
- out:
- time_done_debug(gc,__func__,ev,rc);
- CTX_UNLOCK;
- return rc;
-}
-
void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev)
{
CTX_LOCK;
@@ -1161,7 +1095,11 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void
*for_libxl)
CTX_LOCK;
assert(!CTX->osevent_in_hook);
- libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, for_libxl);
+ libxl__osevent_hook_nexus *nexus = for_libxl;
+ libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, nexus);
+
+ osevent_release_nexus(gc, &CTX->hook_timeout_nexi_idle, nexus);
+
if (!ev) goto out;
assert(!ev->infinite);
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index 3bcb6d3..51f2721 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -287,8 +287,10 @@ typedef struct libxl_osevent_hooks {
int (*timeout_register)(void *user, void **for_app_registration_out,
struct timeval abs, void *for_libxl);
int (*timeout_modify)(void *user, void **for_app_registration_update,
- struct timeval abs);
- void (*timeout_deregister)(void *user, void *for_app_registration);
+ struct timeval abs)
+ /* only ever called with abs={0,0}, meaning ASAP */;
+ void (*timeout_deregister)(void *user, void *for_app_registration)
+ /* will never be called */;
} libxl_osevent_hooks;
/* The application which calls register_fd_hooks promises to
@@ -337,6 +339,17 @@ typedef struct libxl_osevent_hooks {
* register (or modify), and pass it to subsequent calls to modify
* or deregister.
*
+ * Note that the application must cope with a call from libxl to
+ * timeout_modify racing with its own call to
+ * libxl__osevent_occurred_timeout. libxl guarantees that
+ * timeout_modify will only be called with abs={0,0} but the
+ * application must still ensure that libxl''s attempt to cause the
+ * timeout to occur immediately is safely ignored even the timeout is
+ * actually already in the process of occurring.
+ *
+ * timeout_deregister is not used because it forms part of a
+ * deprecated unsafe mode of use of the API.
+ *
* osevent_register_hooks may be called only once for each libxl_ctx.
* libxl may make calls to register/modify/deregister from within
* any libxl function (indeed, it will usually call register from
--
1.7.2.5
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@suse.com>
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
--
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,
+ 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);
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;
--
1.7.2.5
Ian Jackson
2012-Dec-07 19:21 UTC
[PATCH 2/2] libxl: fix stale timeout event callback race
Because there is not necessarily any lock held at the point the
application (eg, libvirt) calls libxl_osevent_occurred_timeout, in a
multithreaded program those calls may be arbitrarily delayed in
relation to other activities within the program.
Specifically this means when ->timeout_deregister returns, libxl does
not know whether it can safely dispose of the for_libxl value or
whether it needs to retain it in case of an in-progress call to
_occurred_timeout.
The interface could be fixed by requiring the application to make a
new call into libxl to say that the deregistration was complete.
However that new call would have to be threaded through the
application''s event loop; this is complicated and some application
authors are likely not to implement it properly. Furthermore the
easiest way to implement this facility in most event loops is to queue
up a time event for "now".
Shortcut all of this by having libxl always call timeout_modify
setting abs={0,0} (ie, ASAP) instead of timeout_deregister. This will
cause the application to call _occurred_timeout. When processing this
calldown we see that we were no longer actually interested and simply
throw it away.
Additionally, there is a race between _occurred_timeout and
->timeout_modify. If libxl ever adjusts the deadline for a timeout
the application may already be in the process of calling _occurred, in
which case the situation with for_app''s lifetime becomes very
complicated. Therefore abolish libxl__ev_time_modify_{abs,rel} (which
have no callers) and promise to the application only ever to call
->timeout_modify with abs=={0,0}. The application still needs to cope
with ->timeout_modify racing with its internal function which calls
_occurred_timeout. Document this.
This is a forwards-compatible change for applications using the libxl
API, and will hopefully eliminate these races in callback-supplying
applications (such as libvirt) without the need for corresponding
changes to the application.
For clarity, fold the body of time_register_finite into its one
remaining call site. This makes the semantics of ev->infinite
slightly clearer.
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_event.c | 88 +++++++--------------------------------------
tools/libxl/libxl_event.h | 17 ++++++++-
2 files changed, 28 insertions(+), 77 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index f1fe425..65c34da 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -267,18 +267,11 @@ static int time_rel_to_abs(libxl__gc *gc, int ms, struct
timeval *abs_out)
return 0;
}
-static void time_insert_finite(libxl__gc *gc, libxl__ev_time *ev)
-{
- libxl__ev_time *evsearch;
- LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch,
/*empty*/,
- timercmp(&ev->abs, &evsearch->abs,
>));
- ev->infinite = 0;
-}
-
static int time_register_finite(libxl__gc *gc, libxl__ev_time *ev,
struct timeval absolute)
{
int rc;
+ libxl__ev_time *evsearch;
rc = OSEVENT_HOOK(timeout,register, alloc,
&ev->nexus->for_app_reg,
absolute, ev->nexus);
@@ -286,7 +279,8 @@ static int time_register_finite(libxl__gc *gc,
libxl__ev_time *ev,
ev->infinite = 0;
ev->abs = absolute;
- time_insert_finite(gc, ev);
+ LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch,
/*empty*/,
+ timercmp(&ev->abs, &evsearch->abs,
>));
return 0;
}
@@ -294,7 +288,11 @@ 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, release,
ev->nexus->for_app_reg);
+ struct timeval right_away = { 0, 0 };
+ ev->nexus->ev = 0;
+ OSEVENT_HOOK_VOID(timeout,modify,
+ noop /* release nexus in _occurred_ */,
+ ev->nexus->for_app_reg, right_away);
LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
}
}
@@ -364,70 +362,6 @@ int libxl__ev_time_register_rel(libxl__gc *gc,
libxl__ev_time *ev,
return rc;
}
-int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev,
- struct timeval absolute)
-{
- int rc;
-
- CTX_LOCK;
-
- DBG("ev_time=%p modify abs==%lu.%06lu",
- ev, (unsigned long)absolute.tv_sec, (unsigned long)absolute.tv_usec);
-
- assert(libxl__ev_time_isregistered(ev));
-
- if (ev->infinite) {
- rc = time_register_finite(gc, ev, absolute);
- if (rc) goto out;
- } else {
- rc = OSEVENT_HOOK(timeout,modify, noop,
- &ev->nexus->for_app_reg, absolute);
- if (rc) goto out;
-
- LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
- ev->abs = absolute;
- time_insert_finite(gc, ev);
- }
-
- rc = 0;
- out:
- time_done_debug(gc,__func__,ev,rc);
- CTX_UNLOCK;
- return rc;
-}
-
-int libxl__ev_time_modify_rel(libxl__gc *gc, libxl__ev_time *ev,
- int milliseconds)
-{
- struct timeval absolute;
- int rc;
-
- CTX_LOCK;
-
- DBG("ev_time=%p modify ms=%d", ev, milliseconds);
-
- assert(libxl__ev_time_isregistered(ev));
-
- if (milliseconds < 0) {
- time_deregister(gc, ev);
- ev->infinite = 1;
- rc = 0;
- goto out;
- }
-
- rc = time_rel_to_abs(gc, milliseconds, &absolute);
- if (rc) goto out;
-
- rc = libxl__ev_time_modify_abs(gc, ev, absolute);
- if (rc) goto out;
-
- rc = 0;
- out:
- time_done_debug(gc,__func__,ev,rc);
- CTX_UNLOCK;
- return rc;
-}
-
void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev)
{
CTX_LOCK;
@@ -1161,7 +1095,11 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void
*for_libxl)
CTX_LOCK;
assert(!CTX->osevent_in_hook);
- libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, for_libxl);
+ libxl__osevent_hook_nexus *nexus = for_libxl;
+ libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, nexus);
+
+ osevent_release_nexus(gc, &CTX->hook_timeout_nexi_idle, nexus);
+
if (!ev) goto out;
assert(!ev->infinite);
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index 3bcb6d3..51f2721 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -287,8 +287,10 @@ typedef struct libxl_osevent_hooks {
int (*timeout_register)(void *user, void **for_app_registration_out,
struct timeval abs, void *for_libxl);
int (*timeout_modify)(void *user, void **for_app_registration_update,
- struct timeval abs);
- void (*timeout_deregister)(void *user, void *for_app_registration);
+ struct timeval abs)
+ /* only ever called with abs={0,0}, meaning ASAP */;
+ void (*timeout_deregister)(void *user, void *for_app_registration)
+ /* will never be called */;
} libxl_osevent_hooks;
/* The application which calls register_fd_hooks promises to
@@ -337,6 +339,17 @@ typedef struct libxl_osevent_hooks {
* register (or modify), and pass it to subsequent calls to modify
* or deregister.
*
+ * Note that the application must cope with a call from libxl to
+ * timeout_modify racing with its own call to
+ * libxl__osevent_occurred_timeout. libxl guarantees that
+ * timeout_modify will only be called with abs={0,0} but the
+ * application must still ensure that libxl''s attempt to cause the
+ * timeout to occur immediately is safely ignored even the timeout is
+ * actually already in the process of occurring.
+ *
+ * timeout_deregister is not used because it forms part of a
+ * deprecated unsafe mode of use of the API.
+ *
* osevent_register_hooks may be called only once for each libxl_ctx.
* libxl may make calls to register/modify/deregister from within
* any libxl function (indeed, it will usually call register from
--
1.7.2.5
Ian Jackson writes ("[PATCH 1/2] libxl: fix stale fd event callback
race"):> 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.
Sorry for the duplicate; the first seemed to have vanished so I resent
it and then naturally it turned up.
Ian.
Ian Campbell
2012-Dec-10 10:19 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
On Fri, 2012-12-07 at 19:11 +0000, Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): > > Can we provide, or (more likely) require the application to provide, a > > lock (perhaps per-event or, again more likely, per-event-loop) which > > must be held while processing callbacks and also while events are being > > registered/unregistered with the application''s event handling subsystem? > > With such a lock in place the application would be able to guarantee > > that having returned from the deregister hook any further events would > > be seen as spurious events by its own event processing loop. > > I think this might be difficult to get right without deadlocks.I took Bamvor''s most recent response to mean that a per-event lock was already in place in libvirt and inferred that this was the reason why the originally proposed one line fix worked for them. Perhaps I misunderstood? Ian.
Ian Jackson
2012-Dec-10 10:37 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Ian Campbell writes ("Re: [PATCH] fix race condition between libvirtd event
handling and libxl fd deregister"):> I took Bamvor''s most recent response to mean that a per-event lock
was
> already in place in libvirt and inferred that this was the reason why
> the originally proposed one line fix worked for them. Perhaps I
> misunderstood?
Yes, I think that''s what Bamvor meant but I don''t think
it''s correct
that such a lock eliminates the race. libvirt has to release that
lock before making the callback (to follow the libxl locking rules
which are necessary to avoid deadlock).
I''m not surprised that the original patch makes Bamvor''s
symptoms go
away. Bamvor had one of the possible races (the fd-related one) but
not the other.
Ian.
Ian Jackson
2012-Dec-10 16:56 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event
handling and libxl fd deregister"):> I''m not surprised that the original patch makes Bamvor''s
symptoms go
> away. Bamvor had one of the possible races (the fd-related one) but
> not the other.
Here (followups to this message, shortly) is v3 of my two-patch series
which after conversation with Ian C I think fully fixes the race, and
which I have tested now.
Bamvor, can you test this and let us know hwether it fixes your problem?
Thanks,
Ian.
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@suse.com>
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
--
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,
+ 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);
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;
--
1.7.2.5
Ian Jackson
2012-Dec-10 16:57 UTC
[PATCH 2/2] libxl: fix stale timeout event callback race
Because there is not necessarily any lock held at the point the
application (eg, libvirt) calls libxl_osevent_occurred_timeout, in a
multithreaded program those calls may be arbitrarily delayed in
relation to other activities within the program.
Specifically this means when ->timeout_deregister returns, libxl does
not know whether it can safely dispose of the for_libxl value or
whether it needs to retain it in case of an in-progress call to
_occurred_timeout.
The interface could be fixed by requiring the application to make a
new call into libxl to say that the deregistration was complete.
However that new call would have to be threaded through the
application''s event loop; this is complicated and some application
authors are likely not to implement it properly. Furthermore the
easiest way to implement this facility in most event loops is to queue
up a time event for "now".
Shortcut all of this by having libxl always call timeout_modify
setting abs={0,0} (ie, ASAP) instead of timeout_deregister. This will
cause the application to call _occurred_timeout. When processing this
calldown we see that we were no longer actually interested and simply
throw it away.
Additionally, there is a race between _occurred_timeout and
->timeout_modify. If libxl ever adjusts the deadline for a timeout
the application may already be in the process of calling _occurred, in
which case the situation with for_app''s lifetime becomes very
complicated. Therefore abolish libxl__ev_time_modify_{abs,rel} (which
have no callers) and promise to the application only ever to call
->timeout_modify with abs=={0,0}. The application still needs to cope
with ->timeout_modify racing with its internal function which calls
_occurred_timeout. Document this.
This is a forwards-compatible change for applications using the libxl
API, and will hopefully eliminate these races in callback-supplying
applications (such as libvirt) without the need for corresponding
changes to the application.
For clarity, fold the body of time_register_finite into its one
remaining call site. This makes the semantics of ev->infinite
slightly clearer.
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
--
v3:
- Fix null pointer dereference in case when hooks not supplied.
---
tools/libxl/libxl_event.c | 89 +++++++--------------------------------------
tools/libxl/libxl_event.h | 17 ++++++++-
2 files changed, 29 insertions(+), 77 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index f1fe425..f86c528 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -267,18 +267,11 @@ static int time_rel_to_abs(libxl__gc *gc, int ms, struct
timeval *abs_out)
return 0;
}
-static void time_insert_finite(libxl__gc *gc, libxl__ev_time *ev)
-{
- libxl__ev_time *evsearch;
- LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch,
/*empty*/,
- timercmp(&ev->abs, &evsearch->abs,
>));
- ev->infinite = 0;
-}
-
static int time_register_finite(libxl__gc *gc, libxl__ev_time *ev,
struct timeval absolute)
{
int rc;
+ libxl__ev_time *evsearch;
rc = OSEVENT_HOOK(timeout,register, alloc,
&ev->nexus->for_app_reg,
absolute, ev->nexus);
@@ -286,7 +279,8 @@ static int time_register_finite(libxl__gc *gc,
libxl__ev_time *ev,
ev->infinite = 0;
ev->abs = absolute;
- time_insert_finite(gc, ev);
+ LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch,
/*empty*/,
+ timercmp(&ev->abs, &evsearch->abs,
>));
return 0;
}
@@ -294,7 +288,12 @@ 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, release,
ev->nexus->for_app_reg);
+ struct timeval right_away = { 0, 0 };
+ if (ev->nexus) /* only set if app provided hooks */
+ ev->nexus->ev = 0;
+ OSEVENT_HOOK_VOID(timeout,modify,
+ noop /* release nexus in _occurred_ */,
+ ev->nexus->for_app_reg, right_away);
LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
}
}
@@ -364,70 +363,6 @@ int libxl__ev_time_register_rel(libxl__gc *gc,
libxl__ev_time *ev,
return rc;
}
-int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev,
- struct timeval absolute)
-{
- int rc;
-
- CTX_LOCK;
-
- DBG("ev_time=%p modify abs==%lu.%06lu",
- ev, (unsigned long)absolute.tv_sec, (unsigned long)absolute.tv_usec);
-
- assert(libxl__ev_time_isregistered(ev));
-
- if (ev->infinite) {
- rc = time_register_finite(gc, ev, absolute);
- if (rc) goto out;
- } else {
- rc = OSEVENT_HOOK(timeout,modify, noop,
- &ev->nexus->for_app_reg, absolute);
- if (rc) goto out;
-
- LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
- ev->abs = absolute;
- time_insert_finite(gc, ev);
- }
-
- rc = 0;
- out:
- time_done_debug(gc,__func__,ev,rc);
- CTX_UNLOCK;
- return rc;
-}
-
-int libxl__ev_time_modify_rel(libxl__gc *gc, libxl__ev_time *ev,
- int milliseconds)
-{
- struct timeval absolute;
- int rc;
-
- CTX_LOCK;
-
- DBG("ev_time=%p modify ms=%d", ev, milliseconds);
-
- assert(libxl__ev_time_isregistered(ev));
-
- if (milliseconds < 0) {
- time_deregister(gc, ev);
- ev->infinite = 1;
- rc = 0;
- goto out;
- }
-
- rc = time_rel_to_abs(gc, milliseconds, &absolute);
- if (rc) goto out;
-
- rc = libxl__ev_time_modify_abs(gc, ev, absolute);
- if (rc) goto out;
-
- rc = 0;
- out:
- time_done_debug(gc,__func__,ev,rc);
- CTX_UNLOCK;
- return rc;
-}
-
void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev)
{
CTX_LOCK;
@@ -1161,7 +1096,11 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void
*for_libxl)
CTX_LOCK;
assert(!CTX->osevent_in_hook);
- libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, for_libxl);
+ libxl__osevent_hook_nexus *nexus = for_libxl;
+ libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, nexus);
+
+ osevent_release_nexus(gc, &CTX->hook_timeout_nexi_idle, nexus);
+
if (!ev) goto out;
assert(!ev->infinite);
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index 3bcb6d3..51f2721 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -287,8 +287,10 @@ typedef struct libxl_osevent_hooks {
int (*timeout_register)(void *user, void **for_app_registration_out,
struct timeval abs, void *for_libxl);
int (*timeout_modify)(void *user, void **for_app_registration_update,
- struct timeval abs);
- void (*timeout_deregister)(void *user, void *for_app_registration);
+ struct timeval abs)
+ /* only ever called with abs={0,0}, meaning ASAP */;
+ void (*timeout_deregister)(void *user, void *for_app_registration)
+ /* will never be called */;
} libxl_osevent_hooks;
/* The application which calls register_fd_hooks promises to
@@ -337,6 +339,17 @@ typedef struct libxl_osevent_hooks {
* register (or modify), and pass it to subsequent calls to modify
* or deregister.
*
+ * Note that the application must cope with a call from libxl to
+ * timeout_modify racing with its own call to
+ * libxl__osevent_occurred_timeout. libxl guarantees that
+ * timeout_modify will only be called with abs={0,0} but the
+ * application must still ensure that libxl''s attempt to cause the
+ * timeout to occur immediately is safely ignored even the timeout is
+ * actually already in the process of occurring.
+ *
+ * timeout_deregister is not used because it forms part of a
+ * deprecated unsafe mode of use of the API.
+ *
* osevent_register_hooks may be called only once for each libxl_ctx.
* libxl may make calls to register/modify/deregister from within
* any libxl function (indeed, it will usually call register from
--
1.7.2.5
Jim Fehlig
2012-Dec-10 23:56 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): > >> I took Bamvor''s most recent response to mean that a per-event lock was >> already in place in libvirt and inferred that this was the reason why >> the originally proposed one line fix worked for them. Perhaps I >> misunderstood? >> > > Yes, I think that''s what Bamvor meant but I don''t think it''s correct > that such a lock eliminates the race. libvirt has to release that > lock before making the callback (to follow the libxl locking rules > which are necessary to avoid deadlock). >And it does. The event loop lock is released just before invoking the callback, and re-acquired just after the callback returns. Jim
Bamvor Jian Zhang
2012-Dec-11 10:19 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
>>>Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event > handling and libxl fd deregister"): > > I''m not surprised that the original patch makes Bamvor''s symptoms go > > away. Bamvor had one of the possible races (the fd-related one) but > > not the other. > > Here (followups to this message, shortly) is v3 of my two-patch series > which after conversation with Ian C I think fully fixes the race, and > which I have tested now. > > Bamvor, can you test this and let us know hwether it fixes your problem?Ok. i am working on it. will send the test result to u. BTW, without your patches, i encounter the time race condition during libvirt save vm sometimes.> Thanks, > Ian. > >
Ian Jackson
2012-Dec-11 11:18 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Jim Fehlig writes ("Re: [Xen-devel] [PATCH] fix race condition between
libvirtd event handling and libxl fd deregister"):> Ian Jackson wrote:
> > Yes, I think that''s what Bamvor meant but I don''t
think it''s correct
> > that such a lock eliminates the race. libvirt has to release that
> > lock before making the callback (to follow the libxl locking rules
> > which are necessary to avoid deadlock).
>
> And it does. The event loop lock is released just before invoking the
> callback, and re-acquired just after the callback returns.
Right. So I think the race that my patches are trying to fix does exist.
Ian.
Ian Jackson
2012-Dec-11 11:20 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Bamvor Jian Zhang writes ("Re: [PATCH] fix race condition between libvirtd
event handling and libxl fd deregister"):> Ok. i am working on it. will send the test result to u.
Marvellous.
> BTW, without your patches, i encounter the time race condition
> during libvirt save vm sometimes.
Oh, excellent, something resembling a test case :-).
Thanks,
Ian.
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@suse.com> > Cc: Bamvor Jian Zhang <bjzhang@suse.com> > Cc: Ian Campbell <Ian.Campbell@citrix.com> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> >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; > >
Jim Fehlig
2012-Dec-11 22:53 UTC
Re: [PATCH 2/2] libxl: fix stale timeout 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, in a > multithreaded program those calls may be arbitrarily delayed in > relation to other activities within the program. > > Specifically this means when ->timeout_deregister returns, libxl does > not know whether it can safely dispose of the for_libxl value or > whether it needs to retain it in case of an in-progress call to > _occurred_timeout. > > The interface could be fixed by requiring the application to make a > new call into libxl to say that the deregistration was complete. > > However that new call would have to be threaded through the > application''s event loop; this is complicated and some application > authors are likely not to implement it properly. Furthermore the > easiest way to implement this facility in most event loops is to queue > up a time event for "now". > > Shortcut all of this by having libxl always call timeout_modify > setting abs={0,0} (ie, ASAP) instead of timeout_deregister. This will > cause the application to call _occurred_timeout. When processing this > calldown we see that we were no longer actually interested and simply > throw it away. > > Additionally, there is a race between _occurred_timeout and > ->timeout_modify. If libxl ever adjusts the deadline for a timeout > the application may already be in the process of calling _occurred, in > which case the situation with for_app''s lifetime becomes very > complicated. Therefore abolish libxl__ev_time_modify_{abs,rel} (which > have no callers) and promise to the application only ever to call > ->timeout_modify with abs=={0,0}. The application still needs to cope > with ->timeout_modify racing with its internal function which calls > _occurred_timeout. Document this. > > This is a forwards-compatible change for applications using the libxl > API, and will hopefully eliminate these races in callback-supplying > applications (such as libvirt) without the need for corresponding > changes to the application. >I''m not sure how to avoid changing the application (libvirt). Currently, the libvirt libxl driver removes the timeout from the libvirt event loop in the timeout_deregister() function. This function is never called now and hence the timeout is never removed. I had to make the following change in the libxl driver to use your patches diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 302f81c..d8f078e 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -225,16 +225,11 @@ static int libxlTimeoutRegisterEventHook(void *priv, static int libxlTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED, void **hndp, - struct timeval abs_t) + struct timeval abs_t ATTRIBUTE_UNUSED) { - struct timeval now; - int timeout; struct libxlOSEventHookTimerInfo *timer_info = *hndp; - gettimeofday(&now, NULL); - timeout = (abs_t.tv_usec - now.tv_usec) / 1000; - timeout += (abs_t.tv_sec - now.tv_sec) * 1000; - virEventUpdateTimeout(timer_info->id, timeout); + virEventRemoveTimeout(timer_info->id); return 0; } I could also call virEventUpdateTimeout() with a timeout of 0 to make it fire on the next iteration of the event loop, and then remove the timeout in the callback before invoking libxl_osevent_occurred_timeout(). But either way, changes need to be made to the application.> For clarity, fold the body of time_register_finite into its one > remaining call site. This makes the semantics of ev->infinite > slightly clearer. > > Cc: Bamvor Jian Zhang <bjzhang@suse.com> > Cc: Ian Campbell <Ian.Campbell@citrix.com> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > > -- > v3: > - Fix null pointer dereference in case when hooks not supplied. > --- > tools/libxl/libxl_event.c | 89 +++++++-------------------------------------- > tools/libxl/libxl_event.h | 17 ++++++++- > 2 files changed, 29 insertions(+), 77 deletions(-) > > diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c > index f1fe425..f86c528 100644 > --- a/tools/libxl/libxl_event.c > +++ b/tools/libxl/libxl_event.c > @@ -267,18 +267,11 @@ static int time_rel_to_abs(libxl__gc *gc, int ms, struct timeval *abs_out) > return 0; > } > > -static void time_insert_finite(libxl__gc *gc, libxl__ev_time *ev) > -{ > - libxl__ev_time *evsearch; > - LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch, /*empty*/, > - timercmp(&ev->abs, &evsearch->abs, >)); > - ev->infinite = 0; > -} > - > static int time_register_finite(libxl__gc *gc, libxl__ev_time *ev, > struct timeval absolute) > { > int rc; > + libxl__ev_time *evsearch; > > rc = OSEVENT_HOOK(timeout,register, alloc, &ev->nexus->for_app_reg, > absolute, ev->nexus); > @@ -286,7 +279,8 @@ static int time_register_finite(libxl__gc *gc, libxl__ev_time *ev, > > ev->infinite = 0; > ev->abs = absolute; > - time_insert_finite(gc, ev); > + LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch, /*empty*/, > + timercmp(&ev->abs, &evsearch->abs, >)); > > return 0; > } > @@ -294,7 +288,12 @@ 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, release, ev->nexus->for_app_reg); > + struct timeval right_away = { 0, 0 }; > + if (ev->nexus) /* only set if app provided hooks */ > + ev->nexus->ev = 0; > + OSEVENT_HOOK_VOID(timeout,modify, >Nit again about the space here.> + noop /* release nexus in _occurred_ */, > + ev->nexus->for_app_reg, right_away); >I had to change this to &ev->nexus->for_app_reg to get it to work properly. But with this change, and the aforementioned hack to the libvirt libxl driver, I''m not seeing the race any longer. Regards, Jim> LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry); > } > } > @@ -364,70 +363,6 @@ int libxl__ev_time_register_rel(libxl__gc *gc, libxl__ev_time *ev, > return rc; > } > > -int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev, > - struct timeval absolute) > -{ > - int rc; > - > - CTX_LOCK; > - > - DBG("ev_time=%p modify abs==%lu.%06lu", > - ev, (unsigned long)absolute.tv_sec, (unsigned long)absolute.tv_usec); > - > - assert(libxl__ev_time_isregistered(ev)); > - > - if (ev->infinite) { > - rc = time_register_finite(gc, ev, absolute); > - if (rc) goto out; > - } else { > - rc = OSEVENT_HOOK(timeout,modify, noop, > - &ev->nexus->for_app_reg, absolute); > - if (rc) goto out; > - > - LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry); > - ev->abs = absolute; > - time_insert_finite(gc, ev); > - } > - > - rc = 0; > - out: > - time_done_debug(gc,__func__,ev,rc); > - CTX_UNLOCK; > - return rc; > -} > - > -int libxl__ev_time_modify_rel(libxl__gc *gc, libxl__ev_time *ev, > - int milliseconds) > -{ > - struct timeval absolute; > - int rc; > - > - CTX_LOCK; > - > - DBG("ev_time=%p modify ms=%d", ev, milliseconds); > - > - assert(libxl__ev_time_isregistered(ev)); > - > - if (milliseconds < 0) { > - time_deregister(gc, ev); > - ev->infinite = 1; > - rc = 0; > - goto out; > - } > - > - rc = time_rel_to_abs(gc, milliseconds, &absolute); > - if (rc) goto out; > - > - rc = libxl__ev_time_modify_abs(gc, ev, absolute); > - if (rc) goto out; > - > - rc = 0; > - out: > - time_done_debug(gc,__func__,ev,rc); > - CTX_UNLOCK; > - return rc; > -} > - > void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev) > { > CTX_LOCK; > @@ -1161,7 +1096,11 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl) > CTX_LOCK; > assert(!CTX->osevent_in_hook); > > - libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, for_libxl); > + libxl__osevent_hook_nexus *nexus = for_libxl; > + libxl__ev_time *ev = osevent_ev_from_hook_nexus(ctx, nexus); > + > + osevent_release_nexus(gc, &CTX->hook_timeout_nexi_idle, nexus); > + > if (!ev) goto out; > assert(!ev->infinite); > > diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h > index 3bcb6d3..51f2721 100644 > --- a/tools/libxl/libxl_event.h > +++ b/tools/libxl/libxl_event.h > @@ -287,8 +287,10 @@ typedef struct libxl_osevent_hooks { > int (*timeout_register)(void *user, void **for_app_registration_out, > struct timeval abs, void *for_libxl); > int (*timeout_modify)(void *user, void **for_app_registration_update, > - struct timeval abs); > - void (*timeout_deregister)(void *user, void *for_app_registration); > + struct timeval abs) > + /* only ever called with abs={0,0}, meaning ASAP */; > + void (*timeout_deregister)(void *user, void *for_app_registration) > + /* will never be called */; > } libxl_osevent_hooks; > > /* The application which calls register_fd_hooks promises to > @@ -337,6 +339,17 @@ typedef struct libxl_osevent_hooks { > * register (or modify), and pass it to subsequent calls to modify > * or deregister. > * > + * Note that the application must cope with a call from libxl to > + * timeout_modify racing with its own call to > + * libxl__osevent_occurred_timeout. libxl guarantees that > + * timeout_modify will only be called with abs={0,0} but the > + * application must still ensure that libxl''s attempt to cause the > + * timeout to occur immediately is safely ignored even the timeout is > + * actually already in the process of occurring. > + * > + * timeout_deregister is not used because it forms part of a > + * deprecated unsafe mode of use of the API. > + * > * osevent_register_hooks may be called only once for each libxl_ctx. > * libxl may make calls to register/modify/deregister from within > * any libxl function (indeed, it will usually call register from >
Jim Fehlig writes ("Re: [Xen-devel] [PATCH 1/2] libxl: fix stale fd event
callback race"):> 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.
Thanks for the review and testing.
> > - 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''?
I did this deliberately because the macro uses token pasting to turn
this into fd_register, at least in many of the uses.
>>Also, not that gcc complained, but register is a keyword.
The token "register" gets pasted together with other things by the
preprocessor before the compiler sees it, so it''s correct as far as
the language spec goes. As for it being potentially confusing,
changing what appears here is difficult without changing the names
in libxl_osevent_hooks.
Thanks,
Ian.
Ian Jackson
2012-Dec-12 17:14 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout
event callback race"):> Ian Jackson wrote:
> > This is a forwards-compatible change for applications using the libxl
> > API, and will hopefully eliminate these races in callback-supplying
> > applications (such as libvirt) without the need for corresponding
> > changes to the application.
When I wrote this of course I forgot to mention that previously libxl
would never call libxl_osevent_hooks->timeout_modify and now it never
calls ->timeout_deregister.
So this change can expose bugs in the application''s implementation of
->timeout_modify.
> I''m not sure how to avoid changing the application (libvirt).
Currently,
> the libvirt libxl driver removes the timeout from the libvirt event loop
> in the timeout_deregister() function. This function is never called now
> and hence the timeout is never removed. I had to make the following
> change in the libxl driver to use your patches
I think this is because of a bug of the kind I describe above.
> - gettimeofday(&now, NULL);
> - timeout = (abs_t.tv_usec - now.tv_usec) / 1000;
> - timeout += (abs_t.tv_sec - now.tv_sec) * 1000;
Specifically, this code has an integer arithmetic overflow.
If libxl calls this function with a value of abs_t which is more than
about INT_MAX/1000 seconds (24 days on a 32-bit machine) in the past,
the calculation (abs_t.tv_sec - now.tv_sec) * 1000; overflows.
And the patch makes libxl always pass abs_t={0,0} which is more than
24 days ago unless the code is running during January 1970 :-).
> - virEventUpdateTimeout(timer_info->id, timeout);
Also, what does this do if timeout is negative ?
Thanks,
Ian.
Ian Jackson
2012-Dec-12 17:16 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Ian Jackson writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout
event callback race"):> Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale
timeout event callback race"):
> > - gettimeofday(&now, NULL);
> > - timeout = (abs_t.tv_usec - now.tv_usec) / 1000;
> > - timeout += (abs_t.tv_sec - now.tv_sec) * 1000;
>
> Specifically, this code has an integer arithmetic overflow.
If you look at beforepoll_internal in libxl_event.c, near line 859,
you can see how I tackled this same problem there.
Ian.
Ian Jackson wrote:> Jim Fehlig writes ("Re: [Xen-devel] [PATCH 1/2] libxl: fix stale fd event callback race"): > >> 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. >> > > Thanks for the review and testing. > > >>> - 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''? >> > > I did this deliberately because the macro uses token pasting to turn > this into fd_register, at least in many of the uses. >Ok.> >>> Also, not that gcc complained, but register is a keyword. >>> > > The token "register" gets pasted together with other things by the > preprocessor before the compiler sees it, so it''s correct as far as > the language spec goes. As for it being potentially confusing, > changing what appears here is difficult without changing the names > in libxl_osevent_hooks. >Yes, understood. Regards, Jim
Jim Fehlig
2012-Dec-12 17:26 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Ian Jackson wrote:> Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout event callback race"): > >> Ian Jackson wrote: >> >>> This is a forwards-compatible change for applications using the libxl >>> API, and will hopefully eliminate these races in callback-supplying >>> applications (such as libvirt) without the need for corresponding >>> changes to the application. >>> > > When I wrote this of course I forgot to mention that previously libxl > would never call libxl_osevent_hooks->timeout_modify and now it never > calls ->timeout_deregister. > > So this change can expose bugs in the application''s implementation of > ->timeout_modify. > > >> I''m not sure how to avoid changing the application (libvirt). Currently, >> the libvirt libxl driver removes the timeout from the libvirt event loop >> in the timeout_deregister() function. This function is never called now >> and hence the timeout is never removed. I had to make the following >> change in the libxl driver to use your patches >> > > I think this is because of a bug of the kind I describe above. > > >> - gettimeofday(&now, NULL); >> - timeout = (abs_t.tv_usec - now.tv_usec) / 1000; >> - timeout += (abs_t.tv_sec - now.tv_sec) * 1000; >> > > Specifically, this code has an integer arithmetic overflow. >Well, this patch is removing that buggy code :). After again reading libxl_event.h, I''m considering the below patch in the libvirt libxl driver. The change is primarily inspired by this comment for libxl_osevent_occurred_timeout: /* Implicitly, on entry to this function the timeout has been * deregistered. If _occurred_timeout is called, libxl will not * call timeout_deregister; if it wants to requeue the timeout it * will call timeout_register again. */ Regards, Jim diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 302f81c..d4055be 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -184,6 +184,8 @@ static void libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_v) { struct libxlOSEventHookTimerInfo *timer_info = timer_v; + virEventRemoveTimeout(timer_info->id); + timer_info->id = -1; libxl_osevent_occurred_timeout(timer_info->priv->ctx, timer_info->xl_priv); } @@ -225,16 +227,12 @@ static int libxlTimeoutRegisterEventHook(void *priv, static int libxlTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED, void **hndp, - struct timeval abs_t) + struct timeval abs_t ATTRIBUTE_UNUSED) { - struct timeval now; - int timeout; struct libxlOSEventHookTimerInfo *timer_info = *hndp; - gettimeofday(&now, NULL); - timeout = (abs_t.tv_usec - now.tv_usec) / 1000; - timeout += (abs_t.tv_sec - now.tv_sec) * 1000; - virEventUpdateTimeout(timer_info->id, timeout); + if (timer_info->id > 0) + virEventUpdateTimeout(timer_info->id, 0); return 0; }
Ian Jackson
2012-Dec-12 17:37 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout
event callback race"):> Ian Jackson wrote:
> > Specifically, this code has an integer arithmetic overflow.
>
> Well, this patch is removing that buggy code :).
I think you need to have some code somewhere which does the
complicated arithmetic dance to avoid the integer overflow. Does your
timer registration function have the same bug ?
> After again reading
> libxl_event.h, I''m considering the below patch in the libvirt
libxl
> driver. The change is primarily inspired by this comment for
> libxl_osevent_occurred_timeout:
...> /* Implicitly, on entry to this function the timeout has been
> * deregistered. If _occurred_timeout is called, libxl will not
> * call timeout_deregister; if it wants to requeue the timeout it
> * will call timeout_register again.
> */
Well your patch is only correct when used with the new libxl, with my
patches.
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 302f81c..d4055be 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -184,6 +184,8 @@ static void libxlTimerCallback(int timer
> ATTRIBUTE_UNUSED, void *timer_v)
> {
> struct libxlOSEventHookTimerInfo *timer_info = timer_v;
>
> + virEventRemoveTimeout(timer_info->id);
> + timer_info->id = -1;
I don''t understand why you need this. Doesn''t libvirt remove
timers
when they fire ? If it doesn''t, do they otherwise not keep
reoccurring ?
Ian.
Jim Fehlig
2012-Dec-12 18:01 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Ian Jackson wrote:> Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout event callback race"): > >> Ian Jackson wrote: >> >>> Specifically, this code has an integer arithmetic overflow. >>> >> Well, this patch is removing that buggy code :). >> > > I think you need to have some code somewhere which does the > complicated arithmetic dance to avoid the integer overflow. Does your > timer registration function have the same bug ? >Ah yes. I''ll take care of it following your suggestion around beforepoll_internal.> >> After again reading >> libxl_event.h, I''m considering the below patch in the libvirt libxl >> driver. The change is primarily inspired by this comment for >> libxl_osevent_occurred_timeout: >> > ... > >> /* Implicitly, on entry to this function the timeout has been >> * deregistered. If _occurred_timeout is called, libxl will not >> * call timeout_deregister; if it wants to requeue the timeout it >> * will call timeout_register again. >> */ >> > > Well your patch is only correct when used with the new libxl, with my > patches. >Hmm, it is not clear to me how to make the libxl driver work correctly with libxl pre and post your patches :-/.> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c >> index 302f81c..d4055be 100644 >> --- a/src/libxl/libxl_driver.c >> +++ b/src/libxl/libxl_driver.c >> @@ -184,6 +184,8 @@ static void libxlTimerCallback(int timer >> ATTRIBUTE_UNUSED, void *timer_v) >> { >> struct libxlOSEventHookTimerInfo *timer_info = timer_v; >> >> + virEventRemoveTimeout(timer_info->id); >> + timer_info->id = -1; >> > > I don''t understand why you need this. Doesn''t libvirt remove timers > when they fire ? If it doesn''t, do they otherwise not keep reoccurring ? >No, timers are not removed when they fire. And they are continuous, so will keep firing at each timeout. They can be disabled by setting the timeout to -1, and can be set to fire on each iteration of the event loop by setting the timeout to 0. But they must be explicitly removed with virEventRemoveTimeout when no longer needed. Regards, Jim
Ian Campbell
2012-Dec-13 10:29 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
On Wed, 2012-12-12 at 18:01 +0000, Jim Fehlig wrote:> Ian Jackson wrote:> > > >> After again reading > >> libxl_event.h, I''m considering the below patch in the libvirt libxl > >> driver. The change is primarily inspired by this comment for > >> libxl_osevent_occurred_timeout: > >> > > ... > > > >> /* Implicitly, on entry to this function the timeout has been > >> * deregistered. If _occurred_timeout is called, libxl will not > >> * call timeout_deregister; if it wants to requeue the timeout it > >> * will call timeout_register again. > >> */ > >> > > > > Well your patch is only correct when used with the new libxl, with my > > patches. > > > > Hmm, it is not clear to me how to make the libxl driver work correctly > with libxl pre and post your patches :-/.Ideally we will find a way to make this work without changes on the application side. But if that turns out to be impossible and applications are going to need patching anyway then I think we should consider just fixing the API rather than playing tricks like the "modify to 0" thing to try and keep it compatible. One option is to add new hooks which libxl can call to take/release the application''s event loop lock + a LIBXL_HAVE_EVENT_LOOP_LOCK define so the application can conditionally provide them. The upside is that I would expect this to result in much simpler code in both libxl and libvirt. The downside is that doing this kind of sucks from an API stability point of view, but if the application has to change anyway then we might as well do it cleanly instead of bending over backwards to keep the API the same. Ian.
Ian Jackson
2012-Dec-13 13:07 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout
event callback race"):> Ian Jackson wrote:
> > Well your patch is only correct when used with the new libxl, with my
> > patches.
>
> Hmm, it is not clear to me how to make the libxl driver work correctly
> with libxl pre and post your patches :-/.
This should be straightforward I think, except for the deregistration
race bug which is unavoidable with the old libxl. Simply correctly
implementing both the deregistration callback and the modification
callback ought to do it.
> >> @@ -184,6 +184,8 @@ static void libxlTimerCallback(int timer
> >> ATTRIBUTE_UNUSED, void *timer_v)
> >> {
> >> struct libxlOSEventHookTimerInfo *timer_info = timer_v;
> >>
> >> + virEventRemoveTimeout(timer_info->id);
> >> + timer_info->id = -1;
> >>
> >
> > I don''t understand why you need this. Doesn''t
libvirt remove timers
> > when they fire ? If it doesn''t, do they otherwise not keep
reoccurring ?
>
> No, timers are not removed when they fire. And they are continuous, so
> will keep firing at each timeout. They can be disabled by setting the
> timeout to -1, and can be set to fire on each iteration of the event
> loop by setting the timeout to 0. But they must be explicitly removed
> with virEventRemoveTimeout when no longer needed.
Right. Well then yes you need to call virEventRemoveTimeout here.
But that applies to both old and new libxl.
Ian.
Ian Jackson
2012-Dec-13 13:12 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout
event callback race"):> On Wed, 2012-12-12 at 18:01 +0000, Jim Fehlig wrote:
> > Hmm, it is not clear to me how to make the libxl driver work correctly
> > with libxl pre and post your patches :-/.
>
> Ideally we will find a way to make this work without changes on the
> application side.
Yes, but if the application has bugs which are exposed by the new
approach I think it is probably simplest to fix those bugs.
The way I''m proposing at the moment means that there are two sets of
relevant changes to libxl and libvirt:
- libxl always use timeout_modify and never _deregister. Officially
speaking this is a backwards-compatible change: libxl now promises
to use only a strict subset of the documented interface provided by
the application. Any correct application will still work.
- libvirt implement both _modify and _deregister correctly. With old
libxl this leaves the timeout deregistration race. With new libxl
there is no problem at all.
> One option is to add new hooks which libxl can call to take/release the
> application''s event loop lock + a LIBXL_HAVE_EVENT_LOOP_LOCK
define so
> the application can conditionally provide them. The upside is that I
> would expect this to result in much simpler code in both libxl and
> libvirt. The downside is that doing this kind of sucks from an API
> stability point of view, but if the application has to change anyway
> then we might as well do it cleanly instead of bending over backwards to
> keep the API the same.
I don''t know what other users there might be who would be hurt by an
API change.
But I think from a deployment/testing/etc. point of view two separate
patches to libxl and libvirt which each make matters strictly better,
and which can be applied independently, does seem like the best
approach.
Ian.
Jim Fehlig
2012-Dec-13 15:41 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Ian Jackson wrote:> Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout event callback race"): > >> Ian Jackson wrote: >> >>> Well your patch is only correct when used with the new libxl, with my >>> patches. >>> >> Hmm, it is not clear to me how to make the libxl driver work correctly >> with libxl pre and post your patches :-/. >> > > This should be straightforward I think, except for the deregistration > race bug which is unavoidable with the old libxl. Simply correctly > implementing both the deregistration callback and the modification > callback ought to do it. >Yes, after thinking about it some more, I agree. As for the modify callback, do you agree that it is fine to ignore the timeval parameter and just update the timer in libvirt''s event loop to fire immediately? I.e. always treat the timeval parameter as containing {0,0} regardless of "old" or "new" libxl? I think my patch here is correct http://lists.xen.org/archives/html/xen-devel/2012-12/msg00985.html> >>>> @@ -184,6 +184,8 @@ static void libxlTimerCallback(int timer >>>> ATTRIBUTE_UNUSED, void *timer_v) >>>> { >>>> struct libxlOSEventHookTimerInfo *timer_info = timer_v; >>>> >>>> + virEventRemoveTimeout(timer_info->id); >>>> + timer_info->id = -1; >>>> >>>> >>> I don''t understand why you need this. Doesn''t libvirt remove timers >>> when they fire ? If it doesn''t, do they otherwise not keep reoccurring ? >>> >> No, timers are not removed when they fire. And they are continuous, so >> will keep firing at each timeout. They can be disabled by setting the >> timeout to -1, and can be set to fire on each iteration of the event >> loop by setting the timeout to 0. But they must be explicitly removed >> with virEventRemoveTimeout when no longer needed. >> > > Right. Well then yes you need to call virEventRemoveTimeout here. > But that applies to both old and new libxl. >Right. Thanks for the help with getting this issue resolved! Jim
Ian Jackson
2012-Dec-13 15:51 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout
event callback race"):> Yes, after thinking about it some more, I agree.
>
> As for the modify callback, do you agree that it is fine to ignore the
> timeval parameter and just update the timer in libvirt''s event
loop to
> fire immediately? I.e. always treat the timeval parameter as containing
> {0,0} regardless of "old" or "new" libxl?
Yes. That is fine.
The reason is that old libxl doesn''t ever call this function and new
libxl always calls it with {0,0}. If you''re worried about this, add
an assertion :-). But it''s theoretical.
> I think my patch here is correct
> http://lists.xen.org/archives/html/xen-devel/2012-12/msg00985.html
Having thought about it, I agree, provided that you also fix the
potential integer overflow in libxlTimeoutRegisterEventHook.
> > Right. Well then yes you need to call virEventRemoveTimeout here.
> > But that applies to both old and new libxl.
>
> Right.
>
> Thanks for the help with getting this issue resolved!
Thank you and I''m sorry to have caused trouble with my inadequate
analysis.
Ian.
Jim Fehlig
2012-Dec-13 15:53 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Ian Campbell wrote:> On Wed, 2012-12-12 at 18:01 +0000, Jim Fehlig wrote: > >> Ian Jackson wrote: >> > > >>> >>> >>>> After again reading >>>> libxl_event.h, I''m considering the below patch in the libvirt libxl >>>> driver. The change is primarily inspired by this comment for >>>> libxl_osevent_occurred_timeout: >>>> >>>> >>> ... >>> >>> >>>> /* Implicitly, on entry to this function the timeout has been >>>> * deregistered. If _occurred_timeout is called, libxl will not >>>> * call timeout_deregister; if it wants to requeue the timeout it >>>> * will call timeout_register again. >>>> */ >>>> >>>> >>> Well your patch is only correct when used with the new libxl, with my >>> patches. >>> >>> >> Hmm, it is not clear to me how to make the libxl driver work correctly >> with libxl pre and post your patches :-/. >> > > Ideally we will find a way to make this work without changes on the > application side. >I think Ian J. is right about applications still working, *if* they have the callbacks coded correctly. There are some bugs on the libvirt side as well :).> But if that turns out to be impossible and applications are going to > need patching anyway then I think we should consider just fixing the API > rather than playing tricks like the "modify to 0" thing to try and keep > it compatible. > > One option is to add new hooks which libxl can call to take/release the > application''s event loop lock + a LIBXL_HAVE_EVENT_LOOP_LOCK define so > the application can conditionally provide them.libvirt''s event loop lock is private to the event impl and not exposed to its numerous users. Regards, Jim
Jim Fehlig
2012-Dec-13 15:57 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Ian Jackson wrote:> Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout event callback race"): > >> Yes, after thinking about it some more, I agree. >> >> As for the modify callback, do you agree that it is fine to ignore the >> timeval parameter and just update the timer in libvirt''s event loop to >> fire immediately? I.e. always treat the timeval parameter as containing >> {0,0} regardless of "old" or "new" libxl? >> > > Yes. That is fine. > > The reason is that old libxl doesn''t ever call this function and new > libxl always calls it with {0,0}. If you''re worried about this, add > an assertion :-). But it''s theoretical. > > >> I think my patch here is correct >> http://lists.xen.org/archives/html/xen-devel/2012-12/msg00985.html >> > > Having thought about it, I agree, provided that you also fix the > potential integer overflow in libxlTimeoutRegisterEventHook. >Right. I''ll squash a fix to handle that into my current patch and send it to the libvirt dev list, cc''ing xen-devel too. Regards, Jim
Ian Jackson
2012-Dec-13 15:58 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout
event callback race"):> Ian Campbell wrote:
> > One option is to add new hooks which libxl can call to take/release
the
> > application''s event loop lock + a LIBXL_HAVE_EVENT_LOOP_LOCK
define so
> > the application can conditionally provide them.
>
> libvirt''s event loop lock is private to the event impl and not
exposed
> to its numerous users.
Right. I still think it might be useful to provide a way for a
consenting application to allow libxl to use the application''s event
loop lock (perhaps, its single giant lock) as the ctx lock. If it had
been possible in this case it would have eliminated these particular
races, so it''s a benefit for those applications. And the extra
complexity doesn''t seem likely to introduce other bugs.
But I think we should fault that feature in when we have a potential
user for it, and from what you say that''s not libvirt.
Ian.
Jim Fehlig
2012-Dec-13 16:53 UTC
Re: [PATCH 2/2] libxl: fix stale timeout event callback race
Ian Jackson wrote:> Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout event callback race"): > >> Ian Campbell wrote: >> >>> One option is to add new hooks which libxl can call to take/release the >>> application''s event loop lock + a LIBXL_HAVE_EVENT_LOOP_LOCK define so >>> the application can conditionally provide them. >>> >> libvirt''s event loop lock is private to the event impl and not exposed >> to its numerous users. >> > > Right. I still think it might be useful to provide a way for a > consenting application to allow libxl to use the application''s event > loop lock (perhaps, its single giant lock) as the ctx lock. If it had > been possible in this case it would have eliminated these particular > races, so it''s a benefit for those applications. And the extra > complexity doesn''t seem likely to introduce other bugs. > > But I think we should fault that feature in when we have a potential > user for it, and from what you say that''s not libvirt. >Correct. That approach doesn''t really fit with libvirt''s generic event loop used by the various drivers. I suppose the libxl driver could have a private event loop, but I''d prefer to keep the pattern used by other drivers. Regards, Jim
Ian Campbell
2013-Jan-11 11:41 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
On Mon, 2012-12-10 at 16:56 +0000, Ian Jackson wrote:> Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): > > I''m not surprised that the original patch makes Bamvor''s symptoms go > > away. Bamvor had one of the possible races (the fd-related one) but > > not the other. > > Here (followups to this message, shortly) is v3 of my two-patch series > which after conversation with Ian C I think fully fixes the race, and > which I have tested now.Is this version now tested and ready to be applied?> Bamvor, can you test this and let us know hwether it fixes your problem? > > Thanks, > Ian.
Jim Fehlig
2013-Jan-11 17:51 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Ian Campbell wrote:> On Mon, 2012-12-10 at 16:56 +0000, Ian Jackson wrote: > >> Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): >> >>> I''m not surprised that the original patch makes Bamvor''s symptoms go >>> away. Bamvor had one of the possible races (the fd-related one) but >>> not the other. >>> >> Here (followups to this message, shortly) is v3 of my two-patch series >> which after conversation with Ian C I think fully fixes the race, and >> which I have tested now. >> > > Is this version now tested and ready to be applied? >Hi Ian, I have been doing quite a bit of testing with this version, but have one remaining issue wrt races between the libvirt libxl driver and libxl. Earlier in this thread you mentioned this potential solution "The other scheme which springs to mind is to do reference counting, with the application holding a reference whenever the event is present in its event loop (such that there is any chance of the event being generated) and libxl holding a reference while it considers the event to be active" I thought this was a good approach, particularly since libvirt has excellent support for it. When libxl registers an fd/timer, I create an object containing the details with an initial reference count of 1. If the fd/timer is successfully injected into libvirt''s event loop, I take another reference on the object. The object is only destroyed after libxl has deregistered the fd/timer *and* it has been removed from libvirt''s event loop. For each fd/timer object, I also increment the reference count on my libxl_ctx object. This approach works well IMO. It ensures the libxl_ctx exists for the life of all fd/timer objects. The only wrench in this machinery is that watch_efd is not deregistered until calling libxl_ctx_free(). But I never get to that point since that fd registration holds a reference on my libxl_ctx :(. My first thought was to cleanup/deregister that fd on domain death, but I didn''t have much success creating a patch. Perhaps I should look at that again... Some other thoughts included: 1) an API to remove fd/timers from libxl, 2) ensure no callbacks are invoked from libxl_ctx_free(). Thanks! Jim
Ian Campbell
2013-Jan-15 10:40 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
On Fri, 2013-01-11 at 17:51 +0000, Jim Fehlig wrote:> Ian Campbell wrote: > > On Mon, 2012-12-10 at 16:56 +0000, Ian Jackson wrote: > > > >> Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): > >> > >>> I''m not surprised that the original patch makes Bamvor''s symptoms go > >>> away. Bamvor had one of the possible races (the fd-related one) but > >>> not the other. > >>> > >> Here (followups to this message, shortly) is v3 of my two-patch series > >> which after conversation with Ian C I think fully fixes the race, and > >> which I have tested now. > >> > > > > Is this version now tested and ready to be applied? > > > > Hi Ian, > > I have been doing quite a bit of testing with this version, but have one > remaining issue wrt races between the libvirt libxl driver and libxl. > Earlier in this thread you mentioned this potential solution > > "The other scheme which springs to mind is to do reference counting, with > the application holding a reference whenever the event is present in its > event loop (such that there is any chance of the event being generated) > and libxl holding a reference while it considers the event to be active" > > I thought this was a good approach, particularly since libvirt has > excellent support for it. When libxl registers an fd/timer, I create an > object containing the details with an initial reference count of 1. If > the fd/timer is successfully injected into libvirt''s event loop, I take > another reference on the object. The object is only destroyed after > libxl has deregistered the fd/timer *and* it has been removed from > libvirt''s event loop. For each fd/timer object, I also increment the > reference count on my libxl_ctx object. This approach works well IMO. > It ensures the libxl_ctx exists for the life of all fd/timer objects.Is taking a reference count on the ctx for each fd/timer strictly necessary? You can guarantee that the ctx lifetime is greater than the fd/timer lifetime because if you were to destroy the ctx then it would teardown the fd/timer as part of ctx_free (I think? More of an Ian J question). Without those extra references I think the problem you describe below doesn''t happen.> The only wrench in this machinery is that watch_efd is not deregistered > until calling libxl_ctx_free(). But I never get to that point since > that fd registration holds a reference on my libxl_ctx :(. My first > thought was to cleanup/deregister that fd on domain death, but I didn''t > have much success creating a patch. Perhaps I should look at that again...I''d be worried about libxl internal uses of this watch which you cannot easily control preventing you from doing this.> Some other thoughts included: 1) an API to remove fd/timers from libxl, > 2) ensure no callbacks are invoked from libxl_ctx_free(). > > Thanks! > Jim >
Jim Fehlig
2013-Jan-15 17:02 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Ian Campbell wrote:> On Fri, 2013-01-11 at 17:51 +0000, Jim Fehlig wrote: > >> Ian Campbell wrote: >> >>> On Mon, 2012-12-10 at 16:56 +0000, Ian Jackson wrote: >>> >>> >>>> Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): >>>> >>>> >>>>> I''m not surprised that the original patch makes Bamvor''s symptoms go >>>>> away. Bamvor had one of the possible races (the fd-related one) but >>>>> not the other. >>>>> >>>>> >>>> Here (followups to this message, shortly) is v3 of my two-patch series >>>> which after conversation with Ian C I think fully fixes the race, and >>>> which I have tested now. >>>> >>>> >>> Is this version now tested and ready to be applied? >>> >>> >> Hi Ian, >> >> I have been doing quite a bit of testing with this version, but have one >> remaining issue wrt races between the libvirt libxl driver and libxl. >> Earlier in this thread you mentioned this potential solution >> >> "The other scheme which springs to mind is to do reference counting, with >> the application holding a reference whenever the event is present in its >> event loop (such that there is any chance of the event being generated) >> and libxl holding a reference while it considers the event to be active" >> >> I thought this was a good approach, particularly since libvirt has >> excellent support for it. When libxl registers an fd/timer, I create an >> object containing the details with an initial reference count of 1. If >> the fd/timer is successfully injected into libvirt''s event loop, I take >> another reference on the object. The object is only destroyed after >> libxl has deregistered the fd/timer *and* it has been removed from >> libvirt''s event loop. For each fd/timer object, I also increment the >> reference count on my libxl_ctx object. This approach works well IMO. >> It ensures the libxl_ctx exists for the life of all fd/timer objects. >> > > Is taking a reference count on the ctx for each fd/timer strictly > necessary? > > You can guarantee that the ctx lifetime is greater than the fd/timer > lifetime because if you were to destroy the ctx then it would teardown > the fd/timer as part of ctx_free (I think? More of an Ian J question). >Yes, but the teardown of timers in particular is asynchronous. libxl calls the modify timeout hook with abs_t of {0,0}, the timer fires on next iteration of event loop invoking the callback, which calls libxl_osevent_occurred_timeout() to finally cleanup the timeout on the libxl side. But in the meantime, the associated ctx has been freed. Taking a ref count on the ctx avoids this race.> Without those extra references I think the problem you describe below > doesn''t happen. >Right, but then the ctx disappears before all fds/timers have been cleaned up.> >> The only wrench in this machinery is that watch_efd is not deregistered >> until calling libxl_ctx_free(). But I never get to that point since >> that fd registration holds a reference on my libxl_ctx :(. My first >> thought was to cleanup/deregister that fd on domain death, but I didn''t >> have much success creating a patch. Perhaps I should look at that again... >> > > I''d be worried about libxl internal uses of this watch which you cannot > easily control preventing you from doing this. >Agreed :/. Jim
Jim Fehlig
2013-Jan-21 19:37 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
Ian Campbell wrote:> On Mon, 2012-12-10 at 16:56 +0000, Ian Jackson wrote: > >> Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): >> >>> I''m not surprised that the original patch makes Bamvor''s symptoms go >>> away. Bamvor had one of the possible races (the fd-related one) but >>> not the other. >>> >> Here (followups to this message, shortly) is v3 of my two-patch series >> which after conversation with Ian C I think fully fixes the race, and >> which I have tested now. >> > > Is this version now tested and ready to be applied? >I have done quite a bit of successful testing with this version and a libvirt patch series [1] that fixes several races and bugs in the libxl driver. So from my perspective, ACK to these patches, assuming my last comment on patch 2 [2] is addressed. [1] https://www.redhat.com/archives/libvir-list/2013-January/msg01463.html [2] http://lists.xen.org/archives/html/xen-devel/2012-12/msg00937.html Thanks for all the help with resolving these races! Jim
Ian Campbell
2013-Jan-22 10:21 UTC
Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister
On Mon, 2013-01-21 at 19:37 +0000, Jim Fehlig wrote:> Ian Campbell wrote: > > On Mon, 2012-12-10 at 16:56 +0000, Ian Jackson wrote: > > > >> Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): > >> > >>> I''m not surprised that the original patch makes Bamvor''s symptoms go > >>> away. Bamvor had one of the possible races (the fd-related one) but > >>> not the other. > >>> > >> Here (followups to this message, shortly) is v3 of my two-patch series > >> which after conversation with Ian C I think fully fixes the race, and > >> which I have tested now. > >> > > > > Is this version now tested and ready to be applied? > > > > I have done quite a bit of successful testing with this version and a > libvirt patch series [1] that fixes several races and bugs in the libxl > driver. So from my perspective, ACK to these patches, assuming my last > comment on patch 2 [2] is addressed.Great, Ian -- can you send a final version please?> > [1] https://www.redhat.com/archives/libvir-list/2013-January/msg01463.html > [2] http://lists.xen.org/archives/html/xen-devel/2012-12/msg00937.html > > Thanks for all the help with resolving these races! > > Jim >