[Qemu-devel] [PULL 0/3] NBD patches for 2.12-rc2

[Qemu-devel] [PULL 0/3] NBD patches for 2.12-rc2

[Eric Blake]

The following changes since commit f184de7553272223d6af731d7d623a7cebf710b5:

  Merge remote-tracking branch 'remotes/riscv/tags/riscv-qemu-2.12-critical-fixes' into staging (2018-03-31 09:42:33 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2018-04-02

for you to fetch changes up to 2b53af2523f6a3387f71372f59d3717f1f7d5fd9:

  nbd: trace meta context negotiation (2018-04-02 09:10:49 -0500)

----------------------------------------------------------------
nbd patches for 2018-04-02

- Eric Blake: nbd: Fix 32-bit compilation on BLOCK_STATUS
- Eric Blake: nbd/client: Correctly handle bad server REP_META_CONTEXT
- Eric Blake: nbd: trace meta context negotiation

----------------------------------------------------------------
Eric Blake (3):
      nbd: Fix 32-bit compilation on BLOCK_STATUS
      nbd/client: Correctly handle bad server REP_META_CONTEXT
      nbd: trace meta context negotiation

 block/nbd-client.c |  2 +-
 nbd/client.c       | 30 +++++++++++++++++++++++-------
 nbd/server.c       |  8 ++++++++
 nbd/trace-events   |  6 ++++++
 4 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.14.3

[Qemu-devel] [PULL 1/3] nbd: Fix 32-bit compilation on BLOCK_STATUS

[Eric Blake]

iotests 123 and 209 fail on 32-bit platforms.  The culprit:
sizeof(extent) is wrong; we want sizeof(*extent).  But since
the struct is 8 bytes, it happened to work on 64-bit platforms
where the pointer is also 8 bytes (nasty).

Fixes: 78a33ab58
Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180327210517.1804242-1-eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index e64e346d690..e7caf49fbb4 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -239,7 +239,7 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client,
 {
     uint32_t context_id;

-    if (chunk->length != sizeof(context_id) + sizeof(extent)) {
+    if (chunk->length != sizeof(context_id) + sizeof(*extent)) {
         error_setg(errp, "Protocol error: invalid payload for "
                          "NBD_REPLY_TYPE_BLOCK_STATUS");
         return -EINVAL;
-- 
2.14.3

[Qemu-devel] [PULL 2/3] nbd/client: Correctly handle bad server REP_META_CONTEXT

[Eric Blake]

It's never a good idea to blindly read for size bytes as
returned by the server without first validating that the size
is within bounds; a malicious or buggy server could cause us
to hang or get out of sync from reading further messages.

It may be smarter to try and teach the client to cope with
unexpected context ids by silently ignoring them instead of
hanging up on the server, but for now, if the server doesn't
reply with exactly the one context we expect, it's easier to
just give up - however, if we give up for any reason other
than an I/O failure, we might as well try to politely tell
the server we are quitting rather than continuing.

Fix some typos in the process.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180329231837.1914680-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 9b9b7f0ea29..dd0174b036e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -599,8 +599,8 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
  * Set one meta context. Simple means that reply must contain zero (not
  * negotiated) or one (negotiated) contexts. More contexts would be considered
  * as a protocol error. It's also implied that meta-data query equals queried
- * context name, so, if server replies with something different then @context,
- * it considered as error too.
+ * context name, so, if server replies with something different than @context,
+ * it is considered an error too.
  * return 1 for successful negotiation, context_id is set
  *        0 if operation is unsupported,
  *        -1 with errp set for any other error
@@ -649,25 +649,33 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,

     if (reply.type == NBD_REP_META_CONTEXT) {
         char *name;
-        size_t len;
+
+        if (reply.length != sizeof(received_id) + context_len) {
+            error_setg(errp, "Failed to negotiate meta context '%s', server "
+                       "answered with unexpected length %" PRIu32, context,
+                       reply.length);
+            nbd_send_opt_abort(ioc);
+            return -1;
+        }

         if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) {
             return -1;
         }
         be32_to_cpus(&received_id);

-        len = reply.length - sizeof(received_id);
-        name = g_malloc(len + 1);
-        if (nbd_read(ioc, name, len, errp) < 0) {
+        reply.length -= sizeof(received_id);
+        name = g_malloc(reply.length + 1);
+        if (nbd_read(ioc, name, reply.length, errp) < 0) {
             g_free(name);
             return -1;
         }
-        name[len] = '\0';
+        name[reply.length] = '\0';
         if (strcmp(context, name)) {
             error_setg(errp, "Failed to negotiate meta context '%s', server "
                        "answered with different context '%s'", context,
                        name);
             g_free(name);
+            nbd_send_opt_abort(ioc);
             return -1;
         }
         g_free(name);
@@ -690,6 +698,12 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
     if (reply.type != NBD_REP_ACK) {
         error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
                    reply.type, NBD_REP_ACK);
+        nbd_send_opt_abort(ioc);
+        return -1;
+    }
+    if (reply.length) {
+        error_setg(errp, "Unexpected length to ACK response");
+        nbd_send_opt_abort(ioc);
         return -1;
     }

-- 
2.14.3

[Qemu-devel] [PULL 3/3] nbd: trace meta context negotiation

[Eric Blake]

Having a more detailed log of the interaction between client and
server is invaluable in debugging how meta context negotiation
actually works.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180330130950.1931229-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c     | 2 ++
 nbd/server.c     | 8 ++++++++
 nbd/trace-events | 6 ++++++
 3 files changed, 16 insertions(+)

diff --git a/nbd/client.c b/nbd/client.c
index dd0174b036e..b9e175d1c27 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -623,6 +623,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
     char *data = g_malloc(data_len);
     char *p = data;

+    trace_nbd_opt_meta_request(context, export);
     stl_be_p(p, export_len);
     memcpy(p += sizeof(export_len), export, export_len);
     stl_be_p(p += export_len, 1);
@@ -680,6 +681,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
         }
         g_free(name);

+        trace_nbd_opt_meta_reply(context, received_id);
         received = true;

         /* receive NBD_REP_ACK */
diff --git a/nbd/server.c b/nbd/server.c
index cea158913ba..9e1f2271784 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -726,6 +726,7 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
         context_id = 0;
     }

+    trace_nbd_negotiate_meta_query_reply(context, context_id);
     set_be_option_rep(&opt.h, client->opt, NBD_REP_META_CONTEXT,
                       sizeof(opt) - sizeof(opt.h) + iov[1].iov_len);
     stl_be_p(&opt.context_id, context_id);
@@ -752,10 +753,12 @@ static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
         if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
             meta->base_allocation = true;
         }
+        trace_nbd_negotiate_meta_query_parse("base:");
         return 1;
     }

     if (len != alen) {
+        trace_nbd_negotiate_meta_query_skip("not base:allocation");
         return nbd_opt_skip(client, len, errp);
     }

