[PATCH 4/5] nbd/server: Avoid unaligned dirty-bitmap status

[Eric Blake]

The NBD spec requires that responses to NBD_CMD_BLOCK_STATUS be
aligned to the server's advertised min_block alignment, if the client
agreed to abide by alignments. In general, since dirty bitmap
granularity cannot go below 512, and it is hard to provoke qcow2 to go
above an alignment of 512, this is not an issue. But now that the
block layer can see through filters, it is possible to use blkdebug to
show a scenario where where the server is provoked into supplying a
non-compliant reply, as show in iotest 241.

Thus, it is time to fix the dirty bitmap code to round requests to
aligned boundaries.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c               | 30 ++++++++++++++++++++++++++----
 tests/qemu-iotests/241     |  5 ++---
 tests/qemu-iotests/241.out |  2 +-
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 40847276ca64..31462abaee63 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1,5 +1,5 @@
 /*
- *  Copyright (C) 2016-2020 Red Hat, Inc.
+ *  Copyright (C) 2016-2021 Red Hat, Inc.
  *  Copyright (C) 2005  Anthony Liguori <anthony@codemonkey.ws>
  *
  *  Network Block Device Server Side
@@ -2175,7 +2175,8 @@ static int nbd_co_send_block_status(NBDClient *client, 
uint64_t handle,
 }

 /* Populate @ea from a dirty bitmap. */
-static void bitmap_to_extents(BdrvDirtyBitmap *bitmap,
+static void bitmap_to_extents(uint32_t align,
+                              BdrvDirtyBitmap *bitmap,
                               uint64_t offset, uint64_t length,
                               NBDExtentArray *es)
 {
@@ -2186,10 +2187,23 @@ static void bitmap_to_extents(BdrvDirtyBitmap *bitmap,
     bdrv_dirty_bitmap_lock(bitmap);

     for (start = offset;
-         bdrv_dirty_bitmap_next_dirty_area(bitmap, start, end, INT32_MAX,
+         bdrv_dirty_bitmap_next_dirty_area(bitmap, start, end,
+                                           INT32_MAX - align + 1,
                                            &dirty_start, &dirty_count);
          start = dirty_start + dirty_count)
     {
+        /*
+         * Round unaligned bits: any transition mid-alignment makes
+         * that entire aligned region appear dirty.
+         */
+        assert(QEMU_IS_ALIGNED(start, align));
+        if (!QEMU_IS_ALIGNED(dirty_start, align)) {
+            dirty_count += dirty_start - QEMU_ALIGN_DOWN(dirty_start, align);
+            dirty_start = QEMU_ALIGN_DOWN(dirty_start, align);
+        }
+        if (!QEMU_IS_ALIGNED(dirty_count, align)) {
+            dirty_count = QEMU_ALIGN_UP(dirty_count, align);
+        }
         if ((nbd_extent_array_add(es, dirty_start - start, 0) < 0) ||
             (nbd_extent_array_add(es, dirty_count, NBD_STATE_DIRTY) < 0))
         {
@@ -2214,7 +2228,15 @@ static int nbd_co_send_bitmap(NBDClient *client, 
uint64_t handle,
     unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
     g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);

-    bitmap_to_extents(bitmap, offset, length, ea);
+    /* Easiest to just refuse to answer an unaligned query */
+    if (client->check_align &&
+        !QEMU_IS_ALIGNED(offset | length, client->check_align)) {
+        return nbd_co_send_structured_error(client, handle, -EINVAL,
+                                            "unaligned dirty bitmap request",
+                                            errp);
+    }
+
+    bitmap_to_extents(client->check_align ?: 1, bitmap, offset, length, ea);

     return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
 }
diff --git a/tests/qemu-iotests/241 b/tests/qemu-iotests/241
index 49e2bc09e5bc..b4d2e1934d66 100755
--- a/tests/qemu-iotests/241
+++ b/tests/qemu-iotests/241
@@ -124,9 +124,8 @@ nbd_server_start_unix_socket -B b0 -A --image-opts \

 TEST_IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
 $QEMU_NBD_PROG --list -k $nbd_unix_socket | grep '\(size\|min\)'
-# FIXME: this should report a single 4k block of "data":false which translates
-# to the dirty bitmap being set for at least part of the region; "data":true
-# is wrong unless the entire 4k is clean.
+# Reports a single 4k block of "data":false, meaning dirty.  Reporting
+# "data":true would be wrong (the entire region is not clean).
 $QEMU_IMG map --output=json --image-opts \
          "$TEST_IMG",x-dirty-bitmap=qemu:dirty-bitmap:b0 | _filter_qemu_img_map
 # Reports a single 4k block of "zero":true,"data":true, meaning allocated
diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
index 12a899ba9181..2368b71054d3 100644
--- a/tests/qemu-iotests/241.out
+++ b/tests/qemu-iotests/241.out
@@ -43,6 +43,6 @@ wrote 512/512 bytes at offset 512
 Formatting 'TEST_DIR/t.IMGFMT.qcow2', fmt=qcow2 size=4096 
backing_file=TEST_DIR/t.IMGFMT.mid backing_fmt=qcow2
   size:  4096
   min block: 4096
-[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true, 
"offset": OFFSET}]
+[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": false}]
 [{ "start": 0, "length": 4096, "depth": 0, "zero": true, "data": true, 
"offset": OFFSET}]
 *** done
-- 
2.30.1