[Qemu-devel] [PATCH v4 00/11] nbd: tighter protocol compliance

[Qemu-devel] [PATCH v4 00/11] nbd: tighter protocol compliance

[Eric Blake]

Fix several corner-case bugs in our implementation of the NBD
protocol, both as client and as server.

Depends on Kevin's block-next branch:
git://repo.or.cz/qemu/kevin.git block-next

Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git nbd-flags-v4

Broken out of a larger v3 series[1], for easier review.  There
are still some places where we aren't quite compliant (for example,
the protocol recommends that the client send NBD_OPT_ABORT before
dropping the connection after receiving a valid server response it
didn't like but which did not violate protocol), but later series
will tackle that.

[1] https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03526.html

Changes in v4: rebase to latest block-next

001/11:[----] [--] 'nbd: Use BDRV_REQ_FUA for better FUA where supported'
002/11:[0004] [FC] 'nbd: More debug typo fixes, use correct formats'
003/11:[----] [--] 'nbd: Quit server after any write error'
004/11:[----] [--] 'nbd: Improve server handling of bogus commands'
005/11:[----] [--] 'nbd: Reject unknown request flags'
006/11:[----] [--] 'nbd: Group all Linux-specific ioctl code in one place'
007/11:[----] [--] 'nbd: Clean up ioctl handling of qemu-nbd -c'
008/11:[----] [-C] 'nbd: Limit nbdflags to 16 bits'
009/11:[----] [--] 'nbd: Add qemu-nbd -D for human-readable description'
010/11:[----] [--] 'nbd: Detect servers that send unexpected error values'
011/11:[----] [--] 'nbd: Avoid magic number for NBD max name size'

Eric Blake (11):
  nbd: Use BDRV_REQ_FUA for better FUA where supported
  nbd: More debug typo fixes, use correct formats
  nbd: Quit server after any write error
  nbd: Improve server handling of bogus commands
  nbd: Reject unknown request flags
  nbd: Group all Linux-specific ioctl code in one place
  nbd: Clean up ioctl handling of qemu-nbd -c
  nbd: Limit nbdflags to 16 bits
  nbd: Add qemu-nbd -D for human-readable description
  nbd: Detect servers that send unexpected error values
  nbd: Avoid magic number for NBD max name size

 block/nbd-client.h  |   2 +-
 include/block/nbd.h |  13 ++-
 nbd/nbd-internal.h  |   5 +-
 nbd/client.c        | 106 ++++++++++++++++---------
 nbd/server.c        | 224 +++++++++++++++++++++++++++++++---------------------
 qemu-nbd.c          |  16 +++-
 qemu-nbd.texi       |   5 +-
 7 files changed, 233 insertions(+), 138 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v4 01/11] nbd: Use BDRV_REQ_FUA for better FUA where supported

[Eric Blake]

Rather than always flushing ourselves, let the block layer
forward the FUA on to the underlying device - where all
underlying layers also understand FUA, we are now more
efficient; and where any underlying layer doesn't understand
it, now the block layer takes care of the full flush fallback
on our behalf.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index fa862cd..6079249 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1038,6 +1038,7 @@ static void nbd_trip(void *opaque)
     struct nbd_reply reply;
     ssize_t ret;
     uint32_t command;
+    int flags;

     TRACE("Reading request.");
     if (client->closing) {
@@ -1114,23 +1115,18 @@ static void nbd_trip(void *opaque)

         TRACE("Writing to device");

+        flags = 0;
+        if (request.type & NBD_CMD_FLAG_FUA) {
+            flags |= BDRV_REQ_FUA;
+        }
         ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
-                         req->data, request.len, 0);
+                         req->data, request.len, flags);
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
             goto error_reply;
         }

-        if (request.type & NBD_CMD_FLAG_FUA) {
-            ret = blk_co_flush(exp->blk);
-            if (ret < 0) {
-                LOG("flush failed");
-                reply.error = -ret;
-                goto error_reply;
-            }
-        }
-
         if (nbd_co_send_reply(req, &reply, 0) < 0) {
             goto out;
         }
-- 
2.5.5

[Qemu-devel] [PATCH v4 02/11] nbd: More debug typo fixes, use correct formats

[Eric Blake]

Clean up some debug message oddities missed earlier; this includes
some typos, and recognizing that %d is not necessarily compatible
with uint32_t. Also add a couple messages that I found useful
while debugging things.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: add a couple more tweaks, drop R-b
v3: rebase
---
 nbd/client.c | 41 ++++++++++++++++++++++-------------------
 nbd/server.c | 48 +++++++++++++++++++++++++++---------------------
 2 files changed, 49 insertions(+), 40 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 48f2a21..42e4e52 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -109,25 +109,27 @@ static int nbd_handle_reply_err(QIOChannel *ioc, uint32_t opt, uint32_t type,

     switch (type) {
     case NBD_REP_ERR_UNSUP:
-        TRACE("server doesn't understand request %d, attempting fallback",
-              opt);
+        TRACE("server doesn't understand request %" PRIx32
+              ", attempting fallback", opt);
         result = 0;
         goto cleanup;

     case NBD_REP_ERR_POLICY:
-        error_setg(errp, "Denied by server for option %x", opt);
+        error_setg(errp, "Denied by server for option %" PRIx32, opt);
         break;

     case NBD_REP_ERR_INVALID:
-        error_setg(errp, "Invalid data length for option %x", opt);
+        error_setg(errp, "Invalid data length for option %" PRIx32, opt);
         break;

     case NBD_REP_ERR_TLS_REQD:
-        error_setg(errp, "TLS negotiation required before option %x", opt);
+        error_setg(errp, "TLS negotiation required before option %" PRIx32,
+                   opt);
         break;

     default:
-        error_setg(errp, "Unknown error code when asking for option %x", opt);
+        error_setg(errp, "Unknown error code when asking for option %" PRIx32,
+                   opt);
         break;
     }

@@ -165,7 +167,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
     }
     opt = be32_to_cpu(opt);
     if (opt != NBD_OPT_LIST) {
-        error_setg(errp, "Unexpected option type %x expected %x",
+        error_setg(errp, "Unexpected option type %" PRIx32 " expected %x",
                    opt, NBD_OPT_LIST);
         return -1;
     }
@@ -207,7 +209,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
             return -1;
         }
         if (namelen > 255) {
-            error_setg(errp, "export name length too long %d", namelen);
+            error_setg(errp, "export name length too long %" PRIu32, namelen);
             return -1;
         }

@@ -234,7 +236,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
             g_free(buf);
         }
     } else {
-        error_setg(errp, "Unexpected reply type %x expected %x",
+        error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
                    type, NBD_REP_SERVER);
         return -1;
     }
@@ -349,7 +351,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
     }
     opt = be32_to_cpu(opt);
     if (opt != NBD_OPT_STARTTLS) {
-        error_setg(errp, "Unexpected option type %x expected %x",
+        error_setg(errp, "Unexpected option type %" PRIx32 " expected %x",
                    opt, NBD_OPT_STARTTLS);
         return NULL;
     }
@@ -361,7 +363,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
     }
     type = be32_to_cpu(type);
     if (type != NBD_REP_ACK) {
-        error_setg(errp, "Server rejected request to start TLS %x",
+        error_setg(errp, "Server rejected request to start TLS %" PRIx32,
                    type);
         return NULL;
     }
@@ -373,7 +375,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
     }
     length = be32_to_cpu(length);
     if (length != 0) {
-        error_setg(errp, "Start TLS reponse was not zero %x",
+        error_setg(errp, "Start TLS response was not zero %" PRIu32,
                    length);
         return NULL;
     }
@@ -384,7 +386,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
         return NULL;
     }
     data.loop = g_main_loop_new(g_main_context_default(), FALSE);
-    TRACE("Starting TLS hanshake");
+    TRACE("Starting TLS handshake");
     qio_channel_tls_handshake(tioc,
                               nbd_tls_handshake,
                               &data,
@@ -474,7 +476,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
         }
         globalflags = be16_to_cpu(globalflags);
         *flags = globalflags << 16;
-        TRACE("Global flags are %x", globalflags);
+        TRACE("Global flags are %" PRIx32, globalflags);
         if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
             fixedNewStyle = true;
             TRACE("Server supports fixed new style");
@@ -550,7 +552,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
         }
         exportflags = be16_to_cpu(exportflags);
         *flags |= exportflags;
-        TRACE("Export flags are %x", exportflags);
+        TRACE("Export flags are %" PRIx16, exportflags);
     } else if (magic == NBD_CLIENT_MAGIC) {
         if (name) {
             error_setg(errp, "Server does not support export names");
@@ -683,7 +685,8 @@ ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request)
     ssize_t ret;

     TRACE("Sending request to server: "
-          "{ .from = %" PRIu64", .len = %u, .handle = %" PRIu64", .type=%i}",
+          "{ .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64
+          ", .type=%" PRIu16 " }",
           request->from, request->len, request->handle, request->type);

     cpu_to_be32w((uint32_t*)buf, NBD_REQUEST_MAGIC);
@@ -732,12 +735,12 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply)

     reply->error = nbd_errno_to_system_errno(reply->error);

-    TRACE("Got reply: "
-          "{ magic = 0x%x, .error = %d, handle = %" PRIu64" }",
+    TRACE("Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32
+          ", handle = %" PRIu64" }",
           magic, reply->error, reply->handle);

     if (magic != NBD_REPLY_MAGIC) {
-        LOG("invalid magic (got 0x%x)", magic);
+        LOG("invalid magic (got 0x%" PRIx32 ")", magic);
         return -EINVAL;
     }
     return 0;
diff --git a/nbd/server.c b/nbd/server.c
index 6079249..fb6d3ce 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -196,7 +196,7 @@ static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)
     uint64_t magic;
     uint32_t len;

-    TRACE("Reply opt=%x type=%x", type, opt);
+    TRACE("Reply opt=%" PRIx32 " type=%" PRIx32, type, opt);

     magic = cpu_to_be64(NBD_REP_MAGIC);
     if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
@@ -226,7 +226,7 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
     uint64_t magic, name_len;
     uint32_t opt, type, len;

-    TRACE("Advertizing export name '%s'", exp->name ? exp->name : "");
+    TRACE("Advertising export name '%s'", exp->name ? exp->name : "");
     name_len = strlen(exp->name);
     magic = cpu_to_be64(NBD_REP_MAGIC);
     if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
@@ -392,12 +392,12 @@ static int nbd_negotiate_options(NBDClient *client)
     TRACE("Checking client flags");
     be32_to_cpus(&flags);
     if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
-        TRACE("Support supports fixed newstyle handshake");
+        TRACE("Client supports fixed newstyle handshake");
         fixedNewstyle = true;
         flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
     }
     if (flags != 0) {
-        TRACE("Unknown client flags 0x%x received", flags);
+        TRACE("Unknown client flags 0x%" PRIx32 " received", flags);
         return -EIO;
     }

