[Xen-devel] [PATCH 2/2] libxl: fix stale timeout event callback race

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

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.  (It is possible that this might expose
bugs in applications, though, as previously libxl would never call
libxl_osevent_hooks->timeout_modify and now it never calls
->timeout_deregister).

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>
Tested-by: Jim Fehlig <jfehlig@xxxxxxxx>
Acked-by: Jim Fehlig <jfehlig@xxxxxxxx>
Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

--
v4:
  - Fix the wrong number of levels of indirection of for_app_reg
    in the new call to ->timeout_modify.  (Reported by Jim Fehlig.)
  - Add Jim Fehlig's Tested-by and Acked-by.
  - Improve the commit message somewhat.
---
 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..bfe317b 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