Libguestfs - Nov 2017 - [PATCH 2/3] Avoid race conditions when nbdkit exits.

This fixes the race conditions for me, using the test described here:

https://www.redhat.com/archives/libguestfs/2017-September/msg00226.html

Rich.

This reverts commit 63f0eb0889c8f8a82ba06a02a8a92d695902baad.
---
 src/internal.h    |  3 ---
 src/main.c        | 13 -------------
 src/threadlocal.c | 35 -----------------------------------
 3 files changed, 51 deletions(-)

diff --git a/src/internal.h b/src/internal.h
index b98be3d..1fc5d69 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -182,9 +182,6 @@ extern size_t threadlocal_get_instance_num (void);
 extern void threadlocal_set_error (int err);
 extern int threadlocal_get_error (void);
 /*extern void threadlocal_get_sockaddr ();*/
-extern size_t get_running_threads (void);
-extern void incr_running_threads (void);
-extern void decr_running_threads (void);
 
 /* Declare program_name. */
 #if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME == 1
diff --git a/src/main.c b/src/main.c
index 6ab556f..c9f08ab 100644
--- a/src/main.c
+++ b/src/main.c
@@ -189,7 +189,6 @@ main (int argc, char *argv[])
   int option_index;
   int help = 0, version = 0, dump_plugin = 0;
   int tls_set_on_cli = 0;
-  size_t count;
 
   threadlocal_init ();
 
@@ -492,12 +491,6 @@ main (int argc, char *argv[])
 
   start_serving ();
 
-  /* Wait, but not forever, for all threads to complete. */
-  debug ("waiting for running threads to complete");
-  for (count = 0; count < 5 && get_running_threads (); ++count)
-    sleep (1);
-  debug ("waited %zus for running threads to complete", count);
-
   plugin_cleanup ();
 
   free (unixsocket);
@@ -643,12 +636,6 @@ start_serving (void)
     threadlocal_new_server_thread ();
     if (handle_single_connection (0, 1) == -1)
       exit (EXIT_FAILURE);
-    /* When handling a single connection we reuse the main thread as
-     * the server thread.  We don't want to count it as a running
-     * thread, but it hasn't called pthread_exit(), so we need to
-     * explicitly decrement the running threads here.
-     */
-    decr_running_threads ();
     return;
   }
 
diff --git a/src/threadlocal.c b/src/threadlocal.c
index cc19d48..d6e3942 100644
--- a/src/threadlocal.c
+++ b/src/threadlocal.c
@@ -71,8 +71,6 @@ free_threadlocal (void *threadlocalv)
 
   free (threadlocal->addr);
   free (threadlocal);
-
-  decr_running_threads ();
 }
 
 void
@@ -93,8 +91,6 @@ threadlocal_new_server_thread (void)
 {
   struct threadlocal *threadlocal;
 
-  incr_running_threads ();
-
   threadlocal = calloc (1, sizeof *threadlocal);
   if (threadlocal == NULL) {
     perror ("malloc");
@@ -181,34 +177,3 @@ threadlocal_get_error (void)
   errno = err;
   return threadlocal ? threadlocal->err : 0;
 }
-
-/* These functions keep track of the number of running threads. */
-static pthread_mutex_t running_threads_lock = PTHREAD_MUTEX_INITIALIZER;
-static size_t running_threads = 0;
-
-size_t
-get_running_threads (void)
-{
-  size_t r;
-
-  pthread_mutex_lock (&running_threads_lock);
-  r = running_threads;
-  pthread_mutex_unlock (&running_threads_lock);
-  return r;
-}
-
-void
-incr_running_threads (void)
-{
-  pthread_mutex_lock (&running_threads_lock);
-  running_threads++;
-  pthread_mutex_unlock (&running_threads_lock);
-}
-
-void
-decr_running_threads (void)
-{
-  pthread_mutex_lock (&running_threads_lock);
-  running_threads--;
-  pthread_mutex_unlock (&running_threads_lock);
-}
-- 
2.13.2

There are several race conditions where nbdkit exits (calling
plugin_cleanup() which unloads the plugin), while the plugin is either
in a callback or in plugin.close().

Avoid these by:

(1) Taking a shared lock around all plugin callbacks.

(2) Taking the same as an exclusive lock in plugin_cleanup.

This delays plugin_cleanup until all callbacks have finished.

(3) Removing a few unnecessary ‘assert (dl)’.

This is necessary because the plugin can now be unloaded while holding
locks.  ‘dl’ is not needed to lock or unlock the plugin so asserting
it is useless.

(4) Don't call plugin.close on the quit path.

Another thread could be unloading the plugin so we cannot call
the .close callback.
---
 src/connections.c | 10 ++++++++--
 src/plugins.c     | 20 +++++++++++++-------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index 46609f0..0cbf54a 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -224,8 +224,14 @@ free_connection (struct connection *conn)
 
   pthread_mutex_destroy (&conn->request_lock);
 
-  if (conn->handle)
-    plugin_close (conn);
+  /* Don't call the plugin again if quit has been set because the main
+   * thread will be in the process of unloading it.  The plugin.unload
+   * callback should always be called.
+   */
+  if (!quit) {
+    if (conn->handle)
+      plugin_close (conn);
+  }
 
   free (conn);
 }