@@ -431,12 +431,12 @@ static int nbd_negotiate_options(NBDClient *client)
         }
         length = be32_to_cpu(length);

-        TRACE("Checking option 0x%x", clientflags);
+        TRACE("Checking option 0x%" PRIx32, clientflags);
         if (client->tlscreds &&
             client->ioc == (QIOChannel *)client->sioc) {
             QIOChannel *tioc;
             if (!fixedNewstyle) {
-                TRACE("Unsupported option 0x%x", clientflags);
+                TRACE("Unsupported option 0x%" PRIx32, clientflags);
                 return -EINVAL;
             }
             switch (clientflags) {
@@ -455,7 +455,8 @@ static int nbd_negotiate_options(NBDClient *client)
                 return -EINVAL;

             default:
-                TRACE("Option 0x%x not permitted before TLS", clientflags);
+                TRACE("Option 0x%" PRIx32 " not permitted before TLS",
+                      clientflags);
                 if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
                     return -EIO;
                 }
@@ -493,7 +494,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 }
                 break;
             default:
-                TRACE("Unsupported option 0x%x", clientflags);
+                TRACE("Unsupported option 0x%" PRIx32, clientflags);
                 if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
                     return -EIO;
                 }
@@ -511,7 +512,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 return nbd_negotiate_handle_export_name(client, length);

             default:
-                TRACE("Unsupported option 0x%x", clientflags);
+                TRACE("Unsupported option 0x%" PRIx32, clientflags);
                 return -EINVAL;
             }
         }
@@ -560,6 +561,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
     oldStyle = client->exp != NULL && !client->tlscreds;
     if (oldStyle) {
         assert ((client->exp->nbdflags & ~65535) == 0);
+        TRACE("advertising size %" PRIu64 " and flags %" PRIx16,
+              client->exp->size, client->exp->nbdflags | myflags);
         stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
         stq_be_p(buf + 16, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
@@ -589,6 +592,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
         }

         assert ((client->exp->nbdflags & ~65535) == 0);
+        TRACE("advertising size %" PRIu64 " and flags %" PRIx16,
+              client->exp->size, client->exp->nbdflags | myflags);
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         if (nbd_negotiate_write(client->ioc, buf + 18, sizeof(buf) - 18) !=
@@ -652,12 +657,12 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, struct nbd_request *request)
     request->from  = be64_to_cpup((uint64_t*)(buf + 16));
     request->len   = be32_to_cpup((uint32_t*)(buf + 24));

-    TRACE("Got request: "
-          "{ magic = 0x%x, .type = %d, from = %" PRIu64" , len = %u }",
+    TRACE("Got request: { magic = 0x%" PRIx32 ", .type = %" PRIx32
+          ", from = %" PRIu64 " , len = %" PRIu32 " }",
           magic, request->type, request->from, request->len);

     if (magic != NBD_REQUEST_MAGIC) {
-        LOG("invalid magic (got 0x%x)", magic);
+        LOG("invalid magic (got 0x%" PRIx32 ")", magic);
         return -EINVAL;
     }
     return 0;
@@ -670,7 +675,8 @@ static ssize_t nbd_send_reply(QIOChannel *ioc, struct nbd_reply *reply)

     reply->error = system_errno_to_nbd_errno(reply->error);

-    TRACE("Sending response to client: { .error = %d, handle = %" PRIu64 " }",
+    TRACE("Sending response to client: { .error = %" PRId32
+          ", handle = %" PRIu64 " }",
           reply->error, reply->handle);

     /* Reply
@@ -999,7 +1005,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
     command = request->type & NBD_CMD_MASK_COMMAND;
     if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
         if (request->len > NBD_MAX_BUFFER_SIZE) {
-            LOG("len (%u) is larger than max len (%u)",
+            LOG("len (%" PRIu32" ) is larger than max len (%u)",
                 request->len, NBD_MAX_BUFFER_SIZE);
             rc = -EINVAL;
             goto out;
@@ -1012,7 +1018,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
         }
     }
     if (command == NBD_CMD_WRITE) {
-        TRACE("Reading %u byte(s)", request->len);
+        TRACE("Reading %" PRIu32 " byte(s)", request->len);

         if (read_sync(client->ioc, req->data, request->len) != request->len) {
             LOG("reading from socket failed");
@@ -1063,10 +1069,10 @@ static void nbd_trip(void *opaque)
     }
     command = request.type & NBD_CMD_MASK_COMMAND;
     if (command != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
-            LOG("From: %" PRIu64 ", Len: %u, Size: %" PRIu64
-            ", Offset: %" PRIu64 "\n",
-                    request.from, request.len,
-                    (uint64_t)exp->size, (uint64_t)exp->dev_offset);
+            LOG("From: %" PRIu64 ", Len: %" PRIu32", Size: %" PRIu64
+                ", Offset: %" PRIu64 "\n",
+                request.from, request.len,
+                (uint64_t)exp->size, (uint64_t)exp->dev_offset);
         LOG("requested operation past EOF--bad client?");
         goto invalid_request;
     }
@@ -1100,7 +1106,7 @@ static void nbd_trip(void *opaque)
             goto error_reply;
         }

-        TRACE("Read %u byte(s)", request.len);
+        TRACE("Read %" PRIu32" byte(s)", request.len);
         if (nbd_co_send_reply(req, &reply, request.len) < 0)
             goto out;
         break;
@@ -1161,7 +1167,7 @@ static void nbd_trip(void *opaque)
         }
         break;
     default:
-        LOG("invalid request type (%u) received", request.type);
+        LOG("invalid request type (%" PRIu32 ") received", request.type);
     invalid_request:
         reply.error = EINVAL;
     error_reply:
-- 
2.5.5

[Qemu-devel] [PATCH v4 03/11] nbd: Quit server after any write error

[Eric Blake]

We should never ignore failure from nbd_negotiate_send_rep(); if
we are unable to write to the client, then it is not worth trying
to continue the negotiation.  Fortunately, the problem is not
too severe - chances are that the errors being ignored here (mainly
inability to write the reply to the client) are indications of
a closed connection or something similar, which will also affect
the next attempt to interact with the client and eventually reach
a point where the errors are detected to end the loop.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index fb6d3ce..53507c5 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -334,7 +334,10 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
         return NULL;
     }

-    nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_STARTTLS);
+    if (nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
+                               NBD_OPT_STARTTLS) < 0) {
+        return NULL;
+    }

     tioc = qio_channel_tls_new_server(ioc,
                                       client->tlscreds,
@@ -460,8 +463,11 @@ static int nbd_negotiate_options(NBDClient *client)
                 if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
                     return -EIO;
                 }
-                nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_TLS_REQD,
-                                       clientflags);
+                ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_TLS_REQD,
+                                             clientflags);
+                if (ret < 0) {
+                    return ret;
+                }
                 break;
             }
         } else if (fixedNewstyle) {
@@ -485,12 +491,17 @@ static int nbd_negotiate_options(NBDClient *client)
                 }
                 if (client->tlscreds) {
                     TRACE("TLS already enabled");
-                    nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_INVALID,
-                                           clientflags);
+                    ret = nbd_negotiate_send_rep(client->ioc,
+                                                 NBD_REP_ERR_INVALID,
+                                                 clientflags);
                 } else {
                     TRACE("TLS not configured");
-                    nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_POLICY,
-                                           clientflags);
+                    ret = nbd_negotiate_send_rep(client->ioc,
+                                                 NBD_REP_ERR_POLICY,
+                                                 clientflags);
+                }
+                if (ret < 0) {
+                    return ret;
                 }
                 break;
             default:
@@ -498,8 +509,11 @@ static int nbd_negotiate_options(NBDClient *client)
                 if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
                     return -EIO;
                 }
-                nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP,
-                                       clientflags);
+                ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP,
+                                             clientflags);
+                if (ret < 0) {
+                    return ret;
+                }
                 break;
             }
         } else {
-- 
2.5.5

[Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake]

We have a few bugs in how we handle invalid client commands:

- A client can send an NBD_CMD_DISC where from + len overflows,
convincing us to reply with an error and stay connected, even
though the protocol requires us to silently disconnect. Fix by
hoisting the special case sooner.

- A client can send an NBD_CMD_WRITE with bogus from and len,
where we reply to the client with EINVAL without consuming the
payload; this will normally cause us to fail if the next thing
read is not the right magic, but in rare cases, could cause us
to interpret the data payload as valid commands and do things
not requested by the client. Fix by adding a complete flag to
track whether we are in sync or must disconnect.

- If we report an error to NBD_CMD_READ, we are not writing out
any data payload; but the protocol says that a client can expect
to read the payload no matter what (and must instead ignore it),
which means the client will start reading our next replies as
its data payload. Fix by disconnecting (an alternative fix of
sending bogus payload would be trickier to implement).

Furthermore, we have split the checks for bogus from/len across
two functions, when it is easier to do it all at once.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 67 +++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 21 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 53507c5..9ac7e01 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -52,6 +52,7 @@ struct NBDRequest {
     QSIMPLEQ_ENTRY(NBDRequest) entry;
     NBDClient *client;
     uint8_t *data;
+    bool complete;
 };

 struct NBDExport {
@@ -989,7 +990,13 @@ static ssize_t nbd_co_send_reply(NBDRequest *req, struct nbd_reply *reply,
     return rc;
 }

-static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *request)
+/* Collect a client request.  Return 0 if request looks valid, -EAGAIN
+ * to keep trying the collection, -EIO to drop connection right away,
+ * and any other negative value to report an error to the client
+ * (although the caller may still need to disconnect after reporting
+ * the error).  */
+static ssize_t nbd_co_receive_request(NBDRequest *req,
+                                      struct nbd_request *request)
 {
     NBDClient *client = req->client;
     uint32_t command;
@@ -1007,16 +1014,26 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
         goto out;
     }

-    if ((request->from + request->len) < request->from) {
-        LOG("integer overflow detected! "
-            "you're probably being attacked");
-        rc = -EINVAL;
-        goto out;
-    }
-
     TRACE("Decoding type");

     command = request->type & NBD_CMD_MASK_COMMAND;
+    if (command == NBD_CMD_DISC) {
+        /* Special case: we're going to disconnect without a reply,
+         * whether or not flags, from, or len are bogus */
+        TRACE("Request type is DISCONNECT");
+        rc = -EIO;
+        goto out;
+    }
+
+    /* Check for sanity in the parameters, part 1.  Defer as many
+     * checks as possible until after reading any NBD_CMD_WRITE
+     * payload, so we can try and keep the connection alive.  */
+    if ((request->from + request->len) < request->from) {
+        LOG("integer overflow detected, you're probably being attacked");
+        rc = -EINVAL;
+        goto out;
+    }
+
     if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
         if (request->len > NBD_MAX_BUFFER_SIZE) {
             LOG("len (%" PRIu32" ) is larger than max len (%u)",
@@ -1039,7 +1056,18 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
             rc = -EIO;
             goto out;
         }
+        req->complete = true;
     }
+
+    /* Sanity checks, part 2. */
+    if (request->from + request->len > client->exp->size) {
+        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
+            ", Size: %" PRIu64, request->from, request->len,
+            (uint64_t)client->exp->size);
+        rc = -EINVAL;
+        goto out;
+    }
+
     rc = 0;

 out:
@@ -1082,14 +1110,6 @@ static void nbd_trip(void *opaque)
         goto error_reply;
     }
     command = request.type & NBD_CMD_MASK_COMMAND;
-    if (command != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
-            LOG("From: %" PRIu64 ", Len: %" PRIu32", Size: %" PRIu64
-                ", Offset: %" PRIu64 "\n",
-                request.from, request.len,
-                (uint64_t)exp->size, (uint64_t)exp->dev_offset);
-        LOG("requested operation past EOF--bad client?");
-        goto invalid_request;
-    }

     if (client->closing) {
         /*
@@ -1151,10 +1171,11 @@ static void nbd_trip(void *opaque)
             goto out;
         }
         break;
+
     case NBD_CMD_DISC:
-        TRACE("Request type is DISCONNECT");
-        errno = 0;
-        goto out;
+        /* unreachable, thanks to special case in nbd_co_receive_request() */
+        abort();
+
     case NBD_CMD_FLUSH:
         TRACE("Request type is FLUSH");

@@ -1182,10 +1203,14 @@ static void nbd_trip(void *opaque)
         break;
     default:
         LOG("invalid request type (%" PRIu32 ") received", request.type);
-    invalid_request:
         reply.error = EINVAL;
     error_reply:
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
+        /* We must disconnect after replying with an error to
+         * NBD_CMD_READ, since we choose not to send bogus filler
+         * data; likewise after NBD_CMD_WRITE if we did not read the
+         * payload. */
+        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ ||
+            (command == NBD_CMD_WRITE && !req->complete)) {
             goto out;
         }
         break;
-- 
2.5.5

[Qemu-devel] [PATCH v4 05/11] nbd: Reject unknown request flags

[Eric Blake]

The NBD protocol says that clients should not send a command flag
that has not been negotiated (whether by the client requesting an
option during a handshake, or because we advertise support for the
flag in response to NBD_OPT_EXPORT_NAME), and that servers should
reject invalid flags with EINVAL.  We were silently ignoring the
flags instead.  The client can't rely on our behavior, since it is
their fault for passing the bad flag in the first place, but it's
better to be robust up front than to possibly behave differently
than the client was expecting with the attempted flag.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alex Bligh <alex@alex.org.uk>

---
v3: reorder in series, defer check until after NBD_CMD_WRITE payload
is consumed
---
 nbd/server.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index 9ac7e01..2ef2dfa 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1067,6 +1067,11 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
         rc = -EINVAL;
         goto out;
     }
