[Libguestfs] [libnbd PATCH v2 4/5] api: Add STRICT_FLAGS to set_strict_mode

Eric Blake
2020-09-11T21:49:55Z

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.
---
 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