[Qemu-devel] [PULL 0/4] 2.12 post-release NBD fixes, 2018-05-04

[Qemu-devel] [PULL 0/4] 2.12 post-release NBD fixes, 2018-05-04

[Eric Blake]

The following changes since commit 26bd8d98c4b3284a4c6fe3b67c98b1edd00e9beb:

  Merge remote-tracking branch 'remotes/vivier/tags/m68k-for-2.13-pull-request' into staging (2018-05-01 15:26:06 +0100)

are available in the Git repository at:

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

for you to fetch changes up to acfd8f7a5f92e703d2d046cbe3d510008a697194:

  nbd/client: Relax handling of large NBD_CMD_BLOCK_STATUS reply (2018-05-04 08:23:39 -0500)

Fix various issues with NBD in 2.12, some found by Coverity, and some by
implementation interoperability testing.

And May the Fourth be with you

----------------------------------------------------------------
nbd patches for 2018-05-04

- Vladimir Sementsov-Ogievskiy: 0/2 fix coverity bugs
- Eric Blake: nbd/client: Fix error messages during NBD_INFO_BLOCK_SIZE
- Eric Blake: nbd/client: Relax handling of large NBD_CMD_BLOCK_STATUS reply

----------------------------------------------------------------
Eric Blake (2):
      nbd/client: Fix error messages during NBD_INFO_BLOCK_SIZE
      nbd/client: Relax handling of large NBD_CMD_BLOCK_STATUS reply

Vladimir Sementsov-Ogievskiy (2):
      nbd/client: fix nbd_negotiate_simple_meta_context
      migration/block-dirty-bitmap: fix memory leak in dirty_bitmap_load_bits

 block/nbd-client.c             | 10 +++++++---
 migration/block-dirty-bitmap.c |  1 +
 nbd/client.c                   | 18 ++++++++++++------
 3 files changed, 20 insertions(+), 9 deletions(-)

Eric Blake (2):
  nbd/client: Fix error messages during NBD_INFO_BLOCK_SIZE
  nbd/client: Relax handling of large NBD_CMD_BLOCK_STATUS reply

Vladimir Sementsov-Ogievskiy (2):
  nbd/client: fix nbd_negotiate_simple_meta_context
  migration/block-dirty-bitmap: fix memory leak in
    dirty_bitmap_load_bits

 block/nbd-client.c             | 10 +++++++---
 migration/block-dirty-bitmap.c |  1 +
 nbd/client.c                   | 18 ++++++++++++------
 3 files changed, 20 insertions(+), 9 deletions(-)

-- 
2.14.3

[Qemu-devel] [PULL 1/4] nbd/client: fix nbd_negotiate_simple_meta_context

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Initialize received variable. Otherwise, is is possible for server to
answer without any contexts, but we will set context_id to something
random (received_id is not initialized too) and return 1, which is
wrong.

To solve it, just initialize received to false. Initialize received_id
too, just to make all possible checkers happy.

