[Libguestfs] [libnbd PATCH 1/2] api: Add nbd_set_strict_mode

Right now, libnbd has refused to issue a command not advertised as supported by a server, including unknown flags, mainly because the NBD protocol does not guarantee what the server will do, and libnbd would rather stay in sync with the server than drop the connection. However, for integration purposes, it can be handy to coerce libnbd into sending something to see how the server will react (whether it be an extension libnbd has not yet learned, or an intentional bad request to test server error handling). Time to make this something the user can control, by adding a new strictness mode. A later patch will add another knob to the mode. --- Question: Should we split this into two knobs? Sending unknown flags is dangerous (in the future, a flag might be defined to alter the on-wire layout expected by either the client or server, and get things out-of-sync or even cause data corruption), while sending known commands contrary to advertised flags is less risky (so far, NBD_CMD_WRITE is the only command that alters what the client sends, so a server can easily reject unknown commands by assuming they match the layout of NBD_CMD_READ; and I just fixed a long-standing bug where we were sending trim and zero requests contrary to server advertisement). Trying to provoke a write to a read-only server is different than trying to experiment with an unknown flag, which is why it might make sense to have two separate knobs for the gating done in this patch. --- lib/internal.h | 3 + generator/API.ml | 60 +++++++++++++- lib/disconnect.c | 18 ++-- lib/handle.c | 16 ++++ lib/rw.c | 208 +++++++++++++++++++++++++---------------------- tests/errors.c | 41 +++++++++- 6 files changed, 239 insertions(+), 107 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index b2637bd..788a62e 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -148,6 +148,9 @@ struct nbd_handle { bool debug; nbd_debug_callback debug_callback; + /* How strict to be. */ + unsigned strict; + /* State machine. * * The actual current state is ‘state’. ‘public_state’ is updated diff --git a/generator/API.ml b/generator/API.ml index 962b787..8811d3c 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -169,7 +169,13 @@ let handshake_flags = { flags = [ "FIXED_NEWSTYLE", 1 lsl 0; "NO_ZEROES", 1 lsl 1; - ] + ] +} +let strict_flags = { + flag_prefix = "STRICT"; + flags = [ + "COMMANDS", 1 lsl 0; + ] } let allow_transport_flags = { flag_prefix = "ALLOW_TRANSPORT"; @@ -179,7 +185,8 @@ let allow_transport_flags = { "VSOCK", 1 lsl 2; ] } -let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags ] +let all_flags = [ cmd_flags; handshake_flags; strict_flags; + allow_transport_flags ] let default_call = { args = []; optargs = []; ret = RErr; shortdesc = ""; longdesc = ""; example = None; @@ -696,6 +703,51 @@ the time of compilation."; Link "aio_is_created"; Link "aio_is_ready"]; }; + "set_strict_mode", { + default_call with + args = [ Flags ("flags", strict_flags) ]; ret = RErr; + shortdesc = "control how strictly to follow NBD protocol"; + longdesc = "\ +By default, libnbd tries to detect requests that would trigger +undefined behavior in the NBD protocol, and rejects them client +side without causing any network traffic rather than risking +undefined server behavior. However, for integration testing, it +can be handy to coerce libnbd into sending such requests in +order to test the robustness of the server in dealing with such +traffic. + +The C<flags> argument is a bitmask, including zero or more of the +following strictness flags: + +=over 4 + +=item C<LIBNBD_STRICT_COMMANDS> = 1 + +If set, this flag rejects client requests that do not comply with the +set of advertised server flags (for example, attempting a write on +a read-only server). If clear, this flag relies on the server to +reject unexpected commands or unknown flags to supported commands. + +=back + +Future versions of libnbd may add further flags. +"; + see_also = [Link "get_strict_mode"]; + }; + + "get_strict_mode", { + default_call with + args = []; ret = RUInt; + may_set_error = false; + shortdesc = "see which strictness flags are in effect"; + longdesc = "\ +Return flags indicating which protocol strictness items are being +enforced locally by libnbd rather than the server. The return value +from a newer library version may include bits that were undefined at +the time of compilation."; + see_also = [Link "set_strict_mode"]; + }; + "set_opt_mode", { default_call with args = [Bool "enable"]; ret = RErr; @@ -2615,6 +2667,10 @@ let first_version = [ "aio_opt_list", (1, 4); "aio_opt_info", (1, 4); + (* Added in 1.5.x development cycle, will be stable and supported in 1.6. *) + "set_strict_mode", (1, 6); + "get_strict_mode", (1, 6); + (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. "get_tls_certificates", (1, ??); diff --git a/lib/disconnect.c b/lib/disconnect.c index dcb95d8..22cd5d7 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -29,9 +29,11 @@ int nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags) { - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (flags != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } } if (!h->disconnect_request && @@ -55,9 +57,11 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id; - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (flags != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } } id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); diff --git a/lib/handle.c b/lib/handle.c index 7987d59..a1fa142 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -75,6 +75,8 @@ nbd_create (void) s = getenv ("LIBNBD_DEBUG"); h->debug = s && strcmp (s, "1") == 0; + h->strict = LIBNBD_STRICT_COMMANDS; + h->public_state = STATE_START; h->state = STATE_START; h->pid = -1; @@ -350,6 +352,20 @@ nbd_unlocked_get_handshake_flags (struct nbd_handle *h) return h->gflags; } +int +nbd_unlocked_set_strict_mode (struct nbd_handle *h, uint32_t flags) +{ + h->strict = flags; + return 0; +} + +/* NB: may_set_error = false. */ +unsigned +nbd_unlocked_get_strict_mode (struct nbd_handle *h) +{ + return h->strict; +} + const char * nbd_unlocked_get_package_name (struct nbd_handle *h) { diff --git a/lib/rw.c b/lib/rw.c index 8b736d3..b5c1698 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -267,13 +267,15 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, { struct command_cb cb = { .completion = completion }; - /* We could silently accept flag DF, but it really only makes sense - * with callbacks, because otherwise there is no observable change - * except that the server may fail where it would otherwise succeed. - */ - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; + if (h->strict & LIBNBD_STRICT_COMMANDS) { + /* We could silently accept flag DF, but it really only makes sense + * with callbacks, because otherwise there is no observable change + * except that the server may fail where it would otherwise succeed. + */ + if (flags != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } } return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count, @@ -290,15 +292,17 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, struct command_cb cb = { .fn.chunk = chunk, .completion = completion }; - if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_DF) != 0 && - nbd_unlocked_can_df (h) != 1) { - set_error (EINVAL, "server does not support the DF flag"); - return -1; + if ((flags & LIBNBD_CMD_FLAG_DF) != 0 && + nbd_unlocked_can_df (h) != 1) { + set_error (EINVAL, "server does not support the DF flag"); + return -1; + } } return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count, @@ -313,20 +317,22 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, { struct command_cb cb = { .completion = completion }; - if (nbd_unlocked_is_read_only (h) == 1) { - set_error (EPERM, "server does not support write operations"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_is_read_only (h) == 1) { + set_error (EPERM, "server does not support write operations"); + return -1; + } - if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } + if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && - nbd_unlocked_can_fua (h) != 1) { - set_error (EINVAL, "server does not support the FUA flag"); - return -1; + if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && + nbd_unlocked_can_fua (h) != 1) { + set_error (EINVAL, "server does not support the FUA flag"); + return -1; + } } return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count, @@ -340,14 +346,16 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, { struct command_cb cb = { .completion = completion }; - if (nbd_unlocked_can_flush (h) != 1) { - set_error (EINVAL, "server does not support flush operations"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_can_flush (h) != 1) { + set_error (EINVAL, "server does not support flush operations"); + return -1; + } - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; + if (flags != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } } return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0, @@ -362,24 +370,26 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, { struct command_cb cb = { .completion = completion }; - if (nbd_unlocked_can_trim (h) != 1) { - set_error (EINVAL, "server does not support trim operations"); - return -1; - } - if (nbd_unlocked_is_read_only (h) == 1) { - set_error (EPERM, "server does not support write operations"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_can_trim (h) != 1) { + set_error (EINVAL, "server does not support trim operations"); + return -1; + } + if (nbd_unlocked_is_read_only (h) == 1) { + set_error (EPERM, "server does not support write operations"); + return -1; + } - if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } + if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && - nbd_unlocked_can_fua (h) != 1) { - set_error (EINVAL, "server does not support the FUA flag"); - return -1; + if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && + nbd_unlocked_can_fua (h) != 1) { + set_error (EINVAL, "server does not support the FUA flag"); + return -1; + } } if (count == 0) { /* NBD protocol forbids this. */ @@ -399,18 +409,20 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, { struct command_cb cb = { .completion = completion }; - /* Actually according to the NBD protocol document, servers do exist - * that support NBD_CMD_CACHE but don't advertise the - * NBD_FLAG_SEND_CACHE bit, but we ignore those. - */ - if (nbd_unlocked_can_cache (h) != 1) { - set_error (EINVAL, "server does not support cache operations"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + /* Actually according to the NBD protocol document, servers do exist + * that support NBD_CMD_CACHE but don't advertise the + * NBD_FLAG_SEND_CACHE bit, but we ignore those. + */ + if (nbd_unlocked_can_cache (h) != 1) { + set_error (EINVAL, "server does not support cache operations"); + return -1; + } - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; + if (flags != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } } return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count, @@ -425,31 +437,33 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, { struct command_cb cb = { .completion = completion }; - if (nbd_unlocked_can_zero (h) != 1) { - set_error (EINVAL, "server does not support zero operations"); - return -1; - } - if (nbd_unlocked_is_read_only (h) == 1) { - set_error (EPERM, "server does not support write operations"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_can_zero (h) != 1) { + set_error (EINVAL, "server does not support zero operations"); + return -1; + } + if (nbd_unlocked_is_read_only (h) == 1) { + set_error (EPERM, "server does not support write operations"); + return -1; + } - if ((flags & ~(LIBNBD_CMD_FLAG_FUA | LIBNBD_CMD_FLAG_NO_HOLE | - LIBNBD_CMD_FLAG_FAST_ZERO)) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } + if ((flags & ~(LIBNBD_CMD_FLAG_FUA | LIBNBD_CMD_FLAG_NO_HOLE | + LIBNBD_CMD_FLAG_FAST_ZERO)) != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && - nbd_unlocked_can_fua (h) != 1) { - set_error (EINVAL, "server does not support the FUA flag"); - return -1; - } + if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && + nbd_unlocked_can_fua (h) != 1) { + set_error (EINVAL, "server does not support the FUA flag"); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_FAST_ZERO) != 0 && - nbd_unlocked_can_fast_zero (h) != 1) { - set_error (EINVAL, "server does not support the fast zero flag"); - return -1; + if ((flags & LIBNBD_CMD_FLAG_FAST_ZERO) != 0 && + nbd_unlocked_can_fast_zero (h) != 1) { + set_error (EINVAL, "server does not support the fast zero flag"); + return -1; + } } if (count == 0) { /* NBD protocol forbids this. */ @@ -471,21 +485,23 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, struct command_cb cb = { .fn.extent = extent, .completion = completion }; - if (!h->structured_replies) { - set_error (ENOTSUP, "server does not support structured replies"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (!h->structured_replies) { + set_error (ENOTSUP, "server does not support structured replies"); + return -1; + } - if (h->meta_contexts == NULL) { - set_error (ENOTSUP, "did not negotiate any metadata contexts, " - "either you did not call nbd_add_meta_context before " - "connecting or the server does not support it"); - return -1; - } + if (h->meta_contexts == NULL) { + set_error (ENOTSUP, "did not negotiate any metadata contexts, " + "either you did not call nbd_add_meta_context before " + "connecting or the server does not support it"); + return -1; + } - if ((flags & ~LIBNBD_CMD_FLAG_REQ_ONE) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; + if ((flags & ~LIBNBD_CMD_FLAG_REQ_ONE) != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } } if (count == 0) { /* NBD protocol forbids this. */ diff --git a/tests/errors.c b/tests/errors.c index ef8303e..a8e1b0c 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -86,6 +86,7 @@ main (int argc, char *argv[]) */ const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent", "sh", script, NULL }; + uint32_t strict; progname = argv[0]; @@ -214,7 +215,25 @@ main (int argc, char *argv[]) } check (EINVAL, "nbd_pread: "); - /* Use unknown command flags */ + /* Use unknown command flags, client-side */ + strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_COMMANDS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_pread (nbd, NULL, 0, 0, -1) != -1) { + fprintf (stderr, "%s: test failed: " + "nbd_pread did not fail with bogus flags\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_pread: "); + /* Use unknown command flags, server-side */ + strict &= ~LIBNBD_STRICT_COMMANDS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if (nbd_pread (nbd, NULL, 0, 0, -1) != -1) { fprintf (stderr, "%s: test failed: " "nbd_pread did not fail with bogus flags\n", @@ -240,7 +259,25 @@ main (int argc, char *argv[]) } check (ERANGE, "nbd_aio_pwrite: "); - /* Use unadvertised command */ + /* Use unadvertised command, client-side */ + strict |= LIBNBD_STRICT_COMMANDS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_trim (nbd, 512, 0, 0) != -1) { + fprintf (stderr, "%s: test failed: " + "unpermitted nbd_trim did not fail\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_trim: "); + /* Use unadvertised command, server-side */ + strict &= ~LIBNBD_STRICT_COMMANDS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if (nbd_trim (nbd, 512, 0, 0) != -1) { fprintf (stderr, "%s: test failed: " "unpermitted nbd_trim did not fail\n", -- 2.28.0