From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: [PATCH 2/2] libxl: fix stale timeout event callback race Date: Fri, 7 Dec 2012 19:15:34 +0000 Message-ID: <1354907734-26934-2-git-send-email-ian.jackson@eu.citrix.com> References: <20674.16214.934271.479230@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20674.16214.934271.479230@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xen.org Cc: Ian Jackson , Ian Campbell , Bamvor Jian Zhang List-Id: xen-devel@lists.xenproject.org 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 Cc: Ian Campbell Signed-off-by: Ian Jackson --- 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