Libguestfs - Sep 2020 - Re: [libnbd PATCH v2 4/5] api: Add STRICT_FLAGS to set_strict_mode

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

On 9/17/20 8:32 AM, Richard W.M. Jones wrote:
> 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.
Splitting the patch makes sense.  Also, I _did_ consider the 
'default_flags with' solution before your reply, but only after I had 
written the email.  In the short term, it is no difference in the amount 
of churn (adding one line to every flags definition); but in the long 
term, it is nicer if we ever add more items.  So from that perspective, 
I'll go with that change.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, Sep 17, 2020 at 08:38:02AM -0500, Eric Blake
wrote:
> On 9/17/20 8:32 AM, Richard W.M. Jones wrote:
> >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.
> 
> Splitting the patch makes sense.  Also, I _did_ consider the
> 'default_flags with' solution before your reply, but only after I
> had written the email.  In the short term, it is no difference in
> the amount of churn (adding one line to every flags definition); but
> in the long term, it is nicer if we ever add more items.  So from
> that perspective, I'll go with that change.
Also to be aware of, OCaml has an annoying warning:

$ rlwrap ocaml
# type t = { foo : int; bar : int };;
type t = { foo : int; bar : int; }
# let default_t = { foo = 1; bar = 2 } ;;
val default_t : t = {foo = 1; bar = 2}
# { default_t with foo = 3 } ;;
- : t = {foo = 3; bar = 2}
# { default_t with foo = 3; bar = 4 } ;;
Warning 23: all the fields are explicitly listed in this record:
the 'with' clause is useless.
- : t = {foo = 3; bar = 4}

It's possible to disable the warning if you can assume a sufficiently
new OCaml, but in libguestfs I added a dummy field to struct instead:

https://github.com/libguestfs/libguestfs/blob/fce82fe55a2b64a1a7e494858aa272d608e5e54e/generator/structs.ml#L31

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