[PULL 3/6] block/monitor: reject persistent bitmap add on a read-only node

From: "Denis V. Lunev" <[email protected]>

qmp_block_dirty_bitmap_add() marks a new bitmap persistent without
checking write access to its node. bdrv_create_dirty_bitmap() always
creates bitmaps writable, so a persistent bitmap added to an
already read-only node stays writable in memory on a node that can
never store it, and the next global inactivation fails:

  Lost persistent bitmaps during inactivation of node '<node>': No write access
  migration_block_inactivate: bdrv_inactivate_all() failed: -22

Forcing it read-only instead does not help: it was never stored,
so it stays unpromotable on the next reopen to read-write and can
trip bdrv_set_dirty()'s readonly assert on the first write. Reject
the add instead, for both read-only and inactive nodes -- an
already-inactive node skips qcow2_inactivate() on close, so a
bitmap added during that window would never get stored either.
Wrapped in a transaction, this denies the whole transaction, since
qmp_transaction() is already all-or-none.

Signed-off-by: Denis V. Lunev <[email protected]>
CC: Eric Blake <[email protected]>
CC: Vladimir Sementsov-Ogievskiy <[email protected]>
CC: John Snow <[email protected]>
CC: Andrey Drobyshev <[email protected]>
Message-ID: <[email protected]>
Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
---
 block/monitor/bitmap-qmp-cmds.c               | 15 +++++++---
 qapi/block-core.json                          |  4 ++-
 .../tests/remove-bitmap-from-backing          | 29 ++++++++++++++++++-
 .../tests/remove-bitmap-from-backing.out      | 20 +++++++++++++
 4 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index a738e7bbf7a..d87ca982aa9 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -125,10 +125,17 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         disabled = false;
     }
 
-    if (persistent &&
-        !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
-    {
-        return;
+    if (persistent) {
+        if (!bdrv_is_writable(bs)) {
+            error_setg(errp, "Cannot add a persistent bitmap to "
+                       "read-only or inactive node '%s'",
+                       bdrv_get_node_name(bs));
+            return;
+        }
+
+        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
+            return;
+        }
     }
 
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1f87b078505..199efc1e008 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2353,7 +2353,9 @@
 # @persistent: the bitmap is persistent, i.e. it will be saved to the
 #     corresponding block device image file on its close.  For now
 #     only Qcow2 disks support persistent bitmaps.  Default is false
-#     for `block-dirty-bitmap-add`.  (Since: 2.10)
+#     for `block-dirty-bitmap-add`.  This fails if the node is
+#     read-only or inactive, since such a bitmap could never be
+#     stored.  (Since: 2.10)
 #
 # @disabled: the bitmap is created in the disabled state, which means
 #     that it will not track drive changes.  The bitmap may be enabled
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing b/tests/qemu-iotests/tests/remove-bitmap-from-backing
index 15be32dcb96..a54984fa58e 100755
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -35,7 +35,7 @@ qemu_img('bitmap', '--add', base, 'bitmap0')
 # Just assert that our method of checking bitmaps in the image works.
 assert 'bitmaps' in qemu_img_info(base)['format-specific']['data']
 
-vm = iotests.VM().add_drive(top, 'backing.node-name=base')
+vm = iotests.VM().add_drive(top, 'node-name=top,backing.node-name=base')
 vm.launch()
 
 log('Trying to remove persistent bitmap from r-o base node, should fail:')
@@ -66,6 +66,33 @@ result = vm.qmp('blockdev-reopen', **new_base_opts)
 if result != {'return': {}}:
     log('Failed to reopen: ' + str(result))
 
+log('Adding a persistent bitmap to the r-o base node, should fail:')
+vm.qmp_log('block-dirty-bitmap-add', node='base', name='bitmap1',
+          persistent=True)
+
+log('Same add inside a transaction, preceded by an otherwise valid')
+log('action: the whole transaction must fail and roll back the')
+log('already-succeeded first action too:')
+vm.qmp_log('transaction', actions=[
+    {'type': 'block-dirty-bitmap-add',
+     'data': {'node': 'top', 'name': 'bitmap2', 'persistent': True}},
+    {'type': 'block-dirty-bitmap-add',
+     'data': {'node': 'base', 'name': 'bitmap1', 'persistent': True}},
+])
+
+log('bitmap2 on the rw top node must not have survived the rollback:')
+vm.qmp_log('block-dirty-bitmap-remove', node='top', name='bitmap2')
+
+log('Marking the rw top node inactive:')
+vm.qmp_log('blockdev-set-active', **{'node-name': 'top', 'active': False})
+
+log('Adding a persistent bitmap to a rw but inactive node, should fail:')
+vm.qmp_log('block-dirty-bitmap-add', node='top', name='bitmap3',
+          persistent=True)
+
+log('Reactivating the top node:')
+vm.qmp_log('blockdev-set-active', **{'node-name': 'top', 'active': True})
+
 vm.shutdown()
 
 if 'bitmaps' in qemu_img_info(base)['format-specific']['data']:
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing.out b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
index c28af82c752..fe105fe0a38 100644
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
@@ -4,3 +4,23 @@ Trying to remove persistent bitmap from r-o base node, should fail:
 Remove persistent bitmap from base node reopened to RW:
 {"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "base"}}
 {"return": {}}
+Adding a persistent bitmap to the r-o base node, should fail:
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap1", "node": "base", "persistent": true}}
+{"error": {"class": "GenericError", "desc": "Cannot add a persistent bitmap to read-only or inactive node 'base'"}}
+Same add inside a transaction, preceded by an otherwise valid
+action: the whole transaction must fail and roll back the
+already-succeeded first action too:
+{"execute": "transaction", "arguments": {"actions": [{"data": {"name": "bitmap2", "node": "top", "persistent": true}, "type": "block-dirty-bitmap-add"}, {"data": {"name": "bitmap1", "node": "base", "persistent": true}, "type": "block-dirty-bitmap-add"}]}}
+{"error": {"class": "GenericError", "desc": "Cannot add a persistent bitmap to read-only or inactive node 'base'"}}
+bitmap2 on the rw top node must not have survived the rollback:
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap2", "node": "top"}}
+{"error": {"class": "GenericError", "desc": "Dirty bitmap 'bitmap2' not found"}}
+Marking the rw top node inactive:
+{"execute": "blockdev-set-active", "arguments": {"active": false, "node-name": "top"}}
+{"return": {}}
+Adding a persistent bitmap to a rw but inactive node, should fail:
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap3", "node": "top", "persistent": true}}
+{"error": {"class": "GenericError", "desc": "Cannot add a persistent bitmap to read-only or inactive node 'top'"}}
+Reactivating the top node:
+{"execute": "blockdev-set-active", "arguments": {"active": true, "node-name": "top"}}
+{"return": {}}
-- 
2.43.0