@@ -768,6 +771,7 @@ static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
         meta->base_allocation = true;
     }

+    trace_nbd_negotiate_meta_query_parse("base:allocation");
     return 1;
 }

@@ -800,6 +804,7 @@ static int nbd_negotiate_meta_query(NBDClient *client,
     /* The only supported namespace for now is 'base'. So query should start
      * with 'base:'. Otherwise, we can ignore it and skip the remainder. */
     if (len < baselen) {
+        trace_nbd_negotiate_meta_query_skip("length too short");
         return nbd_opt_skip(client, len, errp);
     }

@@ -809,6 +814,7 @@ static int nbd_negotiate_meta_query(NBDClient *client,
         return ret;
     }
     if (strncmp(query, "base:", baselen) != 0) {
+        trace_nbd_negotiate_meta_query_skip("not for base: namespace");
         return nbd_opt_skip(client, len, errp);
     }

@@ -858,6 +864,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         return ret;
     }
     cpu_to_be32s(&nb_queries);
+    trace_nbd_negotiate_meta_context(nbd_opt_lookup(client->opt),
+                                     meta->export_name, nb_queries);

     if (client->opt == NBD_OPT_LIST_META_CONTEXT && !nb_queries) {
         /* enable all known contexts */
diff --git a/nbd/trace-events b/nbd/trace-events
index 0d03edc967d..dee081e7758 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -10,6 +10,8 @@ nbd_receive_query_exports_start(const char *wantname) "Querying export list for
 nbd_receive_query_exports_success(const char *wantname) "Found desired export name '%s'"
 nbd_receive_starttls_new_client(void) "Setting up TLS"
 nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
+nbd_opt_meta_request(const char *context, const char *export) "Requesting to set meta context %s for export %s"
+nbd_opt_meta_reply(const char *context, uint32_t id) "Received mapping of context %s to id %" PRIu32
 nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving negotiation tlscreds=%p hostname=%s"
 nbd_receive_negotiate_magic(uint64_t magic) "Magic is 0x%" PRIx64
 nbd_receive_negotiate_server_flags(uint32_t globalflags) "Global flags are 0x%" PRIx32
@@ -44,6 +46,10 @@ nbd_negotiate_handle_info_request(int request, const char *name) "Client request
 nbd_negotiate_handle_info_block_size(uint32_t minimum, uint32_t preferred, uint32_t maximum) "advertising minimum 0x%" PRIx32 ", preferred 0x%" PRIx32 ", maximum 0x%" PRIx32
 nbd_negotiate_handle_starttls(void) "Setting up TLS"
 nbd_negotiate_handle_starttls_handshake(void) "Starting TLS handshake"
+nbd_negotiate_meta_context(const char *optname, const char *export, uint32_t queries) "Client requested %s for export %s, with %" PRIu32 " queries"
+nbd_negotiate_meta_query_skip(const char *reason) "Skipping meta query: %s"
+nbd_negotiate_meta_query_parse(const char *query) "Parsed meta query '%s'"
+nbd_negotiate_meta_query_reply(const char *context, uint32_t id) "Replying with meta context '%s' id %" PRIu32
 nbd_negotiate_options_flags(uint32_t flags) "Received client flags 0x%" PRIx32
 nbd_negotiate_options_check_magic(uint64_t magic) "Checking opts magic 0x%" PRIx64
 nbd_negotiate_options_check_option(uint32_t option, const char *name) "Checking option %" PRIu32 " (%s)"
-- 
2.14.3

Re: [Qemu-devel] [PULL 0/3] NBD patches for 2.12-rc2

[Peter Maydell]

On 2 April 2018 at 15:16, Eric Blake <eblake@redhat.com> wrote:
> The following changes since commit f184de7553272223d6af731d7d623a7cebf710b5:
>
>   Merge remote-tracking branch 'remotes/riscv/tags/riscv-qemu-2.12-critical-fixes' into staging (2018-03-31 09:42:33 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2018-04-02
>
> for you to fetch changes up to 2b53af2523f6a3387f71372f59d3717f1f7d5fd9:
>
>   nbd: trace meta context negotiation (2018-04-02 09:10:49 -0500)
>
> ----------------------------------------------------------------
> nbd patches for 2018-04-02
>
> - Eric Blake: nbd: Fix 32-bit compilation on BLOCK_STATUS
> - Eric Blake: nbd/client: Correctly handle bad server REP_META_CONTEXT
> - Eric Blake: nbd: trace meta context negotiation
>
> ----------------------------------------------------------------
> Eric Blake (3):
>       nbd: Fix 32-bit compilation on BLOCK_STATUS
>       nbd/client: Correctly handle bad server REP_META_CONTEXT
>       nbd: trace meta context negotiation

Applied, thanks.

-- PMM