+    if (request->type & ~NBD_CMD_MASK_COMMAND & ~NBD_CMD_FLAG_FUA) {
+        LOG("unsupported flags (got 0x%x)",
+            request->type & ~NBD_CMD_MASK_COMMAND);
+        return -EINVAL;
+    }

     rc = 0;

-- 
2.5.5

[Qemu-devel] [PATCH v4 06/11] nbd: Group all Linux-specific ioctl code in one place

[Eric Blake]

NBD ioctl()s are used to manage an NBD client session where
initial handshake is done in userspace, but then the transmission
phase is handed off to the kernel through a /dev/nbdX device.
As such, all ioctls sent to the kernel on the /dev/nbdX fd belong
in client.c; nbd_disconnect() was out-of-place in server.c.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c | 13 +++++++++++++
 nbd/server.c | 18 ------------------
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 42e4e52..ae9fdd4 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -667,6 +667,15 @@ int nbd_client(int fd)
     errno = serrno;
     return ret;
 }
+
+int nbd_disconnect(int fd)
+{
+    ioctl(fd, NBD_CLEAR_QUE);
+    ioctl(fd, NBD_DISCONNECT);
+    ioctl(fd, NBD_CLEAR_SOCK);
+    return 0;
+}
+
 #else
 int nbd_init(int fd, QIOChannelSocket *ioc, uint32_t flags, off_t size)
 {
@@ -677,6 +686,10 @@ int nbd_client(int fd)
 {
     return -ENOTSUP;
 }
+int nbd_disconnect(int fd)
+{
+    return -ENOTSUP;
+}
 #endif

 ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request)
diff --git a/nbd/server.c b/nbd/server.c
index 2ef2dfa..77b0385 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -624,24 +624,6 @@ fail:
     return rc;
 }

-#ifdef __linux__
-
-int nbd_disconnect(int fd)
-{
-    ioctl(fd, NBD_CLEAR_QUE);
-    ioctl(fd, NBD_DISCONNECT);
-    ioctl(fd, NBD_CLEAR_SOCK);
-    return 0;
-}
-
-#else
-
-int nbd_disconnect(int fd)
-{
-    return -ENOTSUP;
-}
-#endif
-
 static ssize_t nbd_receive_request(QIOChannel *ioc, struct nbd_request *request)
 {
     uint8_t buf[NBD_REQUEST_SIZE];
-- 
2.5.5

[Qemu-devel] [PATCH v4 07/11] nbd: Clean up ioctl handling of qemu-nbd -c

[Eric Blake]

The kernel ioctl() interface into NBD is limited to 'unsigned long';
we MUST pass in input with that type (and not int or size_t, as
there may be platform ABIs where the wrong types promote incorrectly
through var-args).  Furthermore, on 32-bit platforms, the kernel
is limited to a maximum export size of 2T (our BLKSIZE of 512 times
a SIZE_BLOCKS constrained by 32 bit unsigned long).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index ae9fdd4..f1afa49 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -593,9 +593,15 @@ fail:
 #ifdef __linux__
 int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size)
 {
+    unsigned long sectors = size / BDRV_SECTOR_SIZE;
+    if (size / BDRV_SECTOR_SIZE != sectors) {
+        LOG("Export size %lld too large for 32-bit kernel", (long long) size);
+        return -E2BIG;
+    }
+
     TRACE("Setting NBD socket");

-    if (ioctl(fd, NBD_SET_SOCK, sioc->fd) < 0) {
+    if (ioctl(fd, NBD_SET_SOCK, (unsigned long) sioc->fd) < 0) {
         int serrno = errno;
         LOG("Failed to set NBD socket");
         return -serrno;
@@ -603,21 +609,25 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size)

     TRACE("Setting block size to %lu", (unsigned long)BDRV_SECTOR_SIZE);

-    if (ioctl(fd, NBD_SET_BLKSIZE, (size_t)BDRV_SECTOR_SIZE) < 0) {
+    if (ioctl(fd, NBD_SET_BLKSIZE, (unsigned long)BDRV_SECTOR_SIZE) < 0) {
         int serrno = errno;
         LOG("Failed setting NBD block size");
         return -serrno;
     }

-    TRACE("Setting size to %zd block(s)", (size_t)(size / BDRV_SECTOR_SIZE));
+    TRACE("Setting size to %lu block(s)", sectors);
+    if (size % BDRV_SECTOR_SIZE) {
+        TRACE("Ignoring trailing %d bytes of export",
+              (int) (size % BDRV_SECTOR_SIZE));
+    }

-    if (ioctl(fd, NBD_SET_SIZE_BLOCKS, (size_t)(size / BDRV_SECTOR_SIZE)) < 0) {
+    if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
         int serrno = errno;
         LOG("Failed setting size (in blocks)");
         return -serrno;
     }

-    if (ioctl(fd, NBD_SET_FLAGS, flags) < 0) {
+    if (ioctl(fd, NBD_SET_FLAGS, (unsigned long) flags) < 0) {
         if (errno == ENOTTY) {
             int read_only = (flags & NBD_FLAG_READ_ONLY) != 0;
             TRACE("Setting readonly attribute");
-- 
2.5.5

[Qemu-devel] [PATCH v4 08/11] nbd: Limit nbdflags to 16 bits

[Eric Blake]

Rather than asserting that nbdflags is within range, just give
it the correct type to begin with :)  nbdflags corresponds to
the per-export portion of NBD Protocol "transmission flags", which
is 16 bits in response to NBD_OPT_EXPORT_NAME and NBD_OPT_GO.

Furthermore, upstream NBD has never passed the global flags to
the kernel via ioctl(NBD_SET_FLAGS) (the ioctl was first
introduced in NBD 2.9.22; then a latent bug in NBD 3.1 actually
tried to OR the global flags with the transmission flags, with
the disaster that the addition of NBD_FLAG_NO_ZEROES in 3.9
caused all earlier NBD 3.x clients to treat every export as
read-only; NBD 3.10 and later intentionally clip things to 16
bits to pass only transmission flags).  Qemu should follow suit,
since the current two global flags (NBD_FLAG_FIXED_NEWSTYLE
and NBD_FLAG_NO_ZEROES) have no impact on the kernel's behavior
during transmission.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: expand scope of patch
---
 block/nbd-client.h  |  2 +-
 include/block/nbd.h |  6 +++---
 nbd/client.c        | 28 +++++++++++++++-------------
 nbd/server.c        | 10 ++++------
 qemu-nbd.c          |  4 ++--
 5 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index c618dad..0596b75 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -20,7 +20,7 @@
 typedef struct NbdClientSession {
     QIOChannelSocket *sioc; /* The master data channel */
     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
-    uint32_t nbdflags;
+    uint16_t nbdflags;
     off_t size;

     CoMutex send_mutex;
diff --git a/include/block/nbd.h b/include/block/nbd.h
index b86a976..134f117 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -83,11 +83,11 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
                      size_t offset,
                      size_t length,
                      bool do_read);
-int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
                           QCryptoTLSCreds *tlscreds, const char *hostname,
                           QIOChannel **outioc,
                           off_t *size, Error **errp);
-int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
+int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size);
 ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
 ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply);
 int nbd_client(int fd);
@@ -97,7 +97,7 @@ typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;

 NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
-                          uint32_t nbdflags, void (*close)(NBDExport *),
+                          uint16_t nbdflags, void (*close)(NBDExport *),
                           Error **errp);
 void nbd_export_close(NBDExport *exp);
 void nbd_export_get(NBDExport *exp);
diff --git a/nbd/client.c b/nbd/client.c
index f1afa49..937344c 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -406,7 +406,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 }


-int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
                           QCryptoTLSCreds *tlscreds, const char *hostname,
                           QIOChannel **outioc,
                           off_t *size, Error **errp)
@@ -466,7 +466,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
         uint32_t opt;
         uint32_t namesize;
         uint16_t globalflags;
-        uint16_t exportflags;
         bool fixedNewStyle = false;

         if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
@@ -475,7 +474,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             goto fail;
         }
         globalflags = be16_to_cpu(globalflags);
-        *flags = globalflags << 16;
         TRACE("Global flags are %" PRIx32, globalflags);
         if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
             fixedNewStyle = true;
@@ -543,17 +541,15 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             goto fail;
         }
         *size = be64_to_cpu(s);
-        TRACE("Size is %" PRIu64, *size);

-        if (read_sync(ioc, &exportflags, sizeof(exportflags)) !=
-            sizeof(exportflags)) {
+        if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
-        exportflags = be16_to_cpu(exportflags);
-        *flags |= exportflags;
-        TRACE("Export flags are %" PRIx16, exportflags);
+        be16_to_cpus(flags);
     } else if (magic == NBD_CLIENT_MAGIC) {
+        uint32_t oldflags;
+
         if (name) {
             error_setg(errp, "Server does not support export names");
             goto fail;
@@ -570,16 +566,22 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
         *size = be64_to_cpu(s);
         TRACE("Size is %" PRIu64, *size);

-        if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
+        if (read_sync(ioc, &oldflags, sizeof(oldflags)) != sizeof(oldflags)) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
-        *flags = be32_to_cpup(flags);
+        be32_to_cpus(&oldflags);
+        if (oldflags & ~0xffff) {
+            error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
+            goto fail;
+        }
+        *flags = oldflags;
     } else {
         error_setg(errp, "Bad magic received");
         goto fail;
     }

+    TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
     if (read_sync(ioc, &buf, 124) != 124) {
         error_setg(errp, "Failed to read reserved block");
         goto fail;
@@ -591,7 +593,7 @@ fail:
 }

 #ifdef __linux__
-int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
 {
     unsigned long sectors = size / BDRV_SECTOR_SIZE;
     if (size / BDRV_SECTOR_SIZE != sectors) {
@@ -687,7 +689,7 @@ int nbd_disconnect(int fd)
 }

 #else
-int nbd_init(int fd, QIOChannelSocket *ioc, uint32_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size)
 {
     return -ENOTSUP;
 }
diff --git a/nbd/server.c b/nbd/server.c
index 77b0385..330ffd7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -63,7 +63,7 @@ struct NBDExport {
     char *name;
     off_t dev_offset;
     off_t size;
-    uint32_t nbdflags;
+    uint16_t nbdflags;
     QTAILQ_HEAD(, NBDClient) clients;
     QTAILQ_ENTRY(NBDExport) next;

@@ -544,8 +544,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
     NBDClient *client = data->client;
     char buf[8 + 8 + 8 + 128];
     int rc;
-    const int myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
-                         NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
+    const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
+                              NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
     bool oldStyle;

     /* Old style negotiation header without options
@@ -575,7 +575,6 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)

     oldStyle = client->exp != NULL && !client->tlscreds;
     if (oldStyle) {
-        assert ((client->exp->nbdflags & ~65535) == 0);
         TRACE("advertising size %" PRIu64 " and flags %" PRIx16,
               client->exp->size, client->exp->nbdflags | myflags);
         stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
@@ -606,7 +605,6 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
             goto fail;
         }

-        assert ((client->exp->nbdflags & ~65535) == 0);
         TRACE("advertising size %" PRIu64 " and flags %" PRIx16,
               client->exp->size, client->exp->nbdflags | myflags);
         stq_be_p(buf + 18, client->exp->size);
@@ -810,7 +808,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
 }

 NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
-                          uint32_t nbdflags, void (*close)(NBDExport *),
+                          uint16_t nbdflags, void (*close)(NBDExport *),
                           Error **errp)
 {
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 3e54113..3ee19a2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -246,7 +246,7 @@ static void *nbd_client_thread(void *arg)
 {
     char *device = arg;
     off_t size;
-    uint32_t nbdflags;
+    uint16_t nbdflags;
     QIOChannelSocket *sioc;
     int fd;
     int ret;
@@ -460,7 +460,7 @@ int main(int argc, char **argv)
     BlockBackend *blk;
     BlockDriverState *bs;
     off_t dev_offset = 0;
-    uint32_t nbdflags = 0;
+    uint16_t nbdflags = 0;
     bool disconnect = false;
     const char *bindto = "0.0.0.0";
     const char *port = NULL;
-- 
2.5.5

[Qemu-devel] [PATCH v4 09/11] nbd: Add qemu-nbd -D for human-readable description

[Eric Blake]

The NBD protocol allows servers to advertise a human-readable
description alongside an export name during NBD_OPT_LIST.  Add
an option to pass through the user's string to the NBD client.

Doing this also makes it easier to test commit 200650d4, which
is the client counterpart of receiving the description.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  1 +
 nbd/nbd-internal.h  |  5 +++--
 nbd/server.c        | 34 ++++++++++++++++++++++++++--------
 qemu-nbd.c          | 12 +++++++++++-
 qemu-nbd.texi       |  5 ++++-
 5 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 134f117..3e2d76b 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -107,6 +107,7 @@ BlockBackend *nbd_export_get_blockdev(NBDExport *exp);

 NBDExport *nbd_export_find(const char *name);
 void nbd_export_set_name(NBDExport *exp, const char *name);
+void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);

 void nbd_client_new(NBDExport *exp,
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 3791535..035ead4 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -103,9 +103,10 @@ static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
     return nbd_wr_syncv(ioc, &iov, 1, 0, size, true);
 }

-static inline ssize_t write_sync(QIOChannel *ioc, void *buffer, size_t size)
+static inline ssize_t write_sync(QIOChannel *ioc, const void *buffer,
+                                 size_t size)
 {
-    struct iovec iov = { .iov_base = buffer, .iov_len = size };
+    struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };

     return nbd_wr_syncv(ioc, &iov, 1, 0, size, false);
 }
diff --git a/nbd/server.c b/nbd/server.c
index 330ffd7..b8de8aa 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -61,6 +61,7 @@ struct NBDExport {

     BlockBackend *blk;
     char *name;
+    char *description;
     off_t dev_offset;
     off_t size;
     uint16_t nbdflags;
@@ -128,7 +129,8 @@ static ssize_t nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)

 }

-static ssize_t nbd_negotiate_write(QIOChannel *ioc, void *buffer, size_t size)
+static ssize_t nbd_negotiate_write(QIOChannel *ioc, const void *buffer,
+                                   size_t size)
 {
     ssize_t ret;
     guint watch;
@@ -224,11 +226,15 @@ static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)

 static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
 {
-    uint64_t magic, name_len;
+    uint64_t magic;
+    size_t name_len, desc_len;
     uint32_t opt, type, len;
+    const char *name = exp->name ? exp->name : "";
+    const char *desc = exp->description ? exp->description : "";

-    TRACE("Advertising export name '%s'", exp->name ? exp->name : "");
-    name_len = strlen(exp->name);
+    TRACE("Advertising export name '%s' description '%s'", name, desc);
+    name_len = strlen(name);
+    desc_len = strlen(desc);
     magic = cpu_to_be64(NBD_REP_MAGIC);
     if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
         LOG("write failed (magic)");
@@ -244,18 +250,22 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
         LOG("write failed (reply type)");
         return -EINVAL;
     }
-    len = cpu_to_be32(name_len + sizeof(len));
+    len = cpu_to_be32(name_len + desc_len + sizeof(len));
     if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
         LOG("write failed (length)");
         return -EINVAL;
     }
     len = cpu_to_be32(name_len);
     if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
-        LOG("write failed (length)");
+        LOG("write failed (name length)");
         return -EINVAL;
     }
-    if (nbd_negotiate_write(ioc, exp->name, name_len) != name_len) {
-        LOG("write failed (buffer)");
+    if (nbd_negotiate_write(ioc, name, name_len) != name_len) {
+        LOG("write failed (name buffer)");
+        return -EINVAL;
+    }
+    if (nbd_negotiate_write(ioc, desc, desc_len) != desc_len) {
+        LOG("write failed (description buffer)");
         return -EINVAL;
     }
     return 0;
@@ -881,6 +891,12 @@ void nbd_export_set_name(NBDExport *exp, const char *name)
     nbd_export_put(exp);
 }

+void nbd_export_set_description(NBDExport *exp, const char *description)
+{
+    g_free(exp->description);
+    exp->description = g_strdup(description);
+}
+
 void nbd_export_close(NBDExport *exp)
 {
     NBDClient *client, *next;
@@ -890,6 +906,7 @@ void nbd_export_close(NBDExport *exp)
         client_close(client);
     }
     nbd_export_set_name(exp, NULL);
+    nbd_export_set_description(exp, NULL);
     nbd_export_put(exp);
 }

@@ -908,6 +925,7 @@ void nbd_export_put(NBDExport *exp)

     if (--exp->refcount == 0) {
         assert(exp->name == NULL);
+        assert(exp->description == NULL);

         if (exp->close) {
             exp->close(exp);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 3ee19a2..1fa782a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -79,6 +79,7 @@ static void usage(const char *name)
 "  -t, --persistent          don't exit on the last connection\n"
 "  -v, --verbose             display extra debugging information\n"
 "  -x, --export-name=NAME    expose export by name\n"
+"  -D, --description=TEXT    with -x, also export a human-readable description\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
@@ -469,7 +470,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:D:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -495,6 +496,7 @@ int main(int argc, char **argv)
         { "verbose", no_argument, NULL, 'v' },
         { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
         { "export-name", required_argument, NULL, 'x' },
+        { "description", required_argument, NULL, 'D' },
         { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
         { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
         { NULL, 0, NULL, 0 }
@@ -514,6 +516,7 @@ int main(int argc, char **argv)
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
     const char *export_name = NULL;
+    const char *export_description = NULL;
     const char *tlscredsid = NULL;
     bool imageOpts = false;
     bool writethrough = true;
@@ -677,6 +680,9 @@ int main(int argc, char **argv)
         case 'x':
             export_name = optarg;
             break;
+        case 'D':
+            export_description = optarg;
+            break;
         case 'v':
             verbose = 1;
             break;
@@ -903,7 +909,11 @@ int main(int argc, char **argv)
     }
     if (export_name) {
         nbd_export_set_name(exp, export_name);
+        nbd_export_set_description(exp, export_description);
         newproto = true;
+    } else if (export_description) {
+        error_report("Export description requires an export name");
+        exit(EXIT_FAILURE);
     }

     server_ioc = qio_channel_socket_new();
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9f23343..923de74 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -79,9 +79,12 @@ Disconnect the device @var{dev}
 Allow up to @var{num} clients to share the device (default @samp{1})
 @item -t, --persistent
 Don't exit on the last connection
-@item -x NAME, --export-name=NAME
+@item -x, --export-name=@var{name}
 Set the NBD volume export name. This switches the server to use
 the new style NBD protocol negotiation
+@item -D, --description=@var{description}
+Set the NBD volume export description, as a human-readable
+string. Requires the use of @option{-x}
 @item --tls-creds=ID
 Enable mandatory TLS encryption for the server by setting the ID
 of the TLS credentials object previously created with the --object
-- 
2.5.5

[Qemu-devel] [PATCH v4 10/11] nbd: Detect servers that send unexpected error values

[Eric Blake]

Add some debugging to flag servers that are not compliant to
the NBD protocol.  This would have flagged the server bug
fixed in commit c0301fcc.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alex Bligh <alex@alex.org.uk>

---
v3: later in series, but no change
---
 nbd/client.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/nbd/client.c b/nbd/client.c
index 937344c..4659df3 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -33,8 +33,10 @@ static int nbd_errno_to_system_errno(int err)
         return ENOMEM;
     case NBD_ENOSPC:
         return ENOSPC;
+    default:
+        TRACE("Squashing unexpected error %d to EINVAL", err);
+        /* fallthrough */
     case NBD_EINVAL:
-    default:
         return EINVAL;
     }
 }
-- 
2.5.5

[Qemu-devel] [PATCH v4 11/11] nbd: Avoid magic number for NBD max name size

[Eric Blake]

Declare a constant and use that when determining if an export
name fits within the constraints we are willing to support.

Note that upstream NBD recently documented that clients MUST
support export names of 256 bytes (not including trailing NUL),
and SHOULD support names up to 4096 bytes.  4096 is a bit big
(we would lose benefits of stack-allocation of a name array),
and we already have other limits in place (for example, qcow2
snapshot names are clamped around 1024).  So for now, just
stick to the required minimum, as that's easier to audit than
a full-scale support for larger names.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: enlarge the limit, and document choice of new value
---
 include/block/nbd.h | 6 ++++++
 nbd/client.c        | 2 +-
 nbd/server.c        | 4 ++--
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 3e2d76b..2c753cc 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -76,6 +76,12 @@ enum {

 /* Maximum size of a single READ/WRITE data buffer */
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
+/* Maximum size of an export name. The NBD spec requires 256 and
+ * suggests that servers support up to 4096, but we stick to only the
+ * required size so that we can stack-allocate the names, and because
+ * going larger would require an audit of more code to make sure we
+ * aren't overflowing some other buffer. */
+#define NBD_MAX_NAME_SIZE 256

 ssize_t nbd_wr_syncv(QIOChannel *ioc,
                      struct iovec *iov,
diff --git a/nbd/client.c b/nbd/client.c
index 4659df3..b700100 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -210,7 +210,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
             error_setg(errp, "incorrect option name length");
             return -1;
         }
-        if (namelen > 255) {
+        if (namelen > NBD_MAX_NAME_SIZE) {
             error_setg(errp, "export name length too long %" PRIu32, namelen);
             return -1;
         }
diff --git a/nbd/server.c b/nbd/server.c
index b8de8aa..7ffa168 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -296,13 +296,13 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
 static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
 {
     int rc = -EINVAL;
-    char name[256];
+    char name[NBD_MAX_NAME_SIZE + 1];

     /* Client sends:
         [20 ..  xx]   export name (length bytes)
      */
     TRACE("Checking length");
-    if (length > 255) {
+    if (length >= sizeof(name)) {
         LOG("Bad length received");
         goto fail;
     }
-- 
2.5.5

Re: [Qemu-devel] [PATCH v4 09/11] nbd: Add qemu-nbd -D for human-readable description

[Daniel P. Berrange]

On Wed, May 11, 2016 at 04:39:42PM -0600, Eric Blake wrote:
> The NBD protocol allows servers to advertise a human-readable
> description alongside an export name during NBD_OPT_LIST.  Add
> an option to pass through the user's string to the NBD client.
> 
> Doing this also makes it easier to test commit 200650d4, which
> is the client counterpart of receiving the description.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/nbd.h |  1 +
>  nbd/nbd-internal.h  |  5 +++--
>  nbd/server.c        | 34 ++++++++++++++++++++++++++--------
>  qemu-nbd.c          | 12 +++++++++++-
>  qemu-nbd.texi       |  5 ++++-
>  5 files changed, 45 insertions(+), 12 deletions(-)

> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 9f23343..923de74 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -79,9 +79,12 @@ Disconnect the device @var{dev}
>  Allow up to @var{num} clients to share the device (default @samp{1})
>  @item -t, --persistent
>  Don't exit on the last connection
> -@item -x NAME, --export-name=NAME
> +@item -x, --export-name=@var{name}

Why this change - that reads as saying that '-x' doesn't take any value
which is wrong IMHO

>  Set the NBD volume export name. This switches the server to use
>  the new style NBD protocol negotiation
> +@item -D, --description=@var{description}

Likewise this suggests -D doesn't take a value

> +Set the NBD volume export description, as a human-readable
> +string. Requires the use of @option{-x}
>  @item --tls-creds=ID
>  Enable mandatory TLS encryption for the server by setting the ID
>  of the TLS credentials object previously created with the --object

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v4 09/11] nbd: Add qemu-nbd -D for human-readable description

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

On 05/12/2016 01:47 AM, Daniel P. Berrange wrote:
> On Wed, May 11, 2016 at 04:39:42PM -0600, Eric Blake wrote:
>> The NBD protocol allows servers to advertise a human-readable
>> description alongside an export name during NBD_OPT_LIST.  Add
>> an option to pass through the user's string to the NBD client.
>>
>> Doing this also makes it easier to test commit 200650d4, which
>> is the client counterpart of receiving the description.
>>

>> -@item -x NAME, --export-name=NAME
>> +@item -x, --export-name=@var{name}
> 
> Why this change - that reads as saying that '-x' doesn't take any value
> which is wrong IMHO

It's consistent with other options-with-arguments in the same file, such as:

@item -p, --port=@var{port}
@item -o, --offset=@var{offset}
@item -b, --bind=@var{iface}
@item -k, --socket=@var{path}

etc. Basically, we want to use this common escape hatch (see 'ls
--help', for example):

Mandatory arguments to long options are mandatory for short options too.

>> +@item -D, --description=@var{description}
> 
> Likewise this suggests -D doesn't take a value

If you don't like it, then we should have a separate patch for the
entire file should switch over to:

@item -D @var{description}, --description=@var{description}

style, where the mandatory option is listed for both option spellings
for every affected option.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 09/11] nbd: Add qemu-nbd -D for human-readable description

[Daniel P. Berrange]

On Thu, May 12, 2016 at 09:38:58AM -0600, Eric Blake wrote:
> On 05/12/2016 01:47 AM, Daniel P. Berrange wrote:
> > On Wed, May 11, 2016 at 04:39:42PM -0600, Eric Blake wrote:
> >> The NBD protocol allows servers to advertise a human-readable
> >> description alongside an export name during NBD_OPT_LIST.  Add
> >> an option to pass through the user's string to the NBD client.
> >>
> >> Doing this also makes it easier to test commit 200650d4, which
> >> is the client counterpart of receiving the description.
> >>
> 
> >> -@item -x NAME, --export-name=NAME
> >> +@item -x, --export-name=@var{name}
> > 
> > Why this change - that reads as saying that '-x' doesn't take any value
> > which is wrong IMHO
> 
> It's consistent with other options-with-arguments in the same file, such as:
> 
> @item -p, --port=@var{port}
> @item -o, --offset=@var{offset}
> @item -b, --bind=@var{iface}
> @item -k, --socket=@var{path}
> 
> etc. Basically, we want to use this common escape hatch (see 'ls
> --help', for example):
> 
> Mandatory arguments to long options are mandatory for short options too.

Ah, I didn't realize it was standard practice todo that - i personally
just historically include the arg value in both, but if QEMU doesn't
that's ok - consistency is more important.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v4 00/11] nbd: tighter protocol compliance

[Alex Bligh]

On 11 May 2016, at 23:39, Eric Blake <eblake@redhat.com> wrote:

> Fix several corner-case bugs in our implementation of the NBD
> protocol, both as client and as server.

I thought I'd added a Reviewed-By: line to more of these before.
On a very very quick look, they all look good to me.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v4 00/11] nbd: tighter protocol compliance

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]

ping

On 05/11/2016 04:39 PM, Eric Blake wrote:
> Fix several corner-case bugs in our implementation of the NBD
> protocol, both as client and as server.
> 
> Depends on Kevin's block-next branch:
> git://repo.or.cz/qemu/kevin.git block-next
> 
> Also available as a tag at this location:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-flags-v4
> 
> Broken out of a larger v3 series[1], for easier review.  There
> are still some places where we aren't quite compliant (for example,
> the protocol recommends that the client send NBD_OPT_ABORT before
> dropping the connection after receiving a valid server response it
> didn't like but which did not violate protocol), but later series
> will tackle that.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03526.html
> 
> Changes in v4: rebase to latest block-next
> 
> 001/11:[----] [--] 'nbd: Use BDRV_REQ_FUA for better FUA where supported'
> 002/11:[0004] [FC] 'nbd: More debug typo fixes, use correct formats'
> 003/11:[----] [--] 'nbd: Quit server after any write error'
> 004/11:[----] [--] 'nbd: Improve server handling of bogus commands'
> 005/11:[----] [--] 'nbd: Reject unknown request flags'
> 006/11:[----] [--] 'nbd: Group all Linux-specific ioctl code in one place'
> 007/11:[----] [--] 'nbd: Clean up ioctl handling of qemu-nbd -c'
> 008/11:[----] [-C] 'nbd: Limit nbdflags to 16 bits'
> 009/11:[----] [--] 'nbd: Add qemu-nbd -D for human-readable description'
> 010/11:[----] [--] 'nbd: Detect servers that send unexpected error values'
> 011/11:[----] [--] 'nbd: Avoid magic number for NBD max name size'
> 
> Eric Blake (11):
>   nbd: Use BDRV_REQ_FUA for better FUA where supported
>   nbd: More debug typo fixes, use correct formats
>   nbd: Quit server after any write error
>   nbd: Improve server handling of bogus commands
>   nbd: Reject unknown request flags
>   nbd: Group all Linux-specific ioctl code in one place
>   nbd: Clean up ioctl handling of qemu-nbd -c
>   nbd: Limit nbdflags to 16 bits
>   nbd: Add qemu-nbd -D for human-readable description
>   nbd: Detect servers that send unexpected error values
>   nbd: Avoid magic number for NBD max name size
> 
>  block/nbd-client.h  |   2 +-
>  include/block/nbd.h |  13 ++-
>  nbd/nbd-internal.h  |   5 +-
>  nbd/client.c        | 106 ++++++++++++++++---------
>  nbd/server.c        | 224 +++++++++++++++++++++++++++++++---------------------
>  qemu-nbd.c          |  16 +++-
>  qemu-nbd.texi       |   5 +-
>  7 files changed, 233 insertions(+), 138 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 02/11] nbd: More debug typo fixes, use correct formats

[Paolo Bonzini]

On 12/05/2016 00:39, Eric Blake wrote:
> Clean up some debug message oddities missed earlier; this includes
> some typos, and recognizing that %d is not necessarily compatible
> with uint32_t.

Actually it should be on any POSIX platform, since (by way of the
requirements on limits.h) POSIX requires sizeof(int) to be >= 4.

I will apply the patch, but fully expect this to bitrot...

Paolo

> Also add a couple messages that I found useful
> while debugging things.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 12/05/2016 00:39, Eric Blake wrote:
> - If we report an error to NBD_CMD_READ, we are not writing out
> any data payload; but the protocol says that a client can expect
> to read the payload no matter what (and must instead ignore it),
> which means the client will start reading our next replies as
> its data payload. Fix by disconnecting (an alternative fix of
> sending bogus payload would be trickier to implement).

This is an error in the spec.  The Linux driver doesn't expect to read
the payload here, and neither does block/nbd-client.c.

Paolo

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 12/05/2016 00:39, Eric Blake wrote:
> We have a few bugs in how we handle invalid client commands:
> 
> - A client can send an NBD_CMD_DISC where from + len overflows,
> convincing us to reply with an error and stay connected, even
> though the protocol requires us to silently disconnect. Fix by
> hoisting the special case sooner.
> 
> - A client can send an NBD_CMD_WRITE with bogus from and len,
> where we reply to the client with EINVAL without consuming the
> payload; this will normally cause us to fail if the next thing
> read is not the right magic, but in rare cases, could cause us
> to interpret the data payload as valid commands and do things
> not requested by the client. Fix by adding a complete flag to
> track whether we are in sync or must disconnect.
> 
> - If we report an error to NBD_CMD_READ, we are not writing out
> any data payload; but the protocol says that a client can expect
> to read the payload no matter what (and must instead ignore it),
> which means the client will start reading our next replies as
> its data payload. Fix by disconnecting (an alternative fix of
> sending bogus payload would be trickier to implement).
> 
> Furthermore, we have split the checks for bogus from/len across
> two functions, when it is easier to do it all at once.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/server.c | 67 +++++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 46 insertions(+), 21 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 53507c5..9ac7e01 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -52,6 +52,7 @@ struct NBDRequest {
>      QSIMPLEQ_ENTRY(NBDRequest) entry;
>      NBDClient *client;
>      uint8_t *data;
> +    bool complete;
>  };
> 
>  struct NBDExport {
> @@ -989,7 +990,13 @@ static ssize_t nbd_co_send_reply(NBDRequest *req, struct nbd_reply *reply,
>      return rc;
>  }
> 
> -static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *request)
> +/* Collect a client request.  Return 0 if request looks valid, -EAGAIN
> + * to keep trying the collection, -EIO to drop connection right away,
> + * and any other negative value to report an error to the client
> + * (although the caller may still need to disconnect after reporting
> + * the error).  */
> +static ssize_t nbd_co_receive_request(NBDRequest *req,
> +                                      struct nbd_request *request)
>  {
>      NBDClient *client = req->client;
>      uint32_t command;
> @@ -1007,16 +1014,26 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
>          goto out;
>      }
> 
> -    if ((request->from + request->len) < request->from) {
> -        LOG("integer overflow detected! "
> -            "you're probably being attacked");
> -        rc = -EINVAL;
> -        goto out;
> -    }
> -
>      TRACE("Decoding type");
> 
>      command = request->type & NBD_CMD_MASK_COMMAND;
> +    if (command == NBD_CMD_DISC) {
> +        /* Special case: we're going to disconnect without a reply,
> +         * whether or not flags, from, or len are bogus */
> +        TRACE("Request type is DISCONNECT");
> +        rc = -EIO;
> +        goto out;
> +    }
> +
> +    /* Check for sanity in the parameters, part 1.  Defer as many
> +     * checks as possible until after reading any NBD_CMD_WRITE
> +     * payload, so we can try and keep the connection alive.  */
> +    if ((request->from + request->len) < request->from) {
> +        LOG("integer overflow detected, you're probably being attacked");
> +        rc = -EINVAL;
> +        goto out;
> +    }
> +
>      if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
>          if (request->len > NBD_MAX_BUFFER_SIZE) {
>              LOG("len (%" PRIu32" ) is larger than max len (%u)",
> @@ -1039,7 +1056,18 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
>              rc = -EIO;
>              goto out;
>          }
> +        req->complete = true;
>      }
> +
> +    /* Sanity checks, part 2. */
> +    if (request->from + request->len > client->exp->size) {
> +        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
> +            ", Size: %" PRIu64, request->from, request->len,
> +            (uint64_t)client->exp->size);
> +        rc = -EINVAL;

For writes, this should be ENOSPC according to the spec.

> +        goto out;
> +    }
> +
>      rc = 0;
> 
>  out:
> @@ -1082,14 +1110,6 @@ static void nbd_trip(void *opaque)
>          goto error_reply;
>      }
>      command = request.type & NBD_CMD_MASK_COMMAND;
> -    if (command != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
> -            LOG("From: %" PRIu64 ", Len: %" PRIu32", Size: %" PRIu64
> -                ", Offset: %" PRIu64 "\n",
> -                request.from, request.len,
> -                (uint64_t)exp->size, (uint64_t)exp->dev_offset);
> -        LOG("requested operation past EOF--bad client?");
> -        goto invalid_request;
> -    }
> 
>      if (client->closing) {
>          /*
> @@ -1151,10 +1171,11 @@ static void nbd_trip(void *opaque)
>              goto out;
>          }
>          break;
> +
>      case NBD_CMD_DISC:
> -        TRACE("Request type is DISCONNECT");
> -        errno = 0;
> -        goto out;
> +        /* unreachable, thanks to special case in nbd_co_receive_request() */
> +        abort();
> +
>      case NBD_CMD_FLUSH:
>          TRACE("Request type is FLUSH");
> 
> @@ -1182,10 +1203,14 @@ static void nbd_trip(void *opaque)
>          break;
>      default:
>          LOG("invalid request type (%" PRIu32 ") received", request.type);
> -    invalid_request:
>          reply.error = EINVAL;
>      error_reply:
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> +        /* We must disconnect after replying with an error to
> +         * NBD_CMD_READ, since we choose not to send bogus filler
> +         * data; likewise after NBD_CMD_WRITE if we did not read the
> +         * payload. */
> +        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ ||
> +            (command == NBD_CMD_WRITE && !req->complete)) {

It's simpler to always set req->complete.  Putting everything together:

diff --git a/nbd/server.c b/nbd/server.c
index 4743316..73505dc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1017,6 +1017,11 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
     TRACE("Decoding type");
 
     command = request->type & NBD_CMD_MASK_COMMAND;
+    if (command != NBD_CMD_WRITE) {
+        /* No payload, we are ready to read the next request.  */
+        req->complete = true;
+    }
+
     if (command == NBD_CMD_DISC) {
         /* Special case: we're going to disconnect without a reply,
          * whether or not flags, from, or len are bogus */
@@ -1064,7 +1069,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
         LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
             ", Size: %" PRIu64, request->from, request->len,
             (uint64_t)client->exp->size);
-        rc = -EINVAL;
+        rc = command == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
         goto out;
     }
 
