Libguestfs - May 2019 - [PATCH libnbd 0/3] Prevent some misuse of multi-conn.

Per recent discussion here:
https://www.redhat.com/archives/libguestfs/2019-May/thread.html#00175

Simple refactoring.
---
 generator/states-newstyle-opt-export-name.c | 12 +++++------
 generator/states-newstyle-opt-go.c          | 13 ++++++------
 generator/states-oldstyle.c                 | 10 +++-------
 lib/flags.c                                 | 22 +++++++++++++++++++++
 lib/internal.h                              |  5 +++++
 5 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/generator/states-newstyle-opt-export-name.c
b/generator/states-newstyle-opt-export-name.c
index 8ff1c1c..07b6c9e 100644
--- a/generator/states-newstyle-opt-export-name.c
+++ b/generator/states-newstyle-opt-export-name.c
@@ -58,13 +58,13 @@
   return 0;
 
  NEWSTYLE.OPT_EXPORT_NAME.CHECK_REPLY:
-  conn->h->exportsize = be64toh
(conn->sbuf.export_name_reply.exportsize);
-  conn->h->eflags = be16toh (conn->sbuf.export_name_reply.eflags);
-  debug (conn->h, "exportsize: %" PRIu64 " eflags: 0x%"
PRIx16,
-         conn->h->exportsize, conn->h->eflags);
-  if (conn->h->eflags == 0) {
+  uint64_t exportsize;
+  uint16_t eflags;
+
+  exportsize = be64toh (conn->sbuf.export_name_reply.exportsize);
+  eflags = be16toh (conn->sbuf.export_name_reply.eflags);
+  if (nbd_internal_set_size_and_flags (conn->h, exportsize, eflags) == -1) {
     SET_NEXT_STATE (%.DEAD);
-    set_error (EINVAL, "handshake: invalid eflags == 0 from server");
     return -1;
   }
   SET_NEXT_STATE (%.READY);
diff --git a/generator/states-newstyle-opt-go.c
b/generator/states-newstyle-opt-go.c
index 4dbeee9..97f25f8 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -106,6 +106,8 @@
   uint32_t reply;
   uint32_t len;
   const size_t maxpayload = sizeof conn->sbuf.or.payload;
+  uint64_t exportsize;
+  uint16_t eflags;
 
   magic = be64toh (conn->sbuf.or.option_reply.magic);
   option = be32toh (conn->sbuf.or.option_reply.option);
@@ -129,14 +131,11 @@
     if (len <= maxpayload /* see RECV_NEWSTYLE_OPT_GO_REPLY */) {
       if (len >= sizeof conn->sbuf.or.payload.export) {
         if (be16toh (conn->sbuf.or.payload.export.info) == NBD_INFO_EXPORT)
{
-          conn->h->exportsize -            be64toh
(conn->sbuf.or.payload.export.exportsize);
-          conn->h->eflags = be16toh
(conn->sbuf.or.payload.export.eflags);
-          debug (conn->h, "exportsize: %" PRIu64 " eflags:
0x%" PRIx16,
-                 conn->h->exportsize, conn->h->eflags);
-          if (conn->h->eflags == 0) {
+          exportsize = be64toh (conn->sbuf.or.payload.export.exportsize);
+          eflags = be16toh (conn->sbuf.or.payload.export.eflags);
+          if (nbd_internal_set_size_and_flags (conn->h,
+                                               exportsize, eflags) == -1) {
             SET_NEXT_STATE (%.DEAD);
-            set_error (EINVAL, "handshake: invalid eflags == 0 from
server");
             return -1;
           }
         }
diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c
index 95b7df9..29cb341 100644
--- a/generator/states-oldstyle.c
+++ b/generator/states-oldstyle.c
@@ -47,14 +47,10 @@
   eflags = be16toh (conn->sbuf.old_handshake.eflags);
 
   conn->gflags = gflags;
-  conn->h->exportsize = exportsize;
-  conn->h->eflags = eflags;
-  debug (conn->h, "exportsize: %" PRIu64 " eflags: 0x%"
PRIx16
-         " gflags: 0x%" PRIx16,
-         exportsize, eflags, gflags);
-  if (eflags == 0) {
+  debug (conn->h, "gflags: 0x%" PRIx16, gflags);
+
+  if (nbd_internal_set_size_and_flags (conn->h, exportsize, eflags) == -1) {
     SET_NEXT_STATE (%.DEAD);
-    set_error (EINVAL, "handshake: invalid eflags == 0 from server");
     return -1;
   }
 
diff --git a/lib/flags.c b/lib/flags.c
index f0d9dc3..825b326 100644
--- a/lib/flags.c
+++ b/lib/flags.c
@@ -21,10 +21,32 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdbool.h>
+#include <stdint.h>
+#include <inttypes.h>
 #include <errno.h>
 
 #include "internal.h"
 
+/* Set the export size and eflags on the handle, validating them.
+ * This is called from the state machine when either the newstyle or
+ * oldstyle negotiation reaches the point that these are available.
+ */
+int
+nbd_internal_set_size_and_flags (struct nbd_handle *h,
+                                 uint64_t exportsize, uint16_t eflags)
+{
+  debug (h, "exportsize: %" PRIu64 " eflags: 0x%" PRIx16,
exportsize, eflags);
+
+  if (eflags == 0) {
+    set_error (EINVAL, "handshake: invalid eflags == 0 from server");
+    return -1;
+  }
+
+  h->exportsize = exportsize;
+  h->eflags = eflags;
+  return 0;
+}
+
 static int
 get_flag (struct nbd_handle *h, uint16_t flag)
 {
diff --git a/lib/internal.h b/lib/internal.h
index 8eadada..f0705ef 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -265,6 +265,11 @@ extern void nbd_internal_set_last_error (int errnum, char
*error);
       nbd_internal_set_last_error ((errnum), _errp);                    \
   } while (0)
 
+/* flags.c */
+extern int nbd_internal_set_size_and_flags (struct nbd_handle *h,
+                                            uint64_t exportsize,
+                                            uint16_t eflags);
+
 /* protocol.c */
 extern int nbd_internal_errno_of_nbd_error (uint32_t error);
 extern const char *nbd_internal_name_of_nbd_cmd (uint16_t type);
-- 
2.21.0

It is unsafe to connect using multi-conn to a server unless the server
advertizes it, so fail if the caller tries to do this.
---
 generator/generator | 16 ++++++++++++++--
 lib/flags.c         | 10 ++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/generator/generator b/generator/generator
index a372074..a8e6fb6 100755
--- a/generator/generator
+++ b/generator/generator
@@ -897,7 +897,8 @@ The C<multi_conn> parameter controls whether this
feature is
 enabled (if E<gt> 1) or disabled (if C<1>).  The parameter
 passed must not be C<0>.  Usually small powers of 2 (eg. 2, 4, 8)
 will provide increments in performance.  Some servers do not
-support this feature and will return an error on connection.
+support this feature and libnbd will fail on connection to
+these servers if this setting is E<gt> 1.
 
 At present, libnbd assumes that all connections will report the
 same capabilities; a server that behaves otherwise may trigger
@@ -1171,7 +1172,18 @@ connected to and completed the handshake with the
server.";
 Returns true if the server supports multi-conn
 (see C<nbd_set_multi_conn>).  Returns false if
 the server does not.  Can return an error if we have not
-connected to and completed the handshake with the server.";
+connected to and completed the handshake with the server.
+
+Note that you I<cannot> set multi-conn on the handle
+to E<gt> 1, connect, and then check this setting, because
+if the server does not support multi-conn the connection
+will fail.  Instead you should connect with multi-conn
+set to C<1> (the default), check this setting, then if
+multi-conn is found to be supported you can call
+C<nbd_set_multi_conn> with a larger value to increase
+the number of connection objects, then call one of the
+C<nbd_connect*> functions again on the handle to connect
+the remaining connections.";
   };
 
   "can_cache", {
diff --git a/lib/flags.c b/lib/flags.c
index 825b326..1956db6 100644
--- a/lib/flags.c
+++ b/lib/flags.c
@@ -42,6 +42,16 @@ nbd_internal_set_size_and_flags (struct nbd_handle *h,
     return -1;
   }
 
+  /* It is unsafe to connect to a server with multi-conn set unless
+   * the server says it is safe to do so.
+   */
+  if (h->multi_conn > 1 && (eflags & NBD_FLAG_CAN_MULTI_CONN)
== 0) {
+    set_error (EINVAL, "handshake: multi-conn is set on this handle,
"
+               "but the server does not advertize multi-conn support
"
+               "so disconnecting because it is not safe to
continue");
+    return -1;
+  }
+
   h->exportsize = exportsize;
   h->eflags = eflags;
   return 0;
-- 
2.21.0

However we don't modify the asynchronous call, continuing the general
policy of allowing users to do what they want with AIO even if it is
unwise.
---
 lib/connect.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/connect.c b/lib/connect.c
index c5970d2..61d945f 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -150,12 +150,15 @@ nbd_unlocked_connect_tcp (struct nbd_handle *h,
   return wait_all_connected (h);
 }
 
-/* Connect to a local command.  Multi-conn doesn't make much sense
- * here, should it be an error?
- */
+/* Connect to a local command. */
 int
 nbd_unlocked_connect_command (struct nbd_handle *h, char **argv)
 {
+  if (h->multi_conn > 1) {
+    set_error (EINVAL, "multi-conn cannot be used when connecting to a
command");
+    return -1;
+  }
+
   if (!nbd_unlocked_aio_is_created (h->conns[0])) {
     set_error (0, "first connection in this handle is not in the created
state, this is likely to be caused by a programming error in the calling
program");
     return -1;
-- 
2.21.0

On 5/23/19 4:32 AM, Richard W.M. Jones wrote:
> It is unsafe to connect using multi-conn to a server unless the server
> advertizes it, so fail if the caller tries to do this.
Actually, I think we can be a little bit looser:

It is unsafe to connect with write access using multi-conn.

But for read-only (where we can't corrupt data), having multiple
connections even when the server does not advertise multi-conn should
(in general) still be safe.
> ---
>  generator/generator | 16 ++++++++++++++--
>  lib/flags.c         | 10 ++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
> index a372074..a8e6fb6 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -897,7 +897,8 @@ The C<multi_conn> parameter controls whether this
feature is
>  enabled (if E<gt> 1) or disabled (if C<1>).  The parameter
>  passed must not be C<0>.  Usually small powers of 2 (eg. 2, 4, 8)
>  will provide increments in performance.  Some servers do not
> -support this feature and will return an error on connection.
> +support this feature and libnbd will fail on connection to
> +these servers if this setting is E<gt> 1.
So maybe change this to: ...libnbd will fail on read-write connections
to these servers if...
>  
>  At present, libnbd assumes that all connections will report the
>  same capabilities; a server that behaves otherwise may trigger
> @@ -1171,7 +1172,18 @@ connected to and completed the handshake with the
server.";
>  Returns true if the server supports multi-conn
>  (see C<nbd_set_multi_conn>).  Returns false if
>  the server does not.  Can return an error if we have not
> -connected to and completed the handshake with the server.";
> +connected to and completed the handshake with the server.
> +
> +Note that you I<cannot> set multi-conn on the handle
> +to E<gt> 1, connect, and then check this setting, because
> +if the server does not support multi-conn the connection
> +will fail.  Instead you should connect with multi-conn
> +set to C<1> (the default), check this setting, then if
> +multi-conn is found to be supported you can call
> +C<nbd_set_multi_conn> with a larger value to increase
> +the number of connection objects, then call one of the
> +C<nbd_connect*> functions again on the handle to connect
> +the remaining connections.";
This one is good.
>    };
>  
>    "can_cache", {
> diff --git a/lib/flags.c b/lib/flags.c
> index 825b326..1956db6 100644
> --- a/lib/flags.c
> +++ b/lib/flags.c
> @@ -42,6 +42,16 @@ nbd_internal_set_size_and_flags (struct nbd_handle *h,
>      return -1;
>    }
>  
> +  /* It is unsafe to connect to a server with multi-conn set unless
> +   * the server says it is safe to do so.
> +   */
> +  if (h->multi_conn > 1 && (eflags &
NBD_FLAG_CAN_MULTI_CONN) == 0) {
Here, I'd add && !(eflags & NBD_FLAG_READ_ONLY)
> +    set_error (EINVAL, "handshake: multi-conn is set on this handle,
"
> +               "but the server does not advertize multi-conn support
"
I know that in general -ise vs. -ize is a common US/UK thing, but
https://english.stackexchange.com/questions/341707/should-advertised-be-spelled-with-a-z-in-american-english
says that advertise is preferred in both.

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

On 5/23/19 4:32 AM, Richard W.M. Jones wrote:
> Simple refactoring.
> ---
>  generator/states-newstyle-opt-export-name.c | 12 +++++------
>  generator/states-newstyle-opt-go.c          | 13 ++++++------
>  generator/states-oldstyle.c                 | 10 +++-------
>  lib/flags.c                                 | 22 +++++++++++++++++++++
>  lib/internal.h                              |  5 +++++
>  5 files changed, 42 insertions(+), 20 deletions(-)
ACK. Odd that the diffstat doesn't match that this does look simpler in
the end.  It also gives us a common hook point to enforce (which we may
want - although we're still deciding that) that secondary multi-conn
connections come up with the same eflags as the first.

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