When nbd_set_opt_mode() was introduced, we did not adjust the behavior of nbd_shutdown(), so as a result it fails if attempted after the initial connection but before nbd_opt_abort() or nbd_opt_go(). To avoid spurious error messages, at least nbdinfo has to perform special-casing on whether it is in opt mode (call nbd_opt_abort) or connected (call nbd_shutdown) prior to calling nbd_close. It would be nicer to just have a single API perform whatever is needed to gracefully shut the connection, without having to hard-code the special casing into each application. The approach taken here is to add a new flag; if the flag is absent (behavior of an older application), then nbd_opt_abort must still be invoked separately before attempting nbd_shutdown. But if the flag is present, then whether or not the server supports opt mode, the shutdown will be graceful. This patch updates the unit test to show the effect of the new flag, and the next commit will simplify nbdinfo by utilizing it. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- generator/API.ml | 21 ++++++++++++++++----- lib/disconnect.c | 15 +++++++++++++++ tests/shutdown-opt-mode.c | 27 ++++++++++++++++++++++++++- 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/generator/API.ml b/generator/API.ml index 1a9c8141..0b837259 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -255,6 +255,7 @@ let shutdown_flags = flag_prefix = "SHUTDOWN"; flags = [ "ABANDON_PENDING", 1 lsl 16; + "COVER_OPT_MODE", 1 lsl 17; ] } let all_flags = [ cmd_flags; handshake_flags; strict_flags; @@ -1231,7 +1232,8 @@ "set_opt_mode", { starting the connection. To leave the mode and proceed on to the ready state, you must use L<nbd_opt_go(3)> successfully; a failed L<nbd_opt_go(3)> returns to the negotiating state to allow a change of -export name before trying again. You may also use L<nbd_opt_abort(3)> +export name before trying again. You may also use L<nbd_opt_abort(3)>, +or the C<LIBNBD_SHUTDOWN_COVER_OPT_MODE> flag of L<nbd_shutdown(3)>, to end the connection without finishing negotiation."; example = Some "examples/list-exports.c"; see_also = [Link "get_opt_mode"; Link "aio_is_negotiating"; @@ -1240,7 +1242,7 @@ "set_opt_mode", { Link "opt_set_meta_context"; Link "opt_starttls"; Link "opt_structured_reply"; Link "set_tls"; Link "set_request_structured_replies"; - Link "aio_connect"]; + Link "aio_connect"; Link "shutdown"]; }; "get_opt_mode", { @@ -2667,7 +2669,7 @@ "shutdown", { default_call with args = []; optargs = [ OFlags ("flags", shutdown_flags, None) ]; ret = RErr; - permitted_states = [ Connected ]; + permitted_states = [ Negotiating; Connected ]; modifies_fd = true; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -2678,7 +2680,7 @@ "shutdown", { This function works whether or not the handle is ready for transmission of commands. If more fine-grained control is -needed, see L<nbd_aio_disconnect(3)>. +needed, see L<nbd_aio_opt_abort(3)> and L<nbd_aio_disconnect(3)>. The C<flags> argument is a bitmask, including zero or more of the following shutdown flags: @@ -2693,12 +2695,21 @@ "shutdown", { issuing those commands before informing the server of the intent to disconnect. +=item C<LIBNBD_SHUTDOWN_COVER_OPT_MODE> = 0x20000 + +If the server is still in option negotiation mode +(see L<nbd_set_opt_mode(3)>), gracefully abandon the +connection as if by L<nbd_opt_abort(3)>, instead of +complaining that the server is not yet fully connected. +If option negotiation mode was not in use or was +completed by L<nbd_opt_go(3)>, this flag has no effect. + =back For convenience, the constant C<LIBNBD_SHUTDOWN_MASK> is available to describe all shutdown flags recognized by this build of libnbd. A future version of the library may add new flags."; - see_also = [Link "close"; Link "aio_disconnect"]; + see_also = [Link "close"; Link "aio_disconnect"; Link "aio_opt_abort"]; example = Some "examples/reads-and-writes.c"; }; diff --git a/lib/disconnect.c b/lib/disconnect.c index 083d6cd3..d250e739 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -30,6 +30,20 @@ int nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags) { + /* If still negotiating, return an error unless COVER_OPT_ABORT lets + * us trigger opt_abort instead. + */ + if (nbd_internal_is_state_negotiating (get_next_state (h))) { + if (flags & LIBNBD_SHUTDOWN_COVER_OPT_MODE) { + if (nbd_unlocked_aio_opt_abort (h) == -1) + return -1; + goto wait; + } + set_error (EINVAL, "invalid state: COVER_OPT_ABORT not requested, but " + "the handle is still negotiating options with the server"); + return -1; + } + /* If ABANDON_PENDING, abort any commands that have not yet had any * bytes sent to the server, so NBD_CMD_DISC becomes next in line. */ @@ -50,6 +64,7 @@ nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags) return -1; } + wait: while (!nbd_internal_is_state_closed (get_next_state (h)) && !nbd_internal_is_state_dead (get_next_state (h))) { if (nbd_unlocked_poll (h, -1) == -1) diff --git a/tests/shutdown-opt-mode.c b/tests/shutdown-opt-mode.c index 34386220..96547607 100644 --- a/tests/shutdown-opt-mode.c +++ b/tests/shutdown-opt-mode.c @@ -119,6 +119,31 @@ main (int argc, char *argv[]) } nbd_close (nbd); - /* Part 3: TODO: add flag to nbd_shutdown */ + /* Part 3: Use of flag avoids the need to manually do opt_abort. */ + nbd = nbd_create (); + if (nbd == NULL) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_set_opt_mode (nbd, true) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_connect_command (nbd, (char **)cmd_new) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_aio_is_negotiating (nbd) != 1) { + fprintf (stderr, "%s: unexpected state\n", progname); + exit (EXIT_FAILURE); + } + + /* Shutdown succeeds, because of the flag. */ + if (nbd_shutdown (nbd, LIBNBD_SHUTDOWN_COVER_OPT_MODE) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + nbd_close (nbd); + exit (EXIT_SUCCESS); } -- 2.41.0