@@ -1213,12 +1218,9 @@ static void nbd_trip(void *opaque)
         LOG("invalid request type (%" PRIu32 ") received", request.type);
         reply.error = EINVAL;
     error_reply:
-        /* We must disconnect after replying with an error to
-         * NBD_CMD_READ, since we choose not to send bogus filler
-         * data; likewise after NBD_CMD_WRITE if we did not read the
-         * payload. */
-        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ ||
-            (command == NBD_CMD_WRITE && !req->complete)) {
+        /* We must disconnect after NBD_CMD_WRITE if we did not
+         * read the payload. */
+        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete)) {
             goto out;
         }
         break;

Paolo

Re: [Qemu-devel] [PATCH v4 02/11] nbd: More debug typo fixes, use correct formats

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

On 06/13/2016 06:04 AM, Paolo Bonzini wrote:
> 
> 
> On 12/05/2016 00:39, Eric Blake wrote:
>> Clean up some debug message oddities missed earlier; this includes
>> some typos, and recognizing that %d is not necessarily compatible
>> with uint32_t.
> 
> Actually it should be on any POSIX platform, since (by way of the
> requirements on limits.h) POSIX requires sizeof(int) to be >= 4.

Not quite true.  On 32-bit platforms, uint32_t can be 'long' rather than
'int' (I think 32-bit cygwin used to be in this camp, once - I don't
remember if it is still the case).  Thus, PRId32 is NOT necessarily "d"
on all platforms.