Bug was introduced in 78a33ab58782efdb206de14 "nbd: BLOCK_STATUS for
standard get_block_status function: client part" with the whole
function.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20180427142002.21930-2-vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index b9e175d1c27..7f35b5c3232 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -613,8 +613,8 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
 {
     int ret;
     NBDOptionReply reply;
-    uint32_t received_id;
-    bool received;
+    uint32_t received_id = 0;
+    bool received = false;
     uint32_t export_len = strlen(export);
     uint32_t context_len = strlen(context);
     uint32_t data_len = sizeof(export_len) + export_len +
-- 
2.14.3

[Qemu-devel] [PULL 2/4] migration/block-dirty-bitmap: fix memory leak in dirty_bitmap_load_bits

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Release buf on error path too.

Bug was introduced in b35ebdf076d697bc "migration: add postcopy
migration of dirty bitmaps" with the whole function.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20180427142002.21930-3-vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 migration/block-dirty-bitmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index dd04f102d8d..8819aabe3a0 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -600,6 +600,7 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
         ret = qemu_get_buffer(f, buf, buf_size);
         if (ret != buf_size) {
             error_report("Failed to read bitmap bits");
+            g_free(buf);
             return -EIO;
         }

-- 
2.14.3

[Qemu-devel] [PULL 3/4] nbd/client: Fix error messages during NBD_INFO_BLOCK_SIZE

[Eric Blake]

A missing space makes for poor error messages, and sizes can't
go negative.  Also, we missed diagnosing a server that sends
a maximum block size less than the minimum.

Fixes: 081dd1fe
CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180501154654.943782-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 7f35b5c3232..232ff4f46da 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -435,8 +435,8 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
             }
             be32_to_cpus(&info->min_block);
             if (!is_power_of_2(info->min_block)) {
-                error_setg(errp, "server minimum block size %" PRId32
-                           "is not a power of two", info->min_block);
+                error_setg(errp, "server minimum block size %" PRIu32
+                           " is not a power of two", info->min_block);
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
@@ -450,8 +450,8 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
             be32_to_cpus(&info->opt_block);
             if (!is_power_of_2(info->opt_block) ||
                 info->opt_block < info->min_block) {
-                error_setg(errp, "server preferred block size %" PRId32
-                           "is not valid", info->opt_block);
+                error_setg(errp, "server preferred block size %" PRIu32
+                           " is not valid", info->opt_block);
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
@@ -462,6 +462,12 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
                 return -1;
             }
             be32_to_cpus(&info->max_block);
+            if (info->max_block < info->min_block) {
+                error_setg(errp, "server maximum block size %" PRIu32
+                           " is not valid", info->max_block);
+                nbd_send_opt_abort(ioc);
+                return -1;
+            }
             trace_nbd_opt_go_info_block_size(info->min_block, info->opt_block,
                                              info->max_block);
             break;
-- 
2.14.3

[Qemu-devel] [PULL 4/4] nbd/client: Relax handling of large NBD_CMD_BLOCK_STATUS reply

[Eric Blake]

The NBD spec is proposing a relaxation of NBD_CMD_BLOCK_STATUS
where a server may have the final extent per context give a
length beyond the original request, if it can easily prove that
subsequent bytes have the same status, on the grounds that a
client can take advantage of this information for fewer block
status requests.  Since qemu 2.12 as a client always sends
NBD_CMD_FLAG_REQ_ONE, and rejects a server that sends extra
length, the upstream NBD spec will probably limit this behavior
to clients that don't request REQ_ONE semantics; but it doesn't
hurt to relax qemu to always be permissive of this server
behavior, even if it continues to use REQ_ONE.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180503222626.1303410-1-eblake@redhat.com>
Reviewed-by:  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index e7caf49fbb4..8d69eaaa32f 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -259,14 +259,18 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client,

     if (extent->length == 0 ||
         (client->info.min_block && !QEMU_IS_ALIGNED(extent->length,
-                                                    client->info.min_block)) ||
-        extent->length > orig_length)
-    {
+                                                    client->info.min_block))) {
         error_setg(errp, "Protocol error: server sent status chunk with "
                    "invalid length");
         return -EINVAL;
     }

+    /* The server is allowed to send us extra information on the final
+     * extent; just clamp it to the length we requested. */
+    if (extent->length > orig_length) {
+        extent->length = orig_length;
+    }
+
     return 0;
 }

-- 
2.14.3

Re: [Qemu-devel] [PULL 0/4] 2.12 post-release NBD fixes, 2018-05-04

[Peter Maydell]

On 4 May 2018 at 14:30, Eric Blake <eblake@redhat.com> wrote:
> The following changes since commit 26bd8d98c4b3284a4c6fe3b67c98b1edd00e9beb:
>
>   Merge remote-tracking branch 'remotes/vivier/tags/m68k-for-2.13-pull-request' into staging (2018-05-01 15:26:06 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2018-05-04
>
> for you to fetch changes up to acfd8f7a5f92e703d2d046cbe3d510008a697194:
>
>   nbd/client: Relax handling of large NBD_CMD_BLOCK_STATUS reply (2018-05-04 08:23:39 -0500)
>
> Fix various issues with NBD in 2.12, some found by Coverity, and some by
> implementation interoperability testing.
>
> And May the Fourth be with you

Applied, but not with git push --force :-)

thanks
-- PMM