[Xen-changelog] [xen master] libxl: Fix libxl_postfork_child_noexec deadlock etc.

[patchbot@xxxxxxx]

commit d6ac84ca0db28b99073d4364815e0f71600c5780
Author:     Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
AuthorDate: Mon Feb 24 12:57:53 2014 +0000
Commit:     Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
CommitDate: Mon Feb 24 16:30:23 2014 +0000

    libxl: Fix libxl_postfork_child_noexec deadlock etc.
    
    libxl_postfork_child_noexec would nestedly reaquire the non-recursive
    "no_forking" mutex: atfork_lock uses it, as does sigchld_user_remove.
    The result on Linux is that the process always deadlocks before
    returning from this function.
    
    This is used by xl's console child.  So, the ultimate effect is that
    xl with pygrub does not manage to connect to the pygrub console.
    This behaviour was reported by Michael Young in Xen 4.4.0 RC5.
    
    Also, the use of sigchld_user_remove in libxl_postfork_child_noexec is
    not correct with SIGCHLD sharing.  libxl_postfork_child_noexec is
    documented to suffice if called only on one ctx.  So deregistering the
    ctx it's called on is not sufficient.  Instead, we need a new approach
    which discards the whole sigchld_user list and unconditionally removes
    our SIGCHLD handler if we had one.
    
    Prompted by this, clarify the semantics of
    libxl_postfork_child_noexec.  Specifically, expand on the meaning of
    "quickly" by explaining what operations are not permitted; and
    document the fact that the function doesn't reclaim the resources in
    the ctxs.
    
    And add a comment in libxl_postfork_child_noexec explaining the
    internal concurrency situation.
    
    This is an important bugfix.  IMO the bug is a blocker for Xen 4.4.
    
    Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
    Reported-by: M A Young <m.a.young@xxxxxxxxxxxx>
    CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
    CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
    Acked-by: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
    Release-acked-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
    
    (cherry picked from commit 5be1e95318147855713709094e6847e3104ae910)
---
 tools/libxl/libxl_event.h |   16 ++++++++++++++++
 tools/libxl/libxl_fork.c  |   44 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index ca43cb9..b5db83c 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -601,6 +601,22 @@ void libxl_childproc_sigchld_occurred(libxl_ctx *ctx)
  * this all previously existing libxl_ctx's are invalidated and
  * must not be used - or even freed.  It is harmless to call this
  * postfork function and then exec anyway.
+ *
+ * Until libxl_postfork_child_noexec has returned:
+ *  - No other libxl calls may be made.
+ *  - If any libxl ctx was configured handle the process's SIGCHLD,
+ *    the child may not create further (grand)child processes, nor
+ *    manipulate SIGCHLD.
+ *
+ * libxl_postfork_child_noexec may not reclaim all the resources
+ * associated with the libxl ctx.  This includes but is not limited
+ * to: ordinary memory; files on disk and in /var/run; file
+ * descriptors; memory mapped into the process from domains being
+ * managed (grant maps); Xen event channels.  Use of libxl in
+ * processes which fork long-lived children is not recommended for
+ * this reason.  libxl_postfork_child_noexec is provided so that
+ * an application can make further libxl calls in a child which
+ * is going to exec or exit soon.
  */
 void libxl_postfork_child_noexec(libxl_ctx *ctx);
 
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index 1d0017b..8421296 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -60,6 +60,9 @@ static void sigchld_removehandler_core(void); /* idempotent */
 static void sigchld_user_remove(libxl_ctx *ctx); /* idempotent */
 static void sigchld_sethandler_raw(void (*handler)(int), struct sigaction 
*old);
 
+static void defer_sigchld(void);
+static void release_sigchld(void);
+
 static void atfork_lock(void)
 {
     int r = pthread_mutex_lock(&no_forking);
@@ -117,6 +120,19 @@ libxl__carefd *libxl__carefd_opened(libxl_ctx *ctx, int fd)
 
 void libxl_postfork_child_noexec(libxl_ctx *ctx)
 {
+    /*
+     * Anything running without the no_forking lock (atfork_lock)
+     * might be interrupted by fork.  But libxl functions other than
+     * this one are then forbidden to the child.
+     *
+     * Conversely, this function might interrupt any other libxl
+     * operation (even though that other operation has the libxl ctx
+     * lock).  We don't take the lock ourselves, since we are running
+     * in the child and if the lock is held the thread that took it no
+     * longer exists.  To prevent us being interrupted by another call
+     * to ourselves (whether in another thread or by virtue of another
+     * fork) we take the atfork lock ourselves.
+     */
     libxl__carefd *cf, *cf_tmp;
     int r;
 
@@ -134,7 +150,33 @@ void libxl_postfork_child_noexec(libxl_ctx *ctx)
     }
     LIBXL_LIST_INIT(&carefds);
 
-    sigchld_user_remove(ctx);
+    if (sigchld_installed) {
+        /* We are in theory not at risk of concurrent execution of the
+         * SIGCHLD handler, because the application should call
+         * libxl_postfork_child_noexec before the child forks again.
+         * (If the SIGCHLD was in flight in the parent at the time of
+         * the fork, the thread it was delivered on exists only in the
+         * parent so is not our concern.)
+         *
+         * But in case the application violated this rule (and did so
+         * while multithreaded in the child), we use our deferral
+         * machinery.  The result is that the SIGCHLD may then be lost
+         * (i.e. signaled to the now-defunct libxl ctx(s)).  But at
+         * least we won't execute undefined behaviour (by examining
+         * the list in the signal handler concurrently with clearing
+         * it here), and since we won't actually reap the new children
+         * things will in fact go OK if the application doesn't try to
+         * use SIGCHLD, but instead just waits for the child(ren). */
+        defer_sigchld();
+
+        LIBXL_LIST_INIT(&sigchld_users);
+        /* After this the ->sigchld_user_registered entries in the
+         * now-obsolete contexts may be lies.  But that's OK because
+         * no-one will look at them. */
+
+        release_sigchld();
+        sigchld_removehandler_core();
+    }
 
     atfork_unlock();
 }
--
generated by git-patchbot for /home/xen/git/xen.git#master

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog