[Libguestfs] [nbdkit PATCH] server: Restrict thread model when no atomic CLOEXEC
Richard W.M. Jones
rjones at redhat.com
Sat Aug 3 06:45:32 UTC 2019
On Fri, Aug 02, 2019 at 05:33:46PM -0500, Eric Blake wrote:
> 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 at 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
ACK
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
More information about the Libguestfs
mailing list