|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [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@xxxxxxxx>
Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
--
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |