February 2023 - Libguestfs - libguestfs List Archives

[cross-project PATCH v2] NBD 64-bit extensions
by Eric Blake 06 Mar '23

06 Mar '23

This is a cover letter for a set of multi-project patch series all designed to implement 64-bit operations in NBD, and demonstrate interoperability of the new extension between projects. v1 of the project was attempted nearly a year ago: https://lists.nongnu.org/archive/html/qemu-devel/2021-12/msg00453.html Since then, I've addressed a lot of preliminary cleanups in libnbd to make it easier to test things, and incorporated review comments issued on v1, including: - no orthogonality between simple/structured replies and 64-bit mode: once extended headers are negotiated, all transmission traffic (both from client to server and server to client) uses just one header size - add support for the client to pass a payload on commands to the server, and demonstrate it by further implementing a way to pass a flag with NBD_CMD_BLOCK_STATUS that says the client is passing a payload to request status of just a subset of the negotiated contexts, rather than all possible contexts that were earlier negotiated during NBD_OPT_SET_META_CONTEXT - tweaks to the header layouts: tweak block status to provide 64-bit flags values (although "base:allocation" will remain usable with just 32-bit flags); reply with offset rather than padding and with fields rearranged for maximal sharing between client and server layouts - word-smithing of the NBD proposed protocol; split things into several smaller patches, where we can choose how much to take - more unit tests added in qemu and libnbd - the series end with RFC patches on whether to support 64-bit hole responses to NBD_CMD_READ, even though our current limitations say servers don't have to accept more than a 32M read request and therefore will never have that big of a hole to read) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

5 64

[V2V PATCH 0/5] Bring support for virtio-scsi back to Windows
by Andrey Drobyshev 03 Mar '23

03 Mar '23

Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 ("Remove guestcaps_block_type Virtio_SCSI") support for installing virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for bringing this feature back have been out there for a while. E.g. I've found a corresponding issue which is still open [1]. The code in b28cd1dc, f0afc439 was removed due to removing the old in-place support. However, having the new in-place support present and bringing this same code (partially) back with several additions and improvements, I'm able to successfully convert and boot a Win guest with a virtio-scsi disk controller. So please consider the following implementation of this feature. [1] https://github.com/libguestfs/virt-v2v/issues/12 v2v: Andrey Drobyshev (2): Revert "Remove guestcaps_block_type Virtio_SCSI" convert_windows: add Inject_virtio_win.Virtio_SCSI as a possible block type convert/convert.ml | 2 +- convert/convert_linux.ml | 9 +++++++-- convert/convert_windows.ml | 1 + convert/target_bus_assignment.ml | 1 + lib/create_ovf.ml | 1 + lib/types.ml | 3 ++- lib/types.mli | 2 +- output/openstack_image_properties.ml | 7 +++++++ 9 files changed, 22 insertions(+), 6 deletions(-) common: Andrey Drobyshev (2): inject_virtio_win: add Virtio_SCSI to block_type inject_virtio_win: make virtio-scsi the default block driver Roman Kagan (1): inject_virtio_win: match only vendor/device mlcustomize/inject_virtio_win.ml | 25 ++++++++++++++++--------- mlcustomize/inject_virtio_win.mli | 2 +- 2 files changed, 17 insertions(+), 10 deletions(-) -- 2.31.1

5 23

Checksums and other verification
by Richard W.M. Jones 02 Mar '23

02 Mar '23

https://github.com/kubevirt/containerized-data-importer/issues/1520 Hi Eric, We had a question from the Kubevirt team related to the above issue. The question is roughly if it's possible to calculate the checksum of an image as an nbdkit filter and/or in the qemu block layer. Supplemental #1: could qemu-img convert calculate a checksum as it goes along? Supplemental #2: could we detect various sorts of common errors, such a webserver that is incorrectly configured and serves up an error page containing "<html>"; or something which is supposed to be a disk image but does not "look like" (in some ill-defined sense) a disk image, eg. it has no partition table. I'm not sure if qemu has any existing features covering the above (and I know for sure that nbdkit doesn't). One issue is that calculating a checksum involves a linear scan of the image, although we can at least skip holes. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit

4 11

[libnbd PATCH v5 0/5] vector: introduce DEFINE_POINTER_VECTOR_TYPE()
by Laszlo Ersek 28 Feb '23

28 Feb '23

V4: http://mid.mail-archive.com/20230224043937.392235-1-lersek@redhat.com Please see the Notes section in each patch for the updates. Laszlo Laszlo Ersek (5): common/include: add TYPE_IS_POINTER() macro common/include: extract STATIC_ASSERT() macro force semicolon after DEFINE_VECTOR_TYPE() macro invocations vector: introduce DEFINE_POINTER_VECTOR_TYPE() convert string_vector_(iter(free) + reset()) to string_vector_empty() common/include/Makefile.am | 1 + common/include/checked-overflow.h | 8 ++-- common/include/compiler-macros.h | 31 ++++++++++++- common/include/static-assert.h | 48 ++++++++++++++++++++ common/include/test-array-size.c | 45 ++++++------------ common/utils/string-vector.h | 2 +- common/utils/test-vector.c | 3 +- common/utils/vector.h | 42 ++++++++++++++++- copy/file-ops.c | 2 +- copy/nbd-ops.c | 2 +- dump/dump.c | 2 +- fuse/nbdfuse.h | 2 +- generator/states-connect-socket-activation.c | 9 ++-- info/list.c | 2 +- info/map.c | 2 +- info/show.c | 3 +- lib/handle.c | 12 ++--- lib/uri.c | 2 +- lib/utils.c | 6 +-- ublk/nbdublk.h | 2 +- ublk/tgt.c | 2 +- 21 files changed, 157 insertions(+), 71 deletions(-) create mode 100644 common/include/static-assert.h base-commit: 594f7a738e0aba23eda37965c96b3df6b8f76960

3 10

[libnbd PATCH v3 00/29] pass LISTEN_FDNAMES with systemd socket activation
by Laszlo Ersek 28 Feb '23

28 Feb '23

Rich posted v2 at: https://listman.redhat.com/archives/libguestfs/2023-January/030555.html This is my stab at the feature, with a number of changes (that I felt were prerequisites) prepended. The series is structured as follows. Note that almost every segment of the series strongly depends on prior segment(s); in other words, the patches are in reasonably strict dependency order. - Patches #1 through #4 are generic, mostly mechanical, tree-wide cleanups, dealing with whitespace and reserved identifiers. - Patch #5 extracts a vector usage pattern we use 11 times in the tree to a new vector API called "name##_empty". - Patches #6 through #8 introduce an assert() variant that is async-signal-safe. This means we can make assertions in a child process before reaching one of the exec functions, even if the parent process was multi-threaded at the time of fork(). - Patches #9 and #10 introduce an execvpe() variant that is async-signal-safe. - Patches #11 through #25 implement the same series of cleanups *twice*, once for CONNECT_SA.START ("socket activation"), and another time for CONNECT_COMMAND.START. The table below describes this sub-structure: change summary CONNECT_SA CONNECT_COMMAND ---------------------------------------------- ---------- --------------- minor thinkos and warts #11 - #13 #18 - #21 check syscalls for errors in the child process #14 #22 centralize resource release #15 #23 plug resource leak on error #16 #24 replace execvp() call with fork-safe variant #17 #25 - Patches #26 through #29 implement the LISTEN_FDNAMES passing. This sub-series includes two patches from Rich's v2 series (with proper attribution). The series buils, and passes "make check" and "make check-valgrind", at every stage. In the Notes sections of some of the patches, there are lines of the form "context:-U<number>" or "context:-W". I've always wanted to control the number of context lines at the *individual* patch level -- for reviewing some patches, the default 3 is just fine, but for reviewing many other patches, significantly larger contexts are beneficial. I've finally bitten the bullet and written a (rather awkward) script that parses this "context:" note out of each patch (where "-W" is the short form of "--function--context") and applies it to formatting. That's why the context size varies over the series. The larger context size may make it harder to apply the series from the list if the master branch advances meanwhile. I don't expect this to happen, but in any case, I've passed "--base=master" to git-format-patch, so that the base commit (the current HEAD of the master branch) be noted at the end of the cover letter. That way the series can be applied precisely, and then rebased with a separate local step (because git has more information that way). Laszlo Laszlo Ersek (27): use space consistently in function and function-like macro invocations generator/C.ml: use space consistently in func. and func.-like macro calls socket activation: rename sa_(tmpdir|sockpath) to sact_(tmpdir|sockpath) ocaml: rename "sa_u" to "saddr_u" vector: (mostly) factor out DEFINE_VECTOR_EMPTY lib/utils: introduce xwrite() as a more robust write() lib/utils: add async-signal-safe assert() lib/utils: add unit test for async-signal-safe assert() lib/utils: introduce async-signal-safe execvpe() lib/utils: add unit tests for async-signal-safe execvpe() socket activation: fix error message upon asprintf() failure socket activation: clean up responsibilities of prep.sock.act.env.() socket activation: avoid manipulating the sign bit socket activation: check syscalls for errors in the child process socket activation: centralize resource release socket activation: plug AF_UNIX socket address (filesystem) leak on error socket activation: replace execvp() call with fork-safe variant CONNECT_COMMAND.START: fix small comment thinko about socket pair usage CONNECT_COMMAND.START: set the NBD error when fcntl() fails CONNECT_COMMAND.START: use symbolic constants for fd#0 and fd#1 CONNECT_COMMAND.START: sanitize close() calls in the child process CONNECT_COMMAND.START: check syscalls for errors in the child process CONNECT_COMMAND.START: centralize resource release CONNECT_COMMAND.START: plug child process leak on error CONNECT_COMMAND.START: replace execvp() call with fork-safe variant socket activation: generalize environment construction socket activation: set LISTEN_FDNAMES Richard W.M. Jones (2): common/include: Copy ascii-ctype functions from nbdkit generator: Add APIs to get/set the socket activation socket name .gitignore | 4 + common/include/Makefile.am | 6 + common/include/array-size.h | 2 +- common/include/ascii-ctype.h | 75 ++++ common/include/byte-swapping.h | 24 +- common/include/checked-overflow.h | 42 +- common/include/compiler-macros.h | 2 +- common/include/iszero.h | 2 +- common/include/minmax.h | 4 +- common/include/test-array-size.c | 26 +- common/include/test-ascii-ctype.c | 88 ++++ common/utils/const-string-vector.h | 2 +- common/utils/nbdkit-string.h | 2 +- common/utils/string-vector.h | 3 +- common/utils/test-human-size.c | 10 +- common/utils/test-vector.c | 7 +- common/utils/vector.h | 45 +- configure.ac | 5 + copy/file-ops.c | 2 +- copy/main.c | 2 +- copy/nbdcopy.h | 2 +- dump/dump.c | 2 +- examples/copy-libev.c | 30 +- examples/list-exports.c | 2 +- examples/strict-structured-reads.c | 2 +- examples/threaded-reads-and-writes.c | 2 +- fuse/nbdfuse.c | 2 +- generator/API.ml | 49 +++ generator/C.ml | 20 +- generator/states-connect-socket-activation.c | 294 +++++++++---- generator/states-connect.c | 123 ++++-- info/main.c | 2 +- info/show.c | 3 +- interop/interop.c | 2 +- lib/Makefile.am | 48 ++- lib/errors.c | 6 +- lib/handle.c | 80 +++- lib/internal.h | 112 +++-- lib/nbd-protocol.h | 12 +- lib/opt.c | 4 +- lib/test-fork-safe-assert.c | 63 +++ lib/test-fork-safe-assert.sh | 31 ++ lib/test-fork-safe-execvpe.c | 117 ++++++ lib/test-fork-safe-execvpe.sh | 270 ++++++++++++ lib/uri.c | 2 +- lib/utils.c | 430 +++++++++++++++++++- ocaml/helpers.c | 8 +- ocaml/nbd-c.h | 6 +- tests/eflags.c | 6 +- tests/get-size.c | 4 +- tests/newstyle-limited.c | 6 +- tests/oldstyle.c | 4 +- ublk/nbdublk.c | 4 +- ublk/tgt.c | 20 +- 54 files changed, 1771 insertions(+), 350 deletions(-) create mode 100644 common/include/ascii-ctype.h create mode 100644 common/include/test-ascii-ctype.c create mode 100644 lib/test-fork-safe-assert.c create mode 100755 lib/test-fork-safe-assert.sh create mode 100644 lib/test-fork-safe-execvpe.c create mode 100755 lib/test-fork-safe-execvpe.sh base-commit: 5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2

4 126

[nbdkit PATCH v2] server: Don't assert on send if client hangs up early
by Eric Blake 27 Feb '23

27 Feb '23