> 
> I will apply the patch, but fully expect this to bitrot...
> 
> Paolo
> 
>> Also add a couple messages that I found useful
>> while debugging things.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

[adding nbd list]

On 06/13/2016 06:10 AM, Paolo Bonzini wrote:
> 
> 
> On 12/05/2016 00:39, Eric Blake wrote:
>> - If we report an error to NBD_CMD_READ, we are not writing out
>> any data payload; but the protocol says that a client can expect
>> to read the payload no matter what (and must instead ignore it),
>> which means the client will start reading our next replies as
>> its data payload. Fix by disconnecting (an alternative fix of
>> sending bogus payload would be trickier to implement).
> 
> This is an error in the spec.  The Linux driver doesn't expect to read
> the payload here, and neither does block/nbd-client.c.

That's one of the reasons that there is a proposal to add
STRUCTURED_READ to the spec (although I still haven't had time to
implement that for qemu), so that we have a newer approach that allows
for proper error handling without ambiguity on whether bogus bytes must
be sent on a failed read.  But you'd have to convince me that ALL
existing NBD server and client implementations expect to handle a read
error without read payload, otherwise, I will stick with the notion that
the current spec wording is correct, and that read errors CANNOT be
gracefully recovered from unless BOTH sides transfer (possibly bogus)
bytes along with the error message, and which is why BOTH sides of the
protocol are warned that read errors usually result in a disconnection
rather than clean continuation, without the addition of STRUCTURED_READ.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 00/11] nbd: tighter protocol compliance

[Paolo Bonzini]

On 12/05/2016 00:39, Eric Blake wrote:
> Fix several corner-case bugs in our implementation of the NBD
> protocol, both as client and as server.
> 
> Depends on Kevin's block-next branch:
> git://repo.or.cz/qemu/kevin.git block-next
> 
> Also available as a tag at this location:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-flags-v4
> 
> Broken out of a larger v3 series[1], for easier review.  There
> are still some places where we aren't quite compliant (for example,
> the protocol recommends that the client send NBD_OPT_ABORT before
> dropping the connection after receiving a valid server response it
> didn't like but which did not violate protocol), but later series
> will tackle that.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03526.html
> 
> Changes in v4: rebase to latest block-next
> 
> 001/11:[----] [--] 'nbd: Use BDRV_REQ_FUA for better FUA where supported'

Applied.

> 002/11:[0004] [FC] 'nbd: More debug typo fixes, use correct formats'

Applied.

> 003/11:[----] [--] 'nbd: Quit server after any write error'

Applied.

> 004/11:[----] [--] 'nbd: Improve server handling of bogus commands'

Applied with some changes, see reply to individual patch.

> 005/11:[----] [--] 'nbd: Reject unknown request flags'

Applied.

> 006/11:[----] [--] 'nbd: Group all Linux-specific ioctl code in one place'

Applied.

> 007/11:[----] [--] 'nbd: Clean up ioctl handling of qemu-nbd -c'

Applied.

> 008/11:[----] [-C] 'nbd: Limit nbdflags to 16 bits'

Doesn't apply anymore.

> 009/11:[----] [--] 'nbd: Add qemu-nbd -D for human-readable description'

Doesn't apply anymore.

> 010/11:[----] [--] 'nbd: Detect servers that send unexpected error values'

Applied.

> 011/11:[----] [--] 'nbd: Avoid magic number for NBD max name size'

Applied.

I'll need a v5 of just patch 8 and patch 9; I'm queuing the rest and
will send a pull request later this week.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v4 00/11] nbd: tighter protocol compliance

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On 06/13/2016 10:28 AM, Paolo Bonzini wrote:

> 
>> 004/11:[----] [--] 'nbd: Improve server handling of bogus commands'
> 
> Applied with some changes, see reply to individual patch.

Not sure I agree with all of your comments on that patch regarding
behavior on read errors, but further changes can be followup patches.


> I'll need a v5 of just patch 8 and patch 9; I'm queuing the rest and
> will send a pull request later this week.

Do you have a git tree that I can rebase on top of?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2189 bytes --]

On 06/13/2016 06:19 AM, Paolo Bonzini wrote:

>> +    /* Sanity checks, part 2. */
>> +    if (request->from + request->len > client->exp->size) {
>> +        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
>> +            ", Size: %" PRIu64, request->from, request->len,
>> +            (uint64_t)client->exp->size);
>> +        rc = -EINVAL;
> 
> For writes, this should be ENOSPC according to the spec.

Good call.


>> +        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ ||
>> +            (command == NBD_CMD_WRITE && !req->complete)) {
> 
> It's simpler to always set req->complete.  Putting everything together:
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 4743316..73505dc 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1017,6 +1017,11 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
>      TRACE("Decoding type");
>  
>      command = request->type & NBD_CMD_MASK_COMMAND;
> +    if (command != NBD_CMD_WRITE) {
> +        /* No payload, we are ready to read the next request.  */
> +        req->complete = true;
> +    }
> +

Nice.

> @@ -1213,12 +1218,9 @@ static void nbd_trip(void *opaque)
>          LOG("invalid request type (%" PRIu32 ") received", request.type);
>          reply.error = EINVAL;
>      error_reply:
> -        /* We must disconnect after replying with an error to
> -         * NBD_CMD_READ, since we choose not to send bogus filler
> -         * data; likewise after NBD_CMD_WRITE if we did not read the
> -         * payload. */
> -        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ ||
> -            (command == NBD_CMD_WRITE && !req->complete)) {
> +        /* We must disconnect after NBD_CMD_WRITE if we did not
> +         * read the payload. */
> +        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete)) {

I'm not sure I agree with your change on NBD_CMD_READ, but we can hash
that out with upstream NBD list on the correct protocol, and make any
further changes as a followup.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Alex Bligh]

[-- Attachment #1: Type: text/plain, Size: 2424 bytes --]


On 13 Jun 2016, at 13:25, Eric Blake <eblake@redhat.com> wrote:

> On 06/13/2016 06:10 AM, Paolo Bonzini wrote:
>> 
>> 
>> On 12/05/2016 00:39, Eric Blake wrote:
>>> - If we report an error to NBD_CMD_READ, we are not writing out
>>> any data payload; but the protocol says that a client can expect
>>> to read the payload no matter what (and must instead ignore it),
>>> which means the client will start reading our next replies as
>>> its data payload. Fix by disconnecting (an alternative fix of
>>> sending bogus payload would be trickier to implement).
>> 
>> This is an error in the spec.  The Linux driver doesn't expect to read
>> the payload here, and neither does block/nbd-client.c.
> 
> That's one of the reasons that there is a proposal to add
> STRUCTURED_READ to the spec (although I still haven't had time to
> implement that for qemu), so that we have a newer approach that allows
> for proper error handling without ambiguity on whether bogus bytes must
> be sent on a failed read.  But you'd have to convince me that ALL
> existing NBD server and client implementations expect to handle a read
> error without read payload, otherwise, I will stick with the notion that
> the current spec wording is correct, and that read errors CANNOT be
> gracefully recovered from unless BOTH sides transfer (possibly bogus)
> bytes along with the error message, and which is why BOTH sides of the
> protocol are warned that read errors usually result in a disconnection
> rather than clean continuation, without the addition of STRUCTURED_READ.

To back up what Eric said:

Unfortunately the design is pretty much broken for reporting errors
on reads (at least in part as there is no way to signal errors that
occur after some of the reply has been written).

The spec specifies that on a read, no matter whether or not there
is an error, the data is all sent. This was after some mailing
list conversations on the subject which indicated this was the
least broken way to do things (IIRC).

This is actually what nbd-server.c does in the threaded handler:
 https://github.com/yoe/nbd/blob/master/nbd-server.c#L1468

For amusement value, the non-threaded handler (which is not used
any more) does not send any payload on an error:
 https://github.com/yoe/nbd/blob/master/nbd-server.c#L1734

In essence read error handling is a horrible mess in NBD, and
I would not expect it to work in general :-(

--
Alex Bligh





[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 13/06/2016 23:41, Alex Bligh wrote:
> That's one of the reasons that there is a proposal to add
> STRUCTURED_READ to the spec (although I still haven't had time to
> implement that for qemu), so that we have a newer approach that allows
> for proper error handling without ambiguity on whether bogus bytes must
> be sent on a failed read.  But you'd have to convince me that ALL
> existing NBD server and client implementations expect to handle a read
> error without read payload, otherwise, I will stick with the notion that
> the current spec wording is correct, and that read errors CANNOT be
> gracefully recovered from unless BOTH sides transfer (possibly bogus)
> bytes along with the error message, and which is why BOTH sides of the
> protocol are warned that read errors usually result in a disconnection
> rather than clean continuation, without the addition of STRUCTURED_READ.

I suspect that there are exactly two client implementations, namely
Linux and QEMU's, and both do the right thing.

What servers do doesn't matter, if all the clients agree.

Paolo

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Alex Bligh]

On 14 Jun 2016, at 14:32, Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> On 13/06/2016 23:41, Alex Bligh wrote:
>> That's one of the reasons that there is a proposal to add
>> STRUCTURED_READ to the spec (although I still haven't had time to
>> implement that for qemu), so that we have a newer approach that allows
>> for proper error handling without ambiguity on whether bogus bytes must
>> be sent on a failed read.  But you'd have to convince me that ALL
>> existing NBD server and client implementations expect to handle a read
>> error without read payload, otherwise, I will stick with the notion that
>> the current spec wording is correct, and that read errors CANNOT be
>> gracefully recovered from unless BOTH sides transfer (possibly bogus)
>> bytes along with the error message, and which is why BOTH sides of the
>> protocol are warned that read errors usually result in a disconnection
>> rather than clean continuation, without the addition of STRUCTURED_READ.
> 
> I suspect that there are exactly two client implementations,

My understanding is that there are more than 2 client implementations.
A quick google found at least one BSD client. I bet read error handling
is a mess in all of them.

> namely
> Linux and QEMU's, and both do the right thing.

This depends what you mean by 'right'. Both appear to be non-compliant
with the standard.

Note the standard is not defined by the client implementation, but
by the protocol document.

IMHO the 'right thing' is what is in the spec. Servers can't send an
error in any other way if they don't buffer the entire read first, as the
read may error towards the end.

To illustrate the problem, look consider what qemu itself would do as
a server if it can't buffer the entire read issued to it.

> What servers do doesn't matter, if all the clients agree.

The spec originally was not clear on how errors on reads should be
handled, leading to any read causing a protocol drop. The spec is
now clear. Unfortunately it is not possible to make a back compatible
fix. Hence the real fix here is to implement structured replies,
which is what Eric and I have been working on.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 14/06/2016 17:02, Alex Bligh wrote:
> 
> On 14 Jun 2016, at 14:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>>
>> On 13/06/2016 23:41, Alex Bligh wrote:
>>> That's one of the reasons that there is a proposal to add
>>> STRUCTURED_READ to the spec (although I still haven't had time to
>>> implement that for qemu), so that we have a newer approach that allows
>>> for proper error handling without ambiguity on whether bogus bytes must
>>> be sent on a failed read.  But you'd have to convince me that ALL
>>> existing NBD server and client implementations expect to handle a read
>>> error without read payload, otherwise, I will stick with the notion that
>>> the current spec wording is correct, and that read errors CANNOT be
>>> gracefully recovered from unless BOTH sides transfer (possibly bogus)
>>> bytes along with the error message, and which is why BOTH sides of the
>>> protocol are warned that read errors usually result in a disconnection
>>> rather than clean continuation, without the addition of STRUCTURED_READ.
>>
>> I suspect that there are exactly two client implementations,
> 
> My understanding is that there are more than 2 client implementations.
> A quick google found at least one BSD client. I bet read error handling
> is a mess in all of them.

Found it, it is exactly the same as Linux and QEMU:

https://github.com/bitrig/bitrig/blob/418985278/sys/dev/nbd.c#L577

>> namely
>> Linux and QEMU's, and both do the right thing.
> 
> This depends what you mean by 'right'. Both appear to be non-compliant
> with the standard.

I mean "what makes sense".

> Note the standard is not defined by the client implementation, but
> by the protocol document.
> 
> IMHO the 'right thing' is what is in the spec. Servers can't send an
> error in any other way if they don't buffer the entire read first, as the
> read may error towards the end.
> 
> To illustrate the problem, look consider what qemu itself would do as
> a server if it can't buffer the entire read issued to it.

Return ENOMEM?

> The spec originally was not clear on how errors on reads should be
> handled, leading to any read causing a protocol drop. The spec is
> now clear. Unfortunately it is not possible to make a back compatible
> fix. Hence the real fix here is to implement structured replies,
> which is what Eric and I have been working on.

I agree that structured replies are better.  However, it looks like the
de facto status prior to structured replies is that the error is in the
spec, and this patch introduces a regression.

Paolo

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Alex Bligh]

> On 14 Jun 2016, at 16:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> To illustrate the problem, look consider what qemu itself would do as
>> a server if it can't buffer the entire read issued to it.
> 
> Return ENOMEM?

Well OK, qemu then 'works' on the basis it breaks another
part of the spec, which is coping with long reads.

> However, it looks like the
> de facto status prior to structured replies is that the error is in the
> spec, and this patch introduces a regression.

Well, I guess the patch makes it work the same as the
reference server implementation and the spec, which I'd
consider a fix. My view is that the error is in the
kernel client. I think Erik CC'd in nbd-general
re the comment that the spec was broken; I don't think
it is, and don't propose to change it. Wouter might or
might not feel differently.

It's been reasonably well known (I wrote about it
at least 3 years ago), that the current implementation
(reference + kernel) does not cope well with errors
on reads, so I'm guessing one is just trading one
set of brokenness for another. So I'm pretty relaxed
about what goes in qemu.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v4 00/11] nbd: tighter protocol compliance