Now that libnbd supports extended headers, it is worth observing that the qemu implementation for NBD_CMD_BLOCK_STATUS that supports an extended header for filtering the server's response, as advertised by NBD_FLAG_BLOCK_STAT_PAYLOAD, is conditionally advertised. When using NBD_OPT_GO, the flag is only advertised if the client requested more than one meta context (as otherwise, there is no use filtering). But for NBD_OPT_INFO, the flag is advertised if extended headers are enabled, regardless of whether meta contexts are negotiated yet. In order for upcoming libnbd patches to add support for probing and using this bit reliably, we therefore need 'nbdinfo --can ...' to favor the information learned by NBD_OPT_INFO instead of NBD_OPT_GO, because that is lighter weight than figuring out whether an export supports at least two meta contexts that can then be negotiated. Do this by changing nbdinfo to prefer opt mode always, then touching up the few places (--map, --content) that need to ensure NBD_OPT_GO. This is made easier by the previous patches that make nbd_shutdown() work sanely regardless of whether we are still in opt mode. In turn, this patch flushed out a double-free bug in 'nbdkit file dir=...' when querying opt_info on the default name, so a couple of unit tests need to avoid false negatives on platforms where nbdkit commit 39d62de9 is not yet backported. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- info/info-list-uris.sh | 9 ++++++++- info/info-uri.sh | 9 ++++++++- info/main.c | 14 +++++++++----- info/map.c | 7 +++++++ 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/info/info-list-uris.sh b/info/info-list-uris.sh index 0d6a16a0..d8ea9108 100755 --- a/info/info-list-uris.sh +++ b/info/info-list-uris.sh @@ -22,7 +22,14 @@ set -e set -x requires nbdkit --version -requires nbdkit file --version +# Avoid tickling a double-free bug in nbdkit 1.35.11 +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c " +try: + h.opt_info() +except nbd.Error: + pass +h.opt_abort() +"' # This test requires nbdkit >= 1.22. minor=$( nbdkit --dump-config | grep ^version_minor | cut -d= -f2 ) diff --git a/info/info-uri.sh b/info/info-uri.sh index d491e904..ba0c36d2 100755 --- a/info/info-uri.sh +++ b/info/info-uri.sh @@ -24,7 +24,14 @@ set -e set -x requires nbdkit --version -requires nbdkit file --version +# Avoid tickling a double-free bug in nbdkit 1.35.11 +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c " +try: + h.opt_info() +except nbd.Error: + pass +h.opt_abort() +"' requires nbdkit -U - null --run 'test "$uri" != ""' requires jq --version diff --git a/info/main.c b/info/main.c index 204290a5..80d38224 100644 --- a/info/main.c +++ b/info/main.c @@ -138,7 +138,6 @@ main (int argc, char *argv[]) size_t output_len = 0; bool content_flag = false, no_content_flag = false; bool list_okay = true; - bool opt_mode = false; progname = argv[0]; colour = isatty (STDOUT_FILENO); @@ -281,11 +280,9 @@ main (int argc, char *argv[]) nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */ /* Set optional modes in the handle. */ - opt_mode = !can && !map && !size_only; - if (opt_mode) { - nbd_set_opt_mode (nbd, true); + nbd_set_opt_mode (nbd, true); + if (!can && !map && !size_only) nbd_set_full_info (nbd, true); - } if (map) nbd_add_meta_context (nbd, map); @@ -398,6 +395,13 @@ do_connect (struct nbd_handle *nbd) fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); exit (EXIT_FAILURE); } + + /* If we are in opt mode, request info on the original export name. + * However, ignoring failure at this time is okay, as later code + * may want to try an alternate export name. + */ + if (nbd_aio_is_negotiating (nbd)) + nbd_opt_info (nbd); } /* The URI field in output is not meaningful unless there's a diff --git a/info/map.c b/info/map.c index 594f24ff..399e0355 100644 --- a/info/map.c +++ b/info/map.c @@ -54,6 +54,13 @@ do_map (void) uint64_t offset, align, max_len; size_t prev_entries_size; + /* Map mode requires switching over to transmission phase. */ + if (nbd_aio_is_negotiating (nbd) && + nbd_opt_go (nbd) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + /* Did we get the requested map? */ if (!nbd_can_meta_context (nbd, map)) { fprintf (stderr, -- 2.41.0

On Tue, Sep 05, 2023 at 03:23:58PM +0100, Richard W.M. Jones wrote: Hmm. There is indeed a range of affected versions, but it is not as bad as you make out. The bug was introduced in 185e7d (v1.33.1) and was not backported to stable-1.32; so if we assume distros only use stable versions, then rejecting 1.34.[012] is sufficient. I've already backported the fix to stable-1.34, but don't know if you would prefer to cut the 1.34.3 release or let me do it. And as you pointed out in the next message, triggering the double-free can still have the side effect of a core file. You've convinced me that a version check is the best here. We now unconditionally request it, but it is up to the server whether we actually got it. An oldstyle server does not support opt mode. And our unit tests do have coverage of connecting to an oldstyle server. At any rate, with the revisions to patch 2 and 3 (nbd_shutdown now automatically covers opt_abort without needing a new flag), and with this patch updated to just do a version range exclusion, this series is now in as db9f6bf6..0b587f8a -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org

On Tue, Sep 05, 2023 at 08:47:34AM -0500, Eric Blake wrote: Actually I think a problem with this is that it always causes a segfault, which could leave a core dump around (does on my machine). It's unfortunate, because obviously you can't test for a double-free in nbdkit without causing a double-free. It might be a rare case when a version test is better. 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