On Fri, Sep 11, 2020 at 04:49:55PM -0500, Eric Blake wrote:
The next strict knob: allow the user to pass unknown flags across the wire (this is different than passing a known flag at the wrong time).
It is interesting to note that NBD only permits 16 bits of flags, but we have a signature that takes uint32_t; if we wanted, we could pack libnbd-specific flags in the upper bits that the NBD protocol would never see.
This is fine ACK. About the "guard" change: I probably would have put that change in a separate commit. Also you could consider having a default_flags parameter to avoid having to set guard = None everywhere, although you would still have to make a one-off change to every *_flags. It would look like this: let default_flags = { flag_prefix = ""; guard = None; flags = [] } ... let handshake_flags = { default_flags with flag_prefix = "HANDSHAKE_FLAG"; flags = [ ... ] } (We already do this with "default_call"). Neither of these is a big deal though. Rich.
generator/API.ml | 46 ++++++++++++++++++++++++++++++++++++++-------- generator/API.mli | 1 + generator/C.ml | 7 +++++-- lib/disconnect.c | 2 +- lib/rw.c | 6 +++--- tests/errors.c | 22 ++++++++++++++++++++-- 6 files changed, 68 insertions(+), 16 deletions(-)
diff --git a/generator/API.ml b/generator/API.ml index aa970e6..4cd425b 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -87,6 +87,7 @@ and enum = { } and flags = { flag_prefix : string; + guard : string option; flags : (string * int) list } and permitted_state = @@ -165,6 +166,7 @@ let all_enums = [ tls_enum; block_size_enum ] (* Flags. See also Constants below. *) let cmd_flags = { flag_prefix = "CMD_FLAG"; + guard = Some "((h->strict & LIBNBD_STRICT_FLAGS) || flags > UINT16_MAX)"; flags = [ "FUA", 1 lsl 0; "NO_HOLE", 1 lsl 1; @@ -175,6 +177,7 @@ let cmd_flags = { } let handshake_flags = { flag_prefix = "HANDSHAKE_FLAG"; + guard = None; flags = [ "FIXED_NEWSTYLE", 1 lsl 0; "NO_ZEROES", 1 lsl 1; @@ -182,12 +185,15 @@ let handshake_flags = { } let strict_flags = { flag_prefix = "STRICT"; + guard = None; flags = [ "COMMANDS", 1 lsl 0; + "FLAGS", 1 lsl 1; ] } let allow_transport_flags = { flag_prefix = "ALLOW_TRANSPORT"; + guard = None; flags = [ "TCP", 1 lsl 0; "UNIX", 1 lsl 1; @@ -196,6 +202,7 @@ let allow_transport_flags = { } let shutdown_flags = { flag_prefix = "SHUTDOWN"; + guard = None; flags = [ "IMMEDIATE", 1 lsl 1; ] @@ -749,6 +756,22 @@ a read-only server, or attempting to use C<LIBNBD_CMD_FLAG_FUA> when L<nbd_can_fua(3)> returned false). If clear, this flag relies on the server to reject unexpected commands.
+=item C<LIBNBD_STRICT_FLAGS> = 2 + +If set, this flag rejects client requests that attempt to set a +command flag not recognized by libnbd (those outside of +C<LIBNBD_CMD_FLAG_MASK>). If clear, this flag passes on unknown +flags to the server. One possible reason to relax this strictness +knob is to send C<LIBNBD_CMD_FLAG_FUA> on a read command (libnbd +normally prevents that, but the NBD protocol allows it to succeed); +however, it is also possible that sending an unknown flag may +cause the server to change its reply in a manner that confuses +libnbd, perhaps causing deadlock or ending the connection. + +Note that the NBD protocol only supports 16 bits of command flags, +even though the libnbd API uses C<uint32_t>; bits outside of the +range permitted by the protocol are always a client-side error. + =back
For convenience, the constant C<LIBNBD_STRICT_MASK> is available to @@ -1568,9 +1591,10 @@ required into the server's replies, or if you want to use C<LIBNBD_CMD_FLAG_DF>.
The C<flags> parameter must be C<0> for now (it exists for future NBD -protocol extensions)."; +protocol extensions)." +^ strict_call_description; see_also = [Link "aio_pread"; Link "pread_structured"; - Link "get_block_size"]; + Link "get_block_size"; Link "set_strict_mode"]; example = Some "examples/fetch-first-sector.c"; };
@@ -1646,9 +1670,11 @@ The C<flags> parameter may be C<0> for no flags, or may contain C<LIBNBD_CMD_FLAG_DF> meaning that the server should not reply with more than one fragment (if that is supported - some servers cannot do this, see L<nbd_can_df(3)>). Libnbd does not validate that the server -actually obeys the flag."; +actually obeys the flag." +^ strict_call_description; see_also = [Link "can_df"; Link "pread"; - Link "aio_pread_structured"; Link "get_block_size"]; + Link "aio_pread_structured"; Link "get_block_size"; + Link "set_strict_mode"]; };
"pwrite", { @@ -2123,10 +2149,12 @@ Or supply the optional C<completion_callback> which will be invoked as described in L<libnbd(3)/Completion callbacks>.
Note that you must ensure C<buf> is valid until the command has -completed. Other parameters behave as documented in L<nbd_pread(3)>."; +completed. Other parameters behave as documented in L<nbd_pread(3)>." +^ strict_call_description; example = Some "examples/aio-connect-read.c"; see_also = [SectionLink "Issuing asynchronous commands"; - Link "aio_pread_structured"; Link "pread"]; + Link "aio_pread_structured"; Link "pread"; + Link "set_strict_mode"]; };
"aio_pread_structured", { @@ -2145,9 +2173,11 @@ To check if the command completed, call L<nbd_aio_command_completed(3)>. Or supply the optional C<completion_callback> which will be invoked as described in L<libnbd(3)/Completion callbacks>.
-Other parameters behave as documented in L<nbd_pread_structured(3)>."; +Other parameters behave as documented in L<nbd_pread_structured(3)>." +^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; - Link "aio_pread"; Link "pread_structured"]; + Link "aio_pread"; Link "pread_structured"; + Link "set_strict_mode"]; };
"aio_pwrite", { diff --git a/generator/API.mli b/generator/API.mli index db978ca..41e09f5 100644 --- a/generator/API.mli +++ b/generator/API.mli @@ -100,6 +100,7 @@ and enum = { } and flags = { flag_prefix : string; (** prefix of each flag name *) + guard : string option; (** additional gating for checking valid flags *) flags : (string * int) list (** flag names and their values in C *) } and permitted_state = diff --git a/generator/C.ml b/generator/C.ml index 86d9c5c..5f68b14 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -494,7 +494,7 @@ let generate_lib_api_c () = );
(* Check parameters are valid. *) - let print_flags_check n { flag_prefix; flags } subset = + let print_flags_check n { flag_prefix; flags; guard } subset = let value = match errcode with | Some value -> value | None -> assert false in @@ -508,7 +508,10 @@ let generate_lib_api_c () = ) flags; sprintf "0x%x" !v | None -> "LIBNBD_" ^ flag_prefix ^ "_MASK" in - pr " if (unlikely ((%s & ~%s) != 0)) {\n" n mask; + let guard = match guard with + | Some value -> " && " ^ value + | None -> "" in + pr " if (unlikely ((%s & ~%s) != 0)%s) {\n" n mask guard; pr " set_error (EINVAL, \"%%s: invalid value for flag: 0x%%x\",\n"; pr " \"%s\", %s);\n" n n; pr " ret = %s;\n" value; diff --git a/lib/disconnect.c b/lib/disconnect.c index 9de1e34..f99fbd0 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -64,7 +64,7 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id;
- id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); + id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, NULL, NULL); if (id == -1) return -1; h->disconnect_request = true; diff --git a/lib/rw.c b/lib/rw.c index f49fe25..e3e80ad 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -281,7 +281,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, struct command_cb cb = { .completion = *completion };
SET_CALLBACK_TO_NULL (*completion); - return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count, + return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count, buf, &cb); }
@@ -350,7 +350,7 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, }
SET_CALLBACK_TO_NULL (*completion); - return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0, + return nbd_internal_command_common (h, flags, NBD_CMD_FLUSH, 0, 0, NULL, &cb); }
@@ -409,7 +409,7 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, }
SET_CALLBACK_TO_NULL (*completion); - return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count, + return nbd_internal_command_common (h, flags, NBD_CMD_CACHE, offset, count, NULL, &cb); }
diff --git a/tests/errors.c b/tests/errors.c index 0c4151a..0cfcac5 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -84,7 +84,7 @@ main (int argc, char *argv[]) * which delays responding to writes until a witness file no longer * exists. */ - const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent", "sh", + const char *cmd[] = { "nbdkit", "-s", "-v", "--exit-with-parent", "sh", script, NULL }; uint32_t strict;
@@ -244,7 +244,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_FLAGS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_pread (nbd, buf, 1, 0, LIBNBD_CMD_FLAG_MASK + 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_FLAGS | 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, buf, 1, 0, LIBNBD_CMD_FLAG_MASK + 1) != -1) { fprintf (stderr, "%s: test failed: " "nbd_pread did not fail with bogus flags\n", -- 2.28.0
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW