Libguestfs - Aug 2019 - Re: [nbdkit PATCH v2 10/17] plugins: Add .fork_safe field

On Fri, Aug 02, 2019 at 02:26:11PM -0500, Eric Blake
wrote:
> Allow a plugin field to declare whether a parallel plugin can tolerate
> windows where fds are not CLOEXEC, or must take precautions to avoid
> leaking fds if the plugin may fork.  For safety reasons, the flag
> defaults to off, but many in-tree plugins can set it to on (most
> commonly because they don't fork after .config_complete; for libvirt
> because it is documented to clean up fds on fork so it is immune to
> anything we might leak; for libnbd because we don't use the API that
> forks).  Note that I did not choose to set the new field for many of
> the various language bindings (it becomes a rather difficult task to
> prove whether the third-party language binding code is itself using
> atomic CLOEXEC or fd sanitization).  However many of our languages are
> still stuck as serialized, and the lack of .fork_safe won't impact
> those thread model anyways.
> 
> Update the testsuite to skip parallel tests that would otherwise fail
> when the thread model is crippled.
> 
> Upcoming patches will then fix the server to audit and fix places
> where we currently leak fds, and then cripple the thread model only on
> platforms where atomic CLOEXEC is not possible.
My worry about this patch is we're adding a new plugin flag which
we'll have to support forever, but IIUC it's only needed on one
platform (ie. Haiku) which really ought to get fixed.  In future we'll
end up in a situation where we have this flag but it's no longer
needed.

How about instead of this we simply restrict Haiku to the fully
serialized mode.  Sucks for them, but they can fix it by adding atomic
CLOEXEC features ...
> +  .fork_safe         = 0, /* libguestfs uses fork(), unaudited for safety
*/
libguestfs does use fork and should be safe - we're pretty careful
about using CLOEXEC, accept4, etc everywhere.  (Also libguestfs
doesn't run on Haiku and architecturally that's unlikely to ever
happen).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 8/2/19 2:52 PM, Richard W.M. Jones wrote:
> On Fri, Aug 02, 2019 at 02:26:11PM -0500, Eric Blake wrote:
>> Allow a plugin field to declare whether a parallel plugin can tolerate
>> windows where fds are not CLOEXEC, or must take precautions to avoid
>> leaking fds if the plugin may fork.  For safety reasons, the flag
>> defaults to off, but many in-tree plugins can set it to on (most
>> commonly because they don't fork after .config_complete; for
libvirt
>> because it is documented to clean up fds on fork so it is immune to
>> anything we might leak; for libnbd because we don't use the API
that
>> forks).  Note that I did not choose to set the new field for many of
>> the various language bindings (it becomes a rather difficult task to
>> prove whether the third-party language binding code is itself using
>> atomic CLOEXEC or fd sanitization).  However many of our languages are
>> still stuck as serialized, and the lack of .fork_safe won't impact
>> those thread model anyways.
>>
>> Update the testsuite to skip parallel tests that would otherwise fail
>> when the thread model is crippled.
>>
>> Upcoming patches will then fix the server to audit and fix places
>> where we currently leak fds, and then cripple the thread model only on
>> platforms where atomic CLOEXEC is not possible.
> 
> My worry about this patch is we're adding a new plugin flag which
> we'll have to support forever, but IIUC it's only needed on one
> platform (ie. Haiku) which really ought to get fixed.  In future we'll
> end up in a situation where we have this flag but it's no longer
> needed.
Yeah, the fact that it is a no-op on Linux means it's hard to test.

Still, the idea of penalizing all plugins, even though only the forking
plugins care, is a bit hard to swallow - maybe that will prod Haiku in
catching up faster.
> How about instead of this we simply restrict Haiku to the fully
> serialized mode.  Sucks for them, but they can fix it by adding atomic
> CLOEXEC features ...
Makes sense. I'll still want to ensure that --dump-plugin shows the
dynamic crippling (so that we can easily skip
tests/test-parallel-file.sh, for example), but I think I can manage to
do that by replacing this patch with just blind penalization of
non-atomic CLOEXEC.
> 
>> +  .fork_safe         = 0, /* libguestfs uses fork(), unaudited for
safety */
> 
> libguestfs does use fork and should be safe - we're pretty careful
> about using CLOEXEC, accept4, etc everywhere.  (Also libguestfs
> doesn't run on Haiku and architecturally that's unlikely to ever
> happen).
Nice to know. That was half of the review - determining which libraries
use which functions, and what might still need fixing (or bugs filed to
other projects).  It also looks like libnbd is using CLOEXEC everywhere
that it should.  But as long as the fault is no longer nbdkit's, any
further bugs we get about fd leaks can be redirected to whoever is
actually doing the leak.

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

On platforms that lack atomic CLOEXEC, it's simpler to just always
force serialization (no client thread will be executing when nbdkit
itself is creating a new file descriptor) than to audit which plugins
actually care about not getting any leaked fds.  We have one final
function that we need to use for atomic CLOEXEC; the next patch will
actually put accept4() to use.

Maybe this penalization will encourage Haiku to implement atomic
CLOEXEC sooner.

Accordingly, the testsuite is tweaked to skip tests that require
parallel execution.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