diff --git a/src/plugins.c b/src/plugins.c
index d2d0b39..f5056d9 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -48,6 +48,7 @@
 
 static pthread_mutex_t connection_lock = PTHREAD_MUTEX_INITIALIZER;
 static pthread_mutex_t all_requests_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_rwlock_t unload_prevention_lock = PTHREAD_RWLOCK_INITIALIZER;
 
 /* Maximum read or write request that we will handle. */
 #define MAX_REQUEST_SIZE (64 * 1024 * 1024)
@@ -155,6 +156,11 @@ void
 plugin_cleanup (void)
 {
   if (dl) {
+    /* Acquiring this lock prevents any plugin callbacks from running
+     * simultaneously.
+     */
+    pthread_rwlock_wrlock (&unload_prevention_lock);
+
     debug ("%s: unload", filename);
     if (plugin.unload)
       plugin.unload ();
@@ -163,6 +169,8 @@ plugin_cleanup (void)
     dl = NULL;
     free (filename);
     filename = NULL;
+
+    pthread_rwlock_unlock (&unload_prevention_lock);
   }
 }
 
@@ -299,8 +307,6 @@ plugin_config_complete (void)
 void
 plugin_lock_connection (void)
 {
-  assert (dl);
-
   if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) {
     debug ("%s: acquire connection lock", filename);
     pthread_mutex_lock (&connection_lock);
@@ -310,8 +316,6 @@ plugin_lock_connection (void)
 void
 plugin_unlock_connection (void)
 {
-  assert (dl);
-
   if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) {
     debug ("%s: release connection lock", filename);
     pthread_mutex_unlock (&connection_lock);
@@ -321,8 +325,6 @@ plugin_unlock_connection (void)
 void
 plugin_lock_request (struct connection *conn)
 {
-  assert (dl);
-
   if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) {
     debug ("acquire global request lock");
     pthread_mutex_lock (&all_requests_lock);
@@ -332,12 +334,16 @@ plugin_lock_request (struct connection *conn)
     debug ("acquire per-connection request lock");
     pthread_mutex_lock (connection_get_request_lock (conn));
   }
+
+  debug ("acquire unload prevention lock");
+  pthread_rwlock_rdlock (&unload_prevention_lock);
 }
 
 void
 plugin_unlock_request (struct connection *conn)
 {
-  assert (dl);
+  debug ("release unload prevention lock");
+  pthread_rwlock_unlock (&unload_prevention_lock);
 
   if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS) {
     debug ("release per-connection request lock");
-- 
2.13.2

This was underspecified before.
---
 docs/nbdkit-plugin.pod | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index ceaef1a..47a46da 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -240,6 +240,7 @@ This may be called once just before the plugin is unloaded
from
 memory.  Note that it's not guaranteed that C<.unload> will always be
 called (eg. the server might be killed or segfault), so you should try
 to make the plugin as robust as possible by not requiring cleanup.
+See also L</SHUTDOWN> below.
 
 =head2 C<.config>
 
@@ -528,6 +529,16 @@ If none of the above thread models are suitable, then use
 C<NBDKIT_THREAD_MODEL_PARALLEL> and implement your own locking using
 C<pthread_mutex_t> etc.
 
+=head1 SHUTDOWN
+
+When nbdkit receives certain signals it will shut down (see
+L<nbdkit(1)/SIGNALS>).  The server will wait for any currently running
+plugin callbacks to finish and also call the C<.unload> callback
+before unloading the plugin.
+
+Note that it's not guaranteed this can always happen (eg. the server
+might be killed by C<SIGKILL> or segfault).
+
 =head1 PARSING SIZE PARAMETERS
 
 Use the C<nbdkit_parse_size> utility function to parse human-readable
-- 
2.13.2

On 11/14/2017 11:30 AM, Richard W.M. Jones wrote:
> This was underspecified before.
> ---
>  docs/nbdkit-plugin.pod | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> +=head1 SHUTDOWN
> +
> +When nbdkit receives certain signals it will shut down (see
> +L<nbdkit(1)/SIGNALS>).  The server will wait for any currently
running
> +plugin callbacks to finish and also call the C<.unload> callback
> +before unloading the plugin.
Do we also guarantee that all subsequent client requests parsed off the
wire in between the signal and when we finally call the .unload()
callback will NOT call into the plugin, but will be immediately answered
to the client with NBD_ESHUTDOWN?  If not, we should.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 11/14/2017 11:30 AM, Richard W.M. Jones wrote:
> There are several race conditions where nbdkit exits (calling
> plugin_cleanup() which unloads the plugin), while the plugin is either
> in a callback or in plugin.close().
> 
> Avoid these by:
> 
> (1) Taking a shared lock around all plugin callbacks.
> 
> (2) Taking the same as an exclusive lock in plugin_cleanup.
> 
> This delays plugin_cleanup until all callbacks have finished.
> 
> (3) Removing a few unnecessary ‘assert (dl)’.
> 
> This is necessary because the plugin can now be unloaded while holding
> locks.  ‘dl’ is not needed to lock or unlock the plugin so asserting
> it is useless.
> 
> (4) Don't call plugin.close on the quit path.
> 
> Another thread could be unloading the plugin so we cannot call
> the .close callback.
Yes, this looks like it solves the same race in a much nicer manner.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 11/14/2017 11:30 AM, Richard W.M. Jones wrote:
> This fixes the race conditions for me, using the test described here:
> 
> https://www.redhat.com/archives/libguestfs/2017-September/msg00226.html
Running test-socket-activation in a loop, I've hit other races (some
provoked a bit more easily with the code I'm working on), and will be
posting some patches where I know the solution:

Right now, our use of threadlocal_set_name (plugin_name ()) makes our
thread-local storage point to a string in module memory. If we have any
nbdkit_debug() or other statement that prints after .unload, we get a
SEGV.  Solution(s): initializing a plugin should strdup() the name so
that plugin_name() is valid for the life of the program, rather than
pointing into module memory; and threadlocal_set_name() should strdup()
the name rather than relying on the lifetime of the storage of whatever
the caller passed in (either fix in isolation solves that SEGV, but
using both seems like a good idea).

We also have a race that results in a hang in
sockets.c:accept_incoming_connections(); if I understand the problem, it
is something like:

main                    signal context
--------------------------------------
first iteration, finish accept_connect()
checks !quit, starts second iteration
                        SIGTERM received
                        set quit
call poll()


which hangs trying to connect to the socket, compared to the usual:

main                    signal context
--------------------------------------
first iteration, finish accept_connect()
                        SIGTERM received
                        set quit
checks !quit, ends loop


or:

main                    signal context
--------------------------------------
first iteration, finish accept_connect()
checks !quit, starts second iteration
call poll()
                        SIGTERM received
                        set quit
poll() fails with EINTR
checks !quit, ends loop


Here, I'm less sure of what would reliably solve things; maybe we want a
pipe-to-self setup where our signal handler (which currently sets !quit)
instead (or in addition) writes into the pipe so that our event loop
(the poll() in accept_connect()) has an additional fd it can poll in
parallel to all the socket fds; that way, the arrival of a signal will
ensure that poll() wakes up even without any of the pending socket fds
receiving another connection.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Thu, Nov 16, 2017 at 05:09:57PM -0600, Eric Blake
wrote:
> Here, I'm less sure of what would reliably solve things; maybe we want
a
> pipe-to-self setup where our signal handler (which currently sets !quit)
> instead (or in addition) writes into the pipe so that our event loop
> (the poll() in accept_connect()) has an additional fd it can poll in
> parallel to all the socket fds; that way, the arrival of a signal will
> ensure that poll() wakes up even without any of the pending socket fds
> receiving another connection.
A pipe-to-self does sound like the better solution here.  poll() will
return immediately that way.

However the implementation of that will be horrible since I think
we'll finally need to keep track of each thread.  OTOH keeping track
of each thread also seems to be necessary to solve the problem you
mentioned on IRC where we pass stack data to pthread_create.

If you want me to look at fixing any of these things let me know,
otherwise I'll just wait & review patches as posted.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html