Re: [Libguestfs] [nbdkit PATCH] server: Restrict thread model when no atomic CLOEXEC

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(a)redhat.com&gt; --- 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