[Qemu-devel] [PATCH 0/3] NBD: flag handling cleanup

[Qemu-devel] [PATCH 0/3] NBD: flag handling cleanup

[Eric Blake]

I'm currently helping with work on adding extensions to the
upstream NBD protocol to allow more efficient handling of sparse
files, and wanted to get my feet wet in the qemu NBD code (both
client and server), to actually implement those proposals to see
how well they work.  In the process, I found a flaw where we
were silently ignoring bad client flags; the same flaw was just
recently patched in upstream NBD:
https://github.com/yoe/nbd/commit/ab22e0820

I didn't go quite as far as upstream (that is, I still silently
permit FLAG_FUA in combination with CMD_TRIM, which does not
have defined semantics upstream, and where our current
implementation might be too weak compared to the actual semantics
that upstream wants to propose, because we aren't actually
flushing in that case), but this at least gets rid of the
worst offenses and starts to document some anomolies that may
need later fixing.

Eric Blake (3):
  nbd: Treat flags vs. command type as separate fields
  nbd: Fix poor debug message
  nbd: Reject unknown request flags

 include/block/nbd.h | 17 +++++++++++------
 block/nbd-client.c  | 11 ++++-------
 nbd/client.c        | 19 +++++++++++--------
 nbd/server.c        | 32 ++++++++++++++++++++------------
 4 files changed, 46 insertions(+), 33 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/3] nbd: Treat flags vs. command type as separate fields

[Eric Blake]

Current upstream NBD documents that requests have a 16-bit flags,
followed by a 16-bit type integer; although older versions mentioned
only a 32-bit field with masking to find flags.  Since the protocol
is in network order (big-endian over the wire), the ABI is unchanged;
but dealing with the flags as a separate field rather than masking
will make it easier to add support for upcoming NBD extensions that
improve the efficiency of sparse file handling (since those
extensions will increase the number of both flags and commands).

Improve some comments in nbd.h based on the current upstream
NBD protocol (https://github.com/yoe/nbd/blob/master/doc/proto.md),
and touch some nearby code to keep checkpatch.pl happy.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h | 17 +++++++++++------
 block/nbd-client.c  | 11 ++++-------
 nbd/client.c        | 17 ++++++++++-------
 nbd/server.c        | 28 ++++++++++++++++------------
 4 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index b86a976..f018968 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -27,7 +27,8 @@

 struct nbd_request {
     uint32_t magic;
-    uint32_t type;
+    uint16_t flags;
+    uint16_t type;
     uint64_t handle;
     uint64_t from;
     uint32_t len;
@@ -39,6 +40,8 @@ struct nbd_reply {
     uint64_t handle;
 } QEMU_PACKED;

+/* Transmission (export) flags: sent from server to client during handshake,
+   but describe what will happen during transmission */
 #define NBD_FLAG_HAS_FLAGS      (1 << 0)        /* Flags are there */
 #define NBD_FLAG_READ_ONLY      (1 << 1)        /* Device is read-only */
 #define NBD_FLAG_SEND_FLUSH     (1 << 2)        /* Send FLUSH */
@@ -46,10 +49,12 @@ struct nbd_reply {
 #define NBD_FLAG_ROTATIONAL     (1 << 4)        /* Use elevator algorithm - rotational media */
 #define NBD_FLAG_SEND_TRIM      (1 << 5)        /* Send TRIM (discard) */

-/* New-style global flags. */
+/* New-style handshake (global) flags, sent from server to client, and
+   control what will happen during handshake phase. */
 #define NBD_FLAG_FIXED_NEWSTYLE     (1 << 0)    /* Fixed newstyle protocol. */

-/* New-style client flags. */
+/* New-style client flags, sent from client to server to control what happens
+   during handshake phase. */
 #define NBD_FLAG_C_FIXED_NEWSTYLE   (1 << 0)    /* Fixed newstyle protocol. */

 /* Reply types. */
@@ -60,10 +65,10 @@ struct nbd_reply {
 #define NBD_REP_ERR_INVALID     ((UINT32_C(1) << 31) | 3) /* Invalid length. */
 #define NBD_REP_ERR_TLS_REQD    ((UINT32_C(1) << 31) | 5) /* TLS required */

+/* Request flags, sent from client to server during transmission phase */
+#define NBD_CMD_FLAG_FUA        (1 << 0)

-#define NBD_CMD_MASK_COMMAND	0x0000ffff
-#define NBD_CMD_FLAG_FUA	(1 << 16)
-
+/* Supported request types */
 enum {
     NBD_CMD_READ = 0,
     NBD_CMD_WRITE = 1,
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 021a88b..8645be4 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -252,7 +252,7 @@ static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,

     if ((*flags & BDRV_REQ_FUA) && (client->nbdflags & NBD_FLAG_SEND_FUA)) {
         *flags &= ~BDRV_REQ_FUA;
-        request.type |= NBD_CMD_FLAG_FUA;
+        request.flags |= NBD_CMD_FLAG_FUA;
     }

     request.from = sector_num * 512;
@@ -319,8 +319,9 @@ int nbd_client_co_flush(BlockDriverState *bs)
         return 0;
     }

+    /* XXX: NBD Protocol only documents use of FUA with WRITE */
     if (client->nbdflags & NBD_FLAG_SEND_FUA) {
-        request.type |= NBD_CMD_FLAG_FUA;
+        request.flags |= NBD_CMD_FLAG_FUA;
     }

     request.from = 0;
@@ -380,11 +381,7 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
 void nbd_client_close(BlockDriverState *bs)
 {
     NbdClientSession *client = nbd_get_client_session(bs);
-    struct nbd_request request = {
-        .type = NBD_CMD_DISC,
-        .from = 0,
-        .len = 0
-    };
+    struct nbd_request request = { .type = NBD_CMD_DISC };

     if (client->ioc == NULL) {
         return;
diff --git a/nbd/client.c b/nbd/client.c
index f89c0a1..5af9f26 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -628,15 +628,18 @@ ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request)
     uint8_t buf[NBD_REQUEST_SIZE];
     ssize_t ret;

-    cpu_to_be32w((uint32_t*)buf, NBD_REQUEST_MAGIC);
-    cpu_to_be32w((uint32_t*)(buf + 4), request->type);
-    cpu_to_be64w((uint64_t*)(buf + 8), request->handle);
-    cpu_to_be64w((uint64_t*)(buf + 16), request->from);
-    cpu_to_be32w((uint32_t*)(buf + 24), request->len);
+    cpu_to_be32w((uint32_t *)buf, NBD_REQUEST_MAGIC);
+    cpu_to_be16w((uint16_t *)(buf + 4), request->flags);
+    cpu_to_be16w((uint16_t *)(buf + 6), request->type);
+    cpu_to_be64w((uint64_t *)(buf + 8), request->handle);
+    cpu_to_be64w((uint64_t *)(buf + 16), request->from);
+    cpu_to_be32w((uint32_t *)(buf + 24), request->len);

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

     ret = write_sync(ioc, buf, sizeof(buf));
     if (ret < 0) {
diff --git a/nbd/server.c b/nbd/server.c
index b95571b..a590773 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -625,21 +625,24 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, struct nbd_request *request)

     /* Request
        [ 0 ..  3]   magic   (NBD_REQUEST_MAGIC)
-       [ 4 ..  7]   type    (0 == READ, 1 == WRITE)
+       [ 4 ..  5]   flags   (NBD_CMD_FLAG_FUA, ...)
+       [ 6 ..  7]   type    (NBD_CMD_READ, ...)
        [ 8 .. 15]   handle
        [16 .. 23]   from
        [24 .. 27]   len
      */

-    magic = be32_to_cpup((uint32_t*)buf);
-    request->type  = be32_to_cpup((uint32_t*)(buf + 4));
-    request->handle = be64_to_cpup((uint64_t*)(buf + 8));
-    request->from  = be64_to_cpup((uint64_t*)(buf + 16));
-    request->len   = be32_to_cpup((uint32_t*)(buf + 24));
+    magic = be32_to_cpup((uint32_t *)buf);
+    request->flags = be16_to_cpup((uint16_t *)(buf + 4));
+    request->type  = be16_to_cpup((uint16_t *)(buf + 6));
+    request->handle = be64_to_cpup((uint64_t *)(buf + 8));
+    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 }",
-          magic, request->type, request->from, request->len);
+          "{ magic = 0x%x, .flags = %x, .type = %d, from = %" PRIu64
+          " , len = %u }",
+          magic, request->flags, request->type, request->from, request->len);

     if (magic != NBD_REQUEST_MAGIC) {
         LOG("invalid magic (got 0x%x)", magic);
@@ -980,7 +983,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque

     TRACE("Decoding type");

-    command = request->type & NBD_CMD_MASK_COMMAND;
+    command = request->type;
     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)",
@@ -1044,7 +1047,7 @@ static void nbd_trip(void *opaque)
         reply.error = -ret;
         goto error_reply;
     }
-    command = request.type & NBD_CMD_MASK_COMMAND;
+    command = request.type;
     if (command != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
             LOG("From: %" PRIu64 ", Len: %u, Size: %" PRIu64
             ", Offset: %" PRIu64 "\n",
@@ -1066,7 +1069,8 @@ static void nbd_trip(void *opaque)
     case NBD_CMD_READ:
         TRACE("Request type is READ");

-        if (request.type & NBD_CMD_FLAG_FUA) {
+        /* XXX: NBD Protocol only documents use of FUA with WRITE */
+        if (request.flags & NBD_CMD_FLAG_FUA) {
             ret = blk_co_flush(exp->blk);
             if (ret < 0) {
                 LOG("flush failed");
@@ -1108,7 +1112,7 @@ static void nbd_trip(void *opaque)
             goto error_reply;
         }

-        if (request.type & NBD_CMD_FLAG_FUA) {
+        if (request.flags & NBD_CMD_FLAG_FUA) {
             ret = blk_co_flush(exp->blk);
             if (ret < 0) {
                 LOG("flush failed");
-- 
2.5.5

[Qemu-devel] [PATCH 2/3] nbd: Fix poor debug message

[Eric Blake]

The client sends messages to the server, not itself.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/client.c b/nbd/client.c
index 5af9f26..d95ad7a 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -635,7 +635,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request)
     cpu_to_be64w((uint64_t *)(buf + 16), request->from);
     cpu_to_be32w((uint32_t *)(buf + 24), request->len);

-    TRACE("Sending request to client: "
+    TRACE("Sending request to server: "
           "{ .from = %" PRIu64", .len = %u, .handle = %" PRIu64
           ", .flags=%x, .type=%i}",
           request->from, request->len, request->handle, request->flags,
-- 
2.5.5

[Qemu-devel] [PATCH 3/3] 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>
---
 nbd/server.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index a590773..31bd9c5 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -974,6 +974,10 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
         goto out;
     }

+    if (request->flags & ~NBD_CMD_FLAG_FUA) {
+        LOG("unsupported flags (got 0x%x)", request->flags);
+        return -EINVAL;
+    }
     if ((request->from + request->len) < request->from) {
         LOG("integer overflow detected! "
             "you're probably being attacked");
-- 
2.5.5

Re: [Qemu-devel] [PATCH 2/3] nbd: Fix poor debug message

[Paolo Bonzini]

On 31/03/2016 23:20, Eric Blake wrote:
> The client sends messages to the server, not itself.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 5af9f26..d95ad7a 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -635,7 +635,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request)
>      cpu_to_be64w((uint64_t *)(buf + 16), request->from);
>      cpu_to_be32w((uint32_t *)(buf + 24), request->len);
> 
> -    TRACE("Sending request to client: "
> +    TRACE("Sending request to server: "
>            "{ .from = %" PRIu64", .len = %u, .handle = %" PRIu64
>            ", .flags=%x, .type=%i}",
>            request->from, request->len, request->handle, request->flags,
> 

(Rebased and) queued for 2.6.

Paolo

Re: [Qemu-devel] [PATCH 3/3] nbd: Reject unknown request flags

[Paolo Bonzini]

On 31/03/2016 23:20, Eric Blake wrote:
> 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>
> ---
>  nbd/server.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index a590773..31bd9c5 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -974,6 +974,10 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
>          goto out;
>      }
> 
> +    if (request->flags & ~NBD_CMD_FLAG_FUA) {
> +        LOG("unsupported flags (got 0x%x)", request->flags);
> +        return -EINVAL;
> +    }
>      if ((request->from + request->len) < request->from) {
>          LOG("integer overflow detected! "
>              "you're probably being attacked");
> 

Queued for 2.6.

Paolo