2:26 p.m.
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.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
I'm open to bikeshed ideas on a better name; perhaps
'.tolerate_fd_leak'? The goal is the following conditional:
- nbdkit doesn't leak fds: no impact to thread model
- plugin doesn't care about leaked fds (either it doesn't fork, or
it scrubs unknown fds after fork): no impact to thread model
- plugin does not want leaked fds (including historical plugins
that didn't state one way or another): cripple thread model to
ensure no leaked fds
---
docs/nbdkit-plugin.pod | 21 +++++++++++++++++----
include/nbdkit-plugin.h | 1 +
plugins/curl/curl.c | 1 +
plugins/data/data.c | 1 +
plugins/example2/example2.c | 1 +
plugins/example3/example3.c | 1 +
plugins/ext2/ext2.c | 1 +
plugins/file/file.c | 1 +
plugins/floppy/floppy.c | 1 +
plugins/full/full.c | 1 +
plugins/guestfs/guestfs-plugin.c | 1 +
plugins/gzip/gzip.c | 1 +
plugins/iso/iso.c | 1 +
plugins/libvirt/libvirt-plugin.c | 1 +
plugins/linuxdisk/linuxdisk.c | 1 +
plugins/memory/memory.c | 1 +
plugins/nbd/nbd-standalone.c | 1 +
plugins/nbd/nbd.c | 1 +
plugins/null/null.c | 1 +
plugins/partitioning/partitioning.c | 1 +
plugins/pattern/pattern.c | 1 +
plugins/random/random.c | 1 +
plugins/sh/sh.c | 1 +
plugins/split/split.c | 1 +
plugins/ssh/ssh.c | 1 +
plugins/streaming/streaming.c | 1 +
plugins/zero/zero.c | 1 +
server/plugins.c | 10 ++++++++++
tests/test-parallel-file.sh | 3 +++
tests/test-parallel-nbd.sh | 5 ++++-
30 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index fe9ada87..6639de61 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -920,8 +920,8 @@ error message, and C<nbdkit_set_error> to record an appropriate
error
=head1 OTHER FIELDS
-The plugin struct also contains an integer field used as a
-boolean in C code, but unlikely to be exposed in other language
+The plugin struct also contains a couple of integer fields used as
+booleans in C code, but unlikely to be exposed in other language
bindings:
=over 4
@@ -932,6 +932,18 @@ This defaults to 0; if non-zero, nbdkit can reliably use the value
of
C<errno> when a callback reports failure, rather than the plugin
having to call C<nbdkit_set_error>.
+=item C<.fork_safe>
+
+This defaults to 0; if non-zero, the plugin either does not create
+child processes via L<fork(2)> or similar, or else takes care to avoid
+leaking a file descriptor created in one thread to a child process
+created in another (either by always using atomic C<FD_CLOEXEC>, or by
+closing unexpected fds between fork and exec). On platforms where
+this is 0 and nbdkit cannot atomically set C<FD_CLOEXEC>, the thread
+model will be restricted to
+C<NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS> to avoid leaking file
+descriptors.
+
=back
=head1 THREADS
@@ -942,8 +954,9 @@ 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 platform-specific limitations implied if
+C<.fork_safe> is not set.
The possible settings for C<THREAD_MODEL> are defined below.
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 632df867..75e08651 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -132,6 +132,7 @@ struct nbdkit_plugin {
int (*cache) (void *handle, uint32_t count, uint64_t offset, uint32_t flags);
int (*thread_model) (void);
+ int fork_safe;
};
extern void nbdkit_set_error (int err);
diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
index 7e3e8c92..192bae4e 100644
--- a/plugins/curl/curl.c
+++ b/plugins/curl/curl.c
@@ -586,6 +586,7 @@ static struct nbdkit_plugin plugin = {
.get_size = curl_get_size,
.pread = curl_pread,
.pwrite = curl_pwrite,
+ .fork_safe = 1, /* libcurl does not appear to use fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/data/data.c b/plugins/data/data.c
index 14fb8997..6e7d4296 100644
--- a/plugins/data/data.c
+++ b/plugins/data/data.c
@@ -433,6 +433,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/example2/example2.c b/plugins/example2/example2.c
index 6adc100a..0ce081c8 100644
--- a/plugins/example2/example2.c
+++ b/plugins/example2/example2.c
@@ -224,6 +224,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/example3/example3.c b/plugins/example3/example3.c
index 5d2b626c..0037349c 100644
--- a/plugins/example3/example3.c
+++ b/plugins/example3/example3.c
@@ -232,6 +232,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/ext2/ext2.c b/plugins/ext2/ext2.c
index 6698d99f..d6f6d792 100644
--- a/plugins/ext2/ext2.c
+++ b/plugins/ext2/ext2.c
@@ -358,6 +358,7 @@ static struct nbdkit_plugin plugin = {
.pwrite = ext2_pwrite,
.flush = ext2_flush,
.errno_is_preserved = 1,
+ .fork_safe = 1, /* ext2fs does not appear to use fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/file/file.c b/plugins/file/file.c
index 9df5001d..d71c2379 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -667,6 +667,7 @@ static struct nbdkit_plugin plugin = {
.cache = file_cache,
#endif
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/floppy/floppy.c b/plugins/floppy/floppy.c
index 41a23644..bcec4074 100644
--- a/plugins/floppy/floppy.c
+++ b/plugins/floppy/floppy.c
@@ -210,6 +210,7 @@ static struct nbdkit_plugin plugin = {
.can_cache = floppy_can_cache,
.pread = floppy_pread,
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/full/full.c b/plugins/full/full.c
index 9cfbcfcd..7d4d2ba3 100644
--- a/plugins/full/full.c
+++ b/plugins/full/full.c
@@ -179,6 +179,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/guestfs/guestfs-plugin.c b/plugins/guestfs/guestfs-plugin.c
index b0061e26..200b0094 100644
--- a/plugins/guestfs/guestfs-plugin.c
+++ b/plugins/guestfs/guestfs-plugin.c
@@ -593,6 +593,7 @@ static struct nbdkit_plugin plugin = {
.pread = plugin_guestfs_pread,
.pwrite = plugin_guestfs_pwrite,
.flush = plugin_guestfs_flush,
+ .fork_safe = 0, /* libguestfs uses fork(), unaudited for safety */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/gzip/gzip.c b/plugins/gzip/gzip.c
index c6baa52e..5821d468 100644
--- a/plugins/gzip/gzip.c
+++ b/plugins/gzip/gzip.c
@@ -222,6 +222,7 @@ static struct nbdkit_plugin plugin = {
.close = gzip_close,
.get_size = gzip_get_size,
.pread = gzip_pread,
+ .fork_safe = 1, /* zlib does not appear to use fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/iso/iso.c b/plugins/iso/iso.c
index 5634bac9..bb942f85 100644
--- a/plugins/iso/iso.c
+++ b/plugins/iso/iso.c
@@ -260,6 +260,7 @@ static struct nbdkit_plugin plugin = {
.can_cache = iso_can_cache,
.pread = iso_pread,
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no fork()s after .config_complete */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/libvirt/libvirt-plugin.c b/plugins/libvirt/libvirt-plugin.c
index 71cac42b..cd4e326c 100644
--- a/plugins/libvirt/libvirt-plugin.c
+++ b/plugins/libvirt/libvirt-plugin.c
@@ -215,6 +215,7 @@ static struct nbdkit_plugin plugin = {
.close = virt_close,
.get_size = virt_get_size,
.pread = virt_pread,
+ .fork_safe = 1, /* libvirt uses fork(), but does so safely */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/linuxdisk/linuxdisk.c b/plugins/linuxdisk/linuxdisk.c
index 99dbc996..24e6b8b2 100644
--- a/plugins/linuxdisk/linuxdisk.c
+++ b/plugins/linuxdisk/linuxdisk.c
@@ -232,6 +232,7 @@ static struct nbdkit_plugin plugin = {
.can_cache = linuxdisk_can_cache,
.pread = linuxdisk_pread,
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no fork()s after .config_complete */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/memory/memory.c b/plugins/memory/memory.c
index 09162ea2..0f3fd86d 100644
--- a/plugins/memory/memory.c
+++ b/plugins/memory/memory.c
@@ -231,6 +231,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/nbd/nbd-standalone.c b/plugins/nbd/nbd-standalone.c
index 1468098b..4f1224c7 100644
--- a/plugins/nbd/nbd-standalone.c
+++ b/plugins/nbd/nbd-standalone.c
@@ -1365,6 +1365,7 @@ static struct nbdkit_plugin plugin = {
.extents = nbd_extents,
.cache = nbd_cache,
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN (plugin)
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index 95d910e7..7762c9c0 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -861,6 +861,7 @@ static struct nbdkit_plugin plugin = {
.extents = nbdplug_extents,
.cache = nbdplug_cache,
.errno_is_preserved = 1,
+ .fork_safe = 1, /* libnbd uses fork() but not for URIs */
};
NBDKIT_REGISTER_PLUGIN (plugin)
diff --git a/plugins/null/null.c b/plugins/null/null.c
index 647624ba..3e7161b7 100644
--- a/plugins/null/null.c
+++ b/plugins/null/null.c
@@ -178,6 +178,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/partitioning/partitioning.c b/plugins/partitioning/partitioning.c
index 90333bf7..1046579a 100644
--- a/plugins/partitioning/partitioning.c
+++ b/plugins/partitioning/partitioning.c
@@ -442,6 +442,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/pattern/pattern.c b/plugins/pattern/pattern.c
index 6b9b3a0d..97418127 100644
--- a/plugins/pattern/pattern.c
+++ b/plugins/pattern/pattern.c
@@ -142,6 +142,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/random/random.c b/plugins/random/random.c
index 6377310e..ebe97e85 100644
--- a/plugins/random/random.c
+++ b/plugins/random/random.c
@@ -172,6 +172,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 5869d9a4..669008e9 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -960,6 +960,7 @@ static struct nbdkit_plugin plugin = {
.cache = sh_cache,
.errno_is_preserved = 1,
+ .fork_safe = 0, /* use of fork() is not yet safe from fd leaks */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/split/split.c b/plugins/split/split.c
index 68682b29..babbaf4a 100644
--- a/plugins/split/split.c
+++ b/plugins/split/split.c
@@ -339,6 +339,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
index d3fa2931..43f71e41 100644
--- a/plugins/ssh/ssh.c
+++ b/plugins/ssh/ssh.c
@@ -632,6 +632,7 @@ static struct nbdkit_plugin plugin = {
.pwrite = ssh_pwrite,
.can_flush = ssh_can_flush,
.flush = ssh_flush,
+ .fork_safe = 0, /* libssh uses fork(), unaudited for safety */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/streaming/streaming.c b/plugins/streaming/streaming.c
index 4ab73b8b..315ff689 100644
--- a/plugins/streaming/streaming.c
+++ b/plugins/streaming/streaming.c
@@ -256,6 +256,7 @@ static struct nbdkit_plugin plugin = {
.pwrite = streaming_pwrite,
.pread = streaming_pread,
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/zero/zero.c b/plugins/zero/zero.c
index b240502c..27b8fe18 100644
--- a/plugins/zero/zero.c
+++ b/plugins/zero/zero.c
@@ -95,6 +95,7 @@ static struct nbdkit_plugin plugin = {
* paths from failed system calls.
*/
.errno_is_preserved = 1,
+ .fork_safe = 1, /* no use of fork() */
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/server/plugins.c b/server/plugins.c
index 3bb20c93..0e6c05b0 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -90,6 +90,15 @@ plugin_thread_model (struct backend *b)
int thread_model = p->plugin._thread_model;
int r;
+ /* For now, we leak fds on all platforms; once that is fixed, this
+ * restriction can be limited to only occur when !HAVE_ACCEPT4.
+ */
+ if (p->plugin._thread_model > NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
&&
+ p->plugin.fork_safe == 0) {
+ nbdkit_debug (".fork_safe not set, serializing to avoid fd leaks");
+ thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS;
+ }
+
if (p->plugin.thread_model) {
r = p->plugin.thread_model ();
if (r == -1)
@@ -187,6 +196,7 @@ plugin_dump_fields (struct backend *b)
plugin_dump_thread_model ("max_thread_model", p->plugin._thread_model);
plugin_dump_thread_model ("thread_model", backend->thread_model
(backend));
printf ("errno_is_preserved=%d\n", !!p->plugin.errno_is_preserved);
+ printf ("fork_safe=%d\n", !!p->plugin.fork_safe);
if (p->plugin.magic_config_key)
printf ("magic_config_key=%s\n", p->plugin.magic_config_key);
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