This replaces my .fork_safe patch; I've rebased the rest of the series
on top of it, including fixing the sh default to serialize if there is
no explicit opt-in to parallel, and pushed.

 docs/nbdkit-plugin.pod      | 19 +++++++++++++++----
 configure.ac                |  1 +
 common/utils/utils.c        |  6 ++++--
 server/plugins.c            |  9 +++++++++
 tests/test-parallel-file.sh |  3 +++
 tests/test-parallel-nbd.sh  |  5 ++++-
 6 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index fe9ada87..9510253f 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -942,8 +942,16 @@ C<NBDKIT_REGISTER_PLUGIN>).  Additionally, a plugin
may implement the
 C<.thread_model> callback, called right after C<.config_complete>
to
 make a runtime decision on which thread model to use.  The nbdkit
 server chooses the most restrictive model between the plugin's
-C<THREAD_MODEL>, the C<.thread_model> if present, and any
restrictions
-requested by filters.
+C<THREAD_MODEL>, the C<.thread_model> if present, any restrictions
+requested by filters, and any limitations imposed by the system (for
+example, a system without atomic C<FD_CLOEXEC> will serialize all
+requests, so as to avoid nbdkit leaking a new file descriptor from one
+thread into a child process created by another thread).
+
+In C<nbdkit --dump-plugin PLUGIN> output, the C<max_thread_model>
line
+matches the C<THREAD_MODEL> macro, and the C<thread_model> line
+matches what the system finally settled on after applying all
+restrictions.

 The possible settings for C<THREAD_MODEL> are defined below.

@@ -981,8 +989,11 @@ Multiple handles can be open and multiple data requests can
happen in
 parallel (even on the same handle).  The server may reorder replies,
 answering a later request before an earlier one.

-All the libraries you use must be thread-safe and reentrant.  You may
-also need to provide mutexes for fields in your connection handle.
+All the libraries you use must be thread-safe and reentrant, and any
+code that creates a file descriptor should atomically set
+C<FD_CLOEXEC> if you do not want it accidentally leaked to another
+thread's child process.  You may also need to provide mutexes for
+fields in your connection handle.

 =back

diff --git a/configure.ac b/configure.ac
index cabec5c8..054ad64a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -193,6 +193,7 @@ AC_CHECK_HEADERS([\

 dnl Check for functions in libc, all optional.
 AC_CHECK_FUNCS([\
+	accept4 \
 	fdatasync \
 	get_current_dir_name \
 	mkostemp \
diff --git a/common/utils/utils.c b/common/utils/utils.c
index 9ac3443b..029b6685 100644
--- a/common/utils/utils.c
+++ b/common/utils/utils.c
@@ -140,13 +140,15 @@ exit_status_to_nbd_error (int status, const char *cmd)
  */
 int
 set_cloexec (int fd) {
-#if defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined
HAVE_PIPE2
+#if (defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined
HAVE_PIPE2 && \
+     defined HAVE_ACCEPT4)
   nbdkit_error ("prefer creating fds with CLOEXEC atomically set");
   close (fd);
   errno = EBADF;
   return -1;
 #else
-# if defined SOCK_CLOEXEC || defined HAVE_MKOSTEMP || defined HAVE_PIPE2
+# if (defined SOCK_CLOEXEC || defined HAVE_MKOSTEMP || defined HAVE_PIPE2 || \
+      !defined HAVE_ACCEPT4)
 # error "Unexpected: your system has incomplete atomic CLOEXEC
support"
 # endif
   int f;
diff --git a/server/plugins.c b/server/plugins.c
index 3bb20c93..be952504 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -39,6 +39,7 @@
 #include <inttypes.h>
 #include <assert.h>
 #include <errno.h>
+#include <sys/socket.h>

 #include <dlfcn.h>

@@ -90,6 +91,14 @@ plugin_thread_model (struct backend *b)
   int thread_model = p->plugin._thread_model;
   int r;

+#if !(defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined
HAVE_PIPE2 && \
+      defined HAVE_ACCEPT4)
+  if (thread_model > NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) {
+    nbdkit_debug ("system lacks atomic CLOEXEC, serializing to avoid fd
leaks");
+    thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS;
+  }
+#endif
+
   if (p->plugin.thread_model) {
     r = p->plugin.thread_model ();
     if (r == -1)
diff --git a/tests/test-parallel-file.sh b/tests/test-parallel-file.sh
index 8335dc99..20276d48 100755
--- a/tests/test-parallel-file.sh
+++ b/tests/test-parallel-file.sh
@@ -36,6 +36,9 @@ source ./functions.sh
 requires test -f file-data
 requires qemu-io --version

+nbdkit --dump-plugin file | grep -q ^thread_model=parallel ||
+    { echo "nbdkit lacks support for parallel requests"; exit 77; }
+
 cleanup_fn rm -f test-parallel-file.data test-parallel-file.out

 # Populate file, and sanity check that qemu-io can issue parallel requests
diff --git a/tests/test-parallel-nbd.sh b/tests/test-parallel-nbd.sh
index 36088fa1..4e546df4 100755
--- a/tests/test-parallel-nbd.sh
+++ b/tests/test-parallel-nbd.sh
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 # nbdkit
-# Copyright (C) 2017-2018 Red Hat Inc.
+# Copyright (C) 2017-2019 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -36,6 +36,9 @@ source ./functions.sh
 requires test -f file-data
 requires qemu-io --version

+nbdkit --dump-plugin nbd | grep -q ^thread_model=parallel ||
+    { echo "nbdkit lacks support for parallel requests"; exit 77; }
+
 sock=`mktemp -u`
 files="test-parallel-nbd.out $sock test-parallel-nbd.data
test-parallel-nbd.pid"
 rm -f $files
-- 
2.20.1