libnbd's copy/copy-nbd-error.sh was triggering an assertion failure in nbdkit (this is the command that triggered the failure, at least with nbdcopy 1.14.2; although future nbdcopy may be fixed independently to more gracefully exit): $ nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M error-pread-rate=0.5 ] null: ... nbdkit: pattern.2: debug: error-inject: pread count=262144 offset=4718592 nbdkit: pattern.2: debug: pattern: pread count=262144 offset=4718592 nbdkit: pattern.1: debug: error-inject: pread count=262144 offset=4456448 nbdkit: pattern.1: error: injecting EIO error into pread nbdkit: pattern.1: debug: sending error reply: Input/output error nbdkit: pattern.3: debug: error-inject: pread count=262144 offset=4980736 nbdkit: pattern.3: debug: pattern: pread count=262144 offset=4980736 nbdkit: pattern.4: error: write data: NBD_CMD_READ: Broken pipe nbdkit: pattern.4: debug: exiting worker thread pattern.4 nbdkit: connections.c:402: raw_send_socket: Assertion `sock >= 0' failed. When there are multiple queued requests, and the client hangs up abruptly as soon as the first error response is seen (rather than waiting until all other queued responses are flushed then sending NBD_CMD_DISC), another thread that has a queued response ready to send sees EPIPE (the client is no longer reading) and closes the socket, then the third thread triggers the assertion failure because it was not expecting to attempt a send to a closed socket. Thus, my claim that sock would always be non-negative after collecting data from the plugin was wrong. The bug can only happen for a server with NBDKIT_THREAD_MODEL_PARALLEL, as that is the only time a single client can have more than one worker thread; and even with parallel service, it is possible to avoid the bug by using 'nbdkit --threads=1' (at the expense of performance, because one client can no longer benefit from interleaved requests). For the nbdcopy example, --exit-with-parent means it's just an annoyance (nbdcopy has already exited, and no further client will be connecting); but for a longer-running nbdkit server accepting multiple clients (particularly likely when advertising MULTI-CONN, although that is not a prerequisite), it means any one client can easily trigger the SIGABRT by intentionally queueing multiple NBD_CMD_READ then disconnecting early, and thereby kill nbdkit and starve other clients. Note that SIGABRT is the most likely outcome; however, there is also a small window for something else: if client 1's first thread triggers conn->close(SHUT_WR) while holding read_lock, there is a window betwen shutdown(conn->sockout) and conn->sockout=-1 where the client's second thread holding write_lock can copy a stale non-negative value of sock=conn->sockout. If during this window a 2nd client connects, an fd allocated as part of the second client can end up with the same value as the stale 'sock' in the first client, where we send data to a completely unrelated socket rather than triggering the assertion failure. Whether this bug rises to the level of CVE depends on whether you consider one client being able to starve others a privilege escalation (if you are not using TLS, there are other ways for a bad client to starve peers; if you are using TLS, then the starved client has the same credentials as the client that caused the SIGABRT or data corruption so there is no privilege boundary escalation). Partially reverts 26fd5e42; protocol_recv_request_send_reply() must once again have a return status, because it is essential that when the client has multiple worker threads, conn->sockout not be modified except under the write_lock, and altering it is easier at a central point where we do not risk deadlock with read_lock or status_lock. Add some testsuite coverage that does not depend on current nbdcopy's abrubt disconnect behavior. The TLS case happens to pass even without this patch, but the plaintext version demonstrates the same core dump that triggered my efforts on this patch. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2173054 Fixes: daef505e ("server: Give client EOF when we are done writing", v1.32.4) --- tests/Makefile.am | 6 +++ server/internal.h | 19 +++++--- server/connections.c | 27 ++++++++--- server/protocol.c | 85 +++++++++++++++------------------- server/public.c | 7 ++- server/test-public.c | 4 +- tests/test-client-death-tls.sh | 81 ++++++++++++++++++++++++++++++++ tests/test-client-death.sh | 60 ++++++++++++++++++++++++ 8 files changed, 224 insertions(+), 65 deletions(-) create mode 100755 tests/test-client-death-tls.sh create mode 100755 tests/test-client-death.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index 15832138..d4717383 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -256,6 +256,8 @@ TESTS += \ test-swap.sh \ test-disconnect.sh \ test-disconnect-tls.sh \ + test-client-death.sh \ + test-client-death-tls.sh \ test-shutdown.sh \ test-nbdkit-backend-debug.sh \ test-read-password.sh \ @@ -299,6 +301,8 @@ EXTRA_DIST += \ test-read-password-plugin.c \ test-disconnect.sh \ test-disconnect-tls.sh \ + test-client-death.sh \ + test-client-death-tls.sh \ test-shutdown.sh \ test-single-from-file.sh \ test-single-sh.sh \ @@ -406,6 +410,8 @@ test_disconnect_plugin_la_LDFLAGS = \ $(NULL) test_disconnect_plugin_la_LIBADD = $(IMPORT_LIBRARY_ON_WINDOWS) +test-client-death-tls.sh: keys.psk + # check_LTLIBRARIES won't build a shared library (see automake manual). # So we have to do this and add a dependency. noinst_LTLIBRARIES += \ diff --git a/server/internal.h b/server/internal.h index 77fdc21d..af5d8677 100644 --- a/server/internal.h +++ b/server/internal.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2022 Red Hat Inc. + * Copyright (C) 2013-2023 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -244,10 +244,13 @@ typedef enum { } conn_status; struct connection { - pthread_mutex_t request_lock; - pthread_mutex_t read_lock; - pthread_mutex_t write_lock; - pthread_mutex_t status_lock; + /* Listed in precedence order: do not grab earlier locks in this list + * while holding a later lock. + */ + pthread_mutex_t request_lock; /* Forces serialization of requests */ + pthread_mutex_t read_lock; /* Read entire client payload off wire */ + pthread_mutex_t write_lock; /* Protect sockout, write response to wire */ + pthread_mutex_t status_lock; /* Track current status of client */ conn_status status; int status_pipe[2]; /* track status changes via poll when nworkers > 1 */ @@ -269,14 +272,16 @@ struct connection { const char *exportname; int sockin, sockout; + /* If nworkers > 1, only call this while read_lock is held */ connection_recv_function recv; + /* If nworkers > 1, only call these while write_lock is held */ connection_send_function send; connection_close_function close; }; extern void handle_single_connection (int sockin, int sockout); extern conn_status connection_get_status (void); -extern void connection_set_status (conn_status value); +extern bool connection_set_status (conn_status value); /* protocol-handshake.c */ extern int protocol_handshake (void); @@ -291,7 +296,7 @@ extern int protocol_handshake_oldstyle (void); extern int protocol_handshake_newstyle (void); /* protocol.c */ -extern void protocol_recv_request_send_reply (void); +extern bool protocol_recv_request_send_reply (void); /* The context ID of base:allocation. As far as I can tell it doesn't * matter what this is as long as nbdkit always returns the same diff --git a/server/connections.c b/server/connections.c index 4d776f2a..6d9f6a96 100644 --- a/server/connections.c +++ b/server/connections.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2022 Red Hat Inc. + * Copyright (C) 2013-2023 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -80,11 +80,14 @@ connection_get_status (void) return r; } -/* Update the status if the new value is lower than the existing value. */ -void +/* Update the status if the new value is lower than the existing value. + * Return true if the caller should shutdown. + */ +bool connection_set_status (conn_status value) { GET_CONN; + bool ret = false; if (conn->nworkers && pthread_mutex_lock (&conn->status_lock)) @@ -99,12 +102,13 @@ connection_set_status (conn_status value) debug ("failed to notify pipe-to-self: %m"); } if (conn->status >= STATUS_CLIENT_DONE && value < STATUS_CLIENT_DONE) - conn->close (SHUT_WR); + ret = true; conn->status = value; } if (conn->nworkers && pthread_mutex_unlock (&conn->status_lock)) abort (); + return ret; } struct worker_data { @@ -126,7 +130,10 @@ connection_worker (void *data) free (worker); while (!quit && connection_get_status () > STATUS_CLIENT_DONE) - protocol_recv_request_send_reply (); + if (protocol_recv_request_send_reply ()) { + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); + conn->close (SHUT_WR); + } debug ("exiting worker thread %s", threadlocal_get_name ()); free (name); return NULL; @@ -178,7 +185,8 @@ handle_single_connection (int sockin, int sockout) /* No need for a separate thread. */ debug ("handshake complete, processing requests serially"); while (!quit && connection_get_status () > STATUS_CLIENT_DONE) - protocol_recv_request_send_reply (); + if (protocol_recv_request_send_reply ()) + conn->close (SHUT_WR); } else { /* Create thread pool to process requests. */ @@ -400,7 +408,10 @@ raw_send_socket (const void *vbuf, size_t len, int flags) ssize_t r; int f = 0; - assert (sock >= 0); + if (sock < 0) { + errno = EBADF; + return -1; + } #ifdef MSG_MORE if (flags & SEND_MORE) f |= MSG_MORE; @@ -492,6 +503,8 @@ raw_recv (void *vbuf, size_t len) /* There's no place in the NBD protocol to send back errors from * close, so this function ignores errors. + * + * If @how == SHUT_WR and conn->nworkers > 1, the caller holds write_lock. */ static void raw_close (int how) diff --git a/server/protocol.c b/server/protocol.c index cc1e4ed8..58f865ef 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2022 Red Hat Inc. + * Copyright (C) 2013-2023 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -362,7 +362,7 @@ nbd_errno (int error, uint16_t flags) } } -static void +static bool send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t flags, const char *buf, uint32_t count, uint32_t error) @@ -380,8 +380,7 @@ send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t flags, r = conn->send (&reply, sizeof reply, f); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } /* Send the read data buffer. */ @@ -389,12 +388,13 @@ send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t flags, r = conn->send (buf, count, 0); if (r == -1) { nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); + return connection_set_status (STATUS_DEAD); } } + return false; } -static void +static bool send_structured_reply_read (uint64_t handle, uint16_t cmd, const char *buf, uint32_t count, uint64_t offset) { @@ -420,8 +420,7 @@ send_structured_reply_read (uint64_t handle, uint16_t cmd, r = conn->send (&reply, sizeof reply, SEND_MORE); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } /* Send the offset + read data buffer. */ @@ -429,15 +428,15 @@ send_structured_reply_read (uint64_t handle, uint16_t cmd, r = conn->send (&offset_data, sizeof offset_data, SEND_MORE); if (r == -1) { nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } r = conn->send (buf, count, 0); if (r == -1) { nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); + return connection_set_status (STATUS_DEAD); } + return false; } /* Convert a list of extents into NBD_REPLY_TYPE_BLOCK_STATUS blocks. @@ -522,7 +521,7 @@ extents_to_block_descriptors (struct nbdkit_extents *extents, return blocks; } -static void +static bool send_structured_reply_block_status (uint64_t handle, uint16_t cmd, uint16_t flags, uint32_t count, uint64_t offset, @@ -542,10 +541,8 @@ send_structured_reply_block_status (uint64_t handle, blocks = extents_to_block_descriptors (extents, flags, count, offset, &nr_blocks); - if (blocks == NULL) { - connection_set_status (STATUS_DEAD); - return; - } + if (blocks == NULL) + return connection_set_status (STATUS_DEAD); reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC); reply.handle = handle; @@ -557,8 +554,7 @@ send_structured_reply_block_status (uint64_t handle, r = conn->send (&reply, sizeof reply, SEND_MORE); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } /* Send the base:allocation context ID. */ @@ -566,8 +562,7 @@ send_structured_reply_block_status (uint64_t handle, r = conn->send (&context_id, sizeof context_id, SEND_MORE); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } /* Send each block descriptor. */ @@ -576,12 +571,13 @@ send_structured_reply_block_status (uint64_t handle, i == nr_blocks - 1 ? 0 : SEND_MORE); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); + return connection_set_status (STATUS_DEAD); } } + return false; } -static void +static bool send_structured_reply_error (uint64_t handle, uint16_t cmd, uint16_t flags, uint32_t error) { @@ -600,8 +596,7 @@ send_structured_reply_error (uint64_t handle, uint16_t cmd, uint16_t flags, r = conn->send (&reply, sizeof reply, SEND_MORE); if (r == -1) { nbdkit_error ("write error reply: %m"); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } /* Send the error. */ @@ -610,12 +605,14 @@ send_structured_reply_error (uint64_t handle, uint16_t cmd, uint16_t flags, r = conn->send (&error_data, sizeof error_data, 0); if (r == -1) { nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); + return connection_set_status (STATUS_DEAD); } /* No human readable error message at the moment. */ + return false; } -void +/* Do a recv/send sequence. Return true if the caller should shutdown. */ +bool protocol_recv_request_send_reply (void) { GET_CONN; @@ -634,24 +631,21 @@ protocol_recv_request_send_reply (void) r = conn->recv (&request, sizeof request); cs = connection_get_status (); if (cs <= STATUS_CLIENT_DONE) - return; + return false; if (r == -1) { nbdkit_error ("read request: %m"); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } if (r == 0) { debug ("client closed input socket, closing connection"); - connection_set_status (STATUS_CLIENT_DONE); /* disconnect */ - return; + return connection_set_status (STATUS_CLIENT_DONE); /* disconnect */ } magic = be32toh (request.magic); if (magic != NBD_REQUEST_MAGIC) { nbdkit_error ("invalid request: 'magic' field is incorrect (0x%x)", magic); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } flags = be16toh (request.flags); @@ -662,16 +656,14 @@ protocol_recv_request_send_reply (void) if (cmd == NBD_CMD_DISC) { debug ("client sent %s, closing connection", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_CLIENT_DONE); /* disconnect */ - return; + return connection_set_status (STATUS_CLIENT_DONE); /* disconnect */ } /* Validate the request. */ if (!validate_request (cmd, flags, offset, count, &error)) { if (cmd == NBD_CMD_WRITE && skip_over_write_buffer (conn->sockin, count) < 0) { - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } goto send_reply; } @@ -685,8 +677,7 @@ protocol_recv_request_send_reply (void) error = ENOMEM; if (cmd == NBD_CMD_WRITE && skip_over_write_buffer (conn->sockin, count) < 0) { - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } goto send_reply; } @@ -711,8 +702,7 @@ protocol_recv_request_send_reply (void) } if (r == -1) { nbdkit_error ("read data: %s: %m", name_of_nbd_cmd (cmd)); - connection_set_status (STATUS_DEAD); - return; + return connection_set_status (STATUS_DEAD); } } } @@ -731,7 +721,7 @@ protocol_recv_request_send_reply (void) /* Send the reply packet. */ send_reply: if (connection_get_status () < STATUS_CLIENT_DONE) - return; + return false; if (error != 0) { /* Since we're about to send only the limited NBD_E* errno to the @@ -752,14 +742,15 @@ protocol_recv_request_send_reply (void) (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) { if (!error) { if (cmd == NBD_CMD_READ) - send_structured_reply_read (request.handle, cmd, buf, count, offset); + return send_structured_reply_read (request.handle, cmd, buf, count, + offset); else /* NBD_CMD_BLOCK_STATUS */ - send_structured_reply_block_status (request.handle, cmd, flags, - count, offset, extents); + return send_structured_reply_block_status (request.handle, cmd, flags, + count, offset, extents); } else - send_structured_reply_error (request.handle, cmd, flags, error); + return send_structured_reply_error (request.handle, cmd, flags, error); } else - send_simple_reply (request.handle, cmd, flags, buf, count, error); + return send_simple_reply (request.handle, cmd, flags, buf, count, error); } diff --git a/server/public.c b/server/public.c index c2f67451..74ac18a1 100644 --- a/server/public.c +++ b/server/public.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2022 Red Hat Inc. + * Copyright (C) 2013-2023 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -1107,5 +1107,8 @@ nbdkit_disconnect (int force) debug ("no connection in this thread, ignoring disconnect request"); return; } - connection_set_status (force ? STATUS_DEAD : STATUS_SHUTDOWN); + if (connection_set_status (force ? STATUS_DEAD : STATUS_SHUTDOWN)) { + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); + conn->close (SHUT_WR); + } } diff --git a/server/test-public.c b/server/test-public.c index e1c48cba..e0249fc7 100644 --- a/server/test-public.c +++ b/server/test-public.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2022 Red Hat Inc. + * Copyright (C) 2018-2023 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -96,7 +96,7 @@ connection_get_status (void) abort (); } -void +bool connection_set_status (conn_status v) { abort (); diff --git a/tests/test-client-death-tls.sh b/tests/test-client-death-tls.sh new file mode 100755 index 00000000..49b3ec18 --- /dev/null +++ b/tests/test-client-death-tls.sh @@ -0,0 +1,81 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2023 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -x + +requires nbdsh -c 'exit(not h.supports_tls())' + + +# Does the nbdkit binary support TLS? +if ! nbdkit --dump-config | grep -sq tls=yes; then + echo "$0: nbdkit built without TLS support" + exit 77 +fi + +# Did we create the PSK keys file? +# Probably 'certtool' is missing. +if [ ! -s keys.psk ]; then + echo "$0: PSK keys file was not created by the test harness" + exit 77 +fi + +sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX) +files="client-death-tls.pid $sock" +cleanup_fn rm -f $files + +# Start long-running nbdkit +start_nbdkit -P client-death-tls.pid --tls require --tls-psk=keys.psk \ + -U $sock memory 2M + +pid=`cat client-death-tls.pid` + +# We can't use 'nbdsh -u "$uri" because of nbd_set_uri_allow_local_file. +# Run a client that abandons several in-flight requests, each large enough +# that we should see EPIPE on one handler while other handlers are still +# waiting to send their response. +nbdsh -c ' +h.set_tls(nbd.TLS_REQUIRE) +h.set_tls_psk_file("keys.psk") +h.set_tls_username("qemu") +h.connect_unix("'"$sock"'") + +buf = nbd.Buffer(2*1024*1024) +c1 = h.aio_pread(buf, 0) +c2 = h.aio_pread(buf, 0) +c3 = h.aio_pread(buf, 0) +c4 = h.aio_pread(buf, 0) +' + +# nbdkit should still be running +sleep 1 +kill -s 0 $pid diff --git a/tests/test-client-death.sh b/tests/test-client-death.sh new file mode 100755 index 00000000..d7f57ca9 --- /dev/null +++ b/tests/test-client-death.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2023 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -x + +requires_nbdsh_uri + +sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX) +files="client-death.pid $sock" +cleanup_fn rm -f $files + +# Start long-running nbdkit +start_nbdkit -P client-death.pid -U $sock memory 2M + +pid=`cat client-death.pid` + +# Run a client that abandons several in-flight requests, each large enough +# that we should see EPIPE on one handler while other handlers are still +# waiting to send their response. +nbdsh -u "nbd+unix:///?socket=$sock" -c ' +buf = nbd.Buffer(2*1024*1024) +c1 = h.aio_pread(buf, 0) +c2 = h.aio_pread(buf, 0) +c3 = h.aio_pread(buf, 0) +c4 = h.aio_pread(buf, 0) +' + +# nbdkit should still be running +sleep 1 +kill -s 0 $pid -- 2.39.2