[Paolo Bonzini]

On 13/06/2016 18:49, Eric Blake wrote:
>>> >> 004/11:[----] [--] 'nbd: Improve server handling of bogus commands'
>> > 
>> > Applied with some changes, see reply to individual patch.
> Not sure I agree with all of your comments on that patch regarding
> behavior on read errors, but further changes can be followup patches.
> 
> 
>> > I'll need a v5 of just patch 8 and patch 9; I'm queuing the rest and
>> > will send a pull request later this week.
> Do you have a git tree that I can rebase on top of?

Just wait tomorrow. :)

Paolo

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

On 06/13/2016 06:19 AM, Paolo Bonzini wrote:
> 
> 
> On 12/05/2016 00:39, Eric Blake wrote:
>> We have a few bugs in how we handle invalid client commands:
>>
>> - A client can send an NBD_CMD_DISC where from + len overflows,
>> convincing us to reply with an error and stay connected, even
>> though the protocol requires us to silently disconnect. Fix by
>> hoisting the special case sooner.
>>

> It's simpler to always set req->complete.  Putting everything together:
> 
> diff --git a/nbd/server.c b/nbd/server.c

> @@ -1213,12 +1218,9 @@ static void nbd_trip(void *opaque)
>          LOG("invalid request type (%" PRIu32 ") received", request.type);
>          reply.error = EINVAL;
>      error_reply:
> -        /* We must disconnect after replying with an error to
> -         * NBD_CMD_READ, since we choose not to send bogus filler
> -         * data; likewise after NBD_CMD_WRITE if we did not read the
> -         * payload. */
> -        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ ||
> -            (command == NBD_CMD_WRITE && !req->complete)) {
> +        /* We must disconnect after NBD_CMD_WRITE if we did not
> +         * read the payload. */
> +        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete)) {

This doesn't even compile (too many ')').  I assume you'll fix that
before your actual pull request goes out.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 14/06/2016 17:59, Alex Bligh wrote:
> 
>> On 14 Jun 2016, at 16:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>> To illustrate the problem, look consider what qemu itself would do as
>>> a server if it can't buffer the entire read issued to it.
>>
>> Return ENOMEM?
> 
> Well OK, qemu then 'works' on the basis it breaks another
> part of the spec, which is coping with long reads.

ENOMEM is a documented error code, and the limits extension will help
with that as well.

>> However, it looks like the
>> de facto status prior to structured replies is that the error is in the
>> spec, and this patch introduces a regression.
> 
> Well, I guess the patch makes it work the same as the
> reference server implementation and the spec, which I'd
> consider a fix. My view is that the error is in the
> kernel client.

... and QEMU and BSD.  What good is a server that doesn't interoperate
(albeit only in error cases) with any client?

Paolo

Re: [Qemu-devel] [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Wouter Verhelst]

On Mon, Jun 13, 2016 at 10:41:05PM +0100, Alex Bligh wrote:
> For amusement value, the non-threaded handler (which is not used
> any more) does not send any payload on an error:
>  https://github.com/yoe/nbd/blob/master/nbd-server.c#L1734

nbd-server used to just drop the connection on read error.

> In essence read error handling is a horrible mess in NBD, and
> I would not expect it to work in general :-(

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Wouter Verhelst]

On Tue, Jun 14, 2016 at 04:02:15PM +0100, Alex Bligh wrote:
> 
> On 14 Jun 2016, at 14:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > 
> > On 13/06/2016 23:41, Alex Bligh wrote:
> >> That's one of the reasons that there is a proposal to add
> >> STRUCTURED_READ to the spec (although I still haven't had time to
> >> implement that for qemu), so that we have a newer approach that allows
> >> for proper error handling without ambiguity on whether bogus bytes must
> >> be sent on a failed read.  But you'd have to convince me that ALL
> >> existing NBD server and client implementations expect to handle a read
> >> error without read payload, otherwise, I will stick with the notion that
> >> the current spec wording is correct, and that read errors CANNOT be
> >> gracefully recovered from unless BOTH sides transfer (possibly bogus)
> >> bytes along with the error message, and which is why BOTH sides of the
> >> protocol are warned that read errors usually result in a disconnection
> >> rather than clean continuation, without the addition of STRUCTURED_READ.
> > 
> > I suspect that there are exactly two client implementations,
> 
> My understanding is that there are more than 2 client implementations.
> A quick google found at least one BSD client. I bet read error handling
> is a mess in all of them.
> 
> > namely
> > Linux and QEMU's, and both do the right thing.
> 
> This depends what you mean by 'right'. Both appear to be non-compliant
> with the standard.
> 
> Note the standard is not defined by the client implementation, but
> by the protocol document.

No, it isn't. At least not yet.

The standard document has only become formal a few months ago. It's
certainly possible that we made a mistake formalizing things, and if so,
we should fix the document rather than saying "whatever you've been
doing these years, even though it worked, is wrong".

There are more clients than the Linux and qemu ones, but I think it's
fair to say that those two are the most important ones. If they agree
that a read reply which errors should come without payload, then I think
we should update the standard to say that, too.

> > What servers do doesn't matter, if all the clients agree.
> 
> The spec originally was not clear on how errors on reads should be
> handled, leading to any read causing a protocol drop. The spec is
> now clear. Unfortunately it is not possible to make a back compatible
> fix. Hence the real fix here is to implement structured replies,
> which is what Eric and I have been working on.

That much, at any rate, is true.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Wouter Verhelst]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote:
> There are more clients than the Linux and qemu ones, but I think it's
> fair to say that those two are the most important ones. If they agree
> that a read reply which errors should come without payload, then I think
> we should update the standard to say that, too.

I've just pushed a commit that changes the spec (and the implementation)
so that if a server encounters a read error, it does not send a payload.

In other words, the current behaviour of qemu is correct, is now
documented to be correct, and should not be changed.

Regards,

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Alex Bligh]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]


> On 15 Jun 2016, at 09:03, Wouter Verhelst <w@uter.be> wrote:
> 
> On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote:
>> There are more clients than the Linux and qemu ones, but I think it's
>> fair to say that those two are the most important ones. If they agree
>> that a read reply which errors should come without payload, then I think
>> we should update the standard to say that, too.
> 
> I've just pushed a commit that changes the spec (and the implementation)
> so that if a server encounters a read error, it does not send a payload.
> 
> In other words, the current behaviour of qemu is correct, is now
> documented to be correct, and should not be changed.

So what should those servers do (like 2 of mine) which don't buffer
the entire read, if they get an error having already sent some data?

--
Alex Bligh





[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

Re: [Qemu-devel] [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

----- Original Message -----
> From: "Alex Bligh" <alex@alex.org.uk>
> To: "Wouter Verhelst" <w@uter.be>
> Cc: "Alex Bligh" <alex@alex.org.uk>, nbd-general@lists.sourceforge.net, "Paolo Bonzini" <pbonzini@redhat.com>,
> qemu-devel@nongnu.org, "qemu block" <qemu-block@nongnu.org>
> Sent: Wednesday, June 15, 2016 10:52:21 AM
> Subject: Re: [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands
> 
> 
> > On 15 Jun 2016, at 09:03, Wouter Verhelst <w@uter.be> wrote:
> > 
> > On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote:
> >> There are more clients than the Linux and qemu ones, but I think it's
> >> fair to say that those two are the most important ones. If they agree
> >> that a read reply which errors should come without payload, then I think
> >> we should update the standard to say that, too.
> > 
> > I've just pushed a commit that changes the spec (and the implementation)
> > so that if a server encounters a read error, it does not send a payload.
> > 
> > In other words, the current behaviour of qemu is correct, is now
> > documented to be correct, and should not be changed.
> 
> So what should those servers do (like 2 of mine) which don't buffer
> the entire read, if they get an error having already sent some data?

They have sent an error code of zero, and it turned out to be wrong.  So
the only thing they can do safely is disconnect.

Paolo

Re: [Qemu-devel] [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Alex Bligh]

On 15 Jun 2016, at 10:18, Paolo Bonzini <pbonzini@redhat.com> wrote:

>> So what should those servers do (like 2 of mine) which don't buffer
>> the entire read, if they get an error having already sent some data?
> 
> They have sent an error code of zero, and it turned out to be wrong.  So
> the only thing they can do safely is disconnect.

Right, but that is not what Wouter's change says:

+    If an error occurs, the server SHOULD set the appropriate error code
+    in the error field. The server MAY then initiate a hard disconnect.
+    If it chooses not to, it MUST NOT send any payload for this request.

I read this as either

a) the server can issue a hard disconnect without sending any reply; or

b) it must send the reply header with no payload

It also seems to permit not setting the error code (it's only a 'SHOULD'),
not disconnecting (it's a MAY), then not sending any payload, which is a
nonsense.

Perhaps this should read "If an error occurs, the server MUST either initiate
a hard disconnect before the entire payload has been sent or
set the appropriate code in the error field and send the response header
without any payload." if we want to go down this route.


-- 
Alex Bligh

Re: [Qemu-devel] [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 15/06/2016 12:27, Alex Bligh wrote:
> 
> On 15 Jun 2016, at 10:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>>> So what should those servers do (like 2 of mine) which don't buffer
>>> the entire read, if they get an error having already sent some data?
>>
>> They have sent an error code of zero, and it turned out to be wrong.  So
>> the only thing they can do safely is disconnect.
> 
> Right, but that is not what Wouter's change says:
> 
> +    If an error occurs, the server SHOULD set the appropriate error code
> +    in the error field. The server MAY then initiate a hard disconnect.
> +    If it chooses not to, it MUST NOT send any payload for this request.
> 
> I read this as either
> 
> a) the server can issue a hard disconnect without sending any reply; or
> 
> b) it must send the reply header with no payload
> 
> It also seems to permit not setting the error code (it's only a 'SHOULD'),
> not disconnecting (it's a MAY), then not sending any payload, which is a
> nonsense.

Right.

> Perhaps this should read "If an error occurs, the server MUST either initiate
> a hard disconnect before the entire payload has been sent or
> set the appropriate code in the error field and send the response header
> without any payload." if we want to go down this route.

Yes, I agree.

I do believe we want to go down this route.  I think we agree that
partial buffering may always require the server to disconnect after an
error.  Therefore I don't see any benefit at all in sending a payload
after an error message.

Paolo

Re: [Qemu-devel] [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Wouter Verhelst]

On Wed, Jun 15, 2016 at 11:27:21AM +0100, Alex Bligh wrote:
> Perhaps this should read "If an error occurs, the server MUST either initiate
> a hard disconnect before the entire payload has been sent or
> set the appropriate code in the error field and send the response header
> without any payload." if we want to go down this route.

Something along those lines is what I meant, yes.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12