2 2

[PATCH nbdkit] curl: Try to share as much as possible between handles in the pool
by Richard W.M. Jones 26 Feb '23

26 Feb '23

I'm mainly posting this to the list as a back-up. It does work, it does _not_ improve performance in any noticable way. However I'm having lots of trouble getting HTTP/2 to work (with or without this patch) and that's stopping me from testing anything properly. Rich.

2 6

[libnbd PATCH v4 0/5] vector: introduce DEFINE_POINTER_VECTOR_TYPE()
by Laszlo Ersek 26 Feb '23

26 Feb '23

This reworks the single [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY http://mid.mail-archive.com/20230215141158.2426855-6-lersek@redhat.com according to the review comments I received from Eric and Rich in that subthread. Clearly, the more closely I look at anything, the more refactoring opportunities emerge. :/ At least, although I didn't like the amount of work needed, I quite like the results. :) This series too is bisectable. Laszlo Laszlo Ersek (5): common/include: add TYPE_IS_POINTER() macro common/include: extract STATIC_ASSERT() macro remove semicolon after DEFINE_VECTOR_TYPE() macro invocations vector: introduce DEFINE_POINTER_VECTOR_TYPE() convert string_vector_(iter(free) + reset()) to string_vector_empty() common/include/Makefile.am | 1 + common/include/checked-overflow.h | 8 ++-- common/include/compiler-macros.h | 31 ++++++++++++- common/include/static-assert.h | 48 ++++++++++++++++++++ common/include/test-array-size.c | 45 ++++++------------ common/utils/const-string-vector.h | 2 +- common/utils/nbdkit-string.h | 2 +- common/utils/string-vector.h | 2 +- common/utils/test-vector.c | 7 ++- common/utils/vector.h | 31 +++++++++++++ copy/nbdcopy.h | 2 +- generator/states-connect-socket-activation.c | 9 ++-- info/show.c | 3 +- lib/handle.c | 12 ++--- lib/internal.h | 2 +- lib/utils.c | 6 +-- 16 files changed, 145 insertions(+), 66 deletions(-) create mode 100644 common/include/static-assert.h base-commit: da8887fd4d7ddc1f9cf2f4a3bc20fd51578fa565

2 9

libnbd | Failed pipeline for master | 3a49373a
by GitLab 25 Feb '23

25 Feb '23

Pipeline #788829014 has failed! Project: libnbd ( https://gitlab.com/nbdkit/libnbd ) Branch: master ( https://gitlab.com/nbdkit/libnbd/-/commits/master ) Commit: 3a49373a ( https://gitlab.com/nbdkit/libnbd/-/commit/3a49373a78a611af7497f76b797c78d61… ) Commit Message: python: Add a contextlib function This functio... Commit Author: Richard W_M_ Jones ( https://gitlab.com/rwmjones ) Pipeline #788829014 ( https://gitlab.com/nbdkit/libnbd/-/pipelines/788829014 ) triggered by Richard W_M_ Jones ( https://gitlab.com/rwmjones ) had 1 failed job. Job #3834126161 ( https://gitlab.com/nbdkit/libnbd/-/jobs/3834126161/raw ) Stage: builds Name: x86_64-almalinux-8-clang-prebuilt-env -- You're receiving this email because of your account on gitlab.com.

1 0

no way to force-close NBD handle in nbdsh
by Eric Blake 25 Feb '23

25 Feb '23

I'm out of time this weekend, but while trying to write a test for an nbdkit bug fix (a nasty assertion failure when the client disconnects uncleanly without NBD_CMD_DISC, which is what nbdcopy does if it gets an EIO read response from the server), I realized it is extremely hard to trigger this using nbdsh, but easy to do in C or the Go bindings. In the Go bindings, we intentionally coded things so that the Go structure knows if its underlying C pointer is live or not; it exposes a way to force early closure, then the bindings return a closed_handle_error() if early closure has happened. We probably need to support something similar in the Python bindings. As a short-term hack, I tried directly calling h.__del__() - this is not a good idea, as our current __del__ implementation is not designed to be called twice, and when the later garbage collection calls it again, we segfault trying to free invalid memory. But less drastic measures, such as: import gc del h h = None gc.collect() were still insufficient to trigger a normal __del__ of h, because I can't figure out how to force the garbage collector to see that I want to close the handle but keep python open. In the end, I did get a working test written (by just quit()ing python instead of trying to keep it alive). See my next email for the nbdkit patch that spawned my request here. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

2 3