[PATCH] block: fix bdrv_next() skipping monitor-owned nodes

["Denis V. Lunev" <den@openvz.org>]

From: Denis V. Lunev <den@openvz.org>

it->bs is the cursor into monitor_bdrv_states for the second phase of
bdrv_next(), so it has to be NULL when that phase starts. Commit
f6d38c9f6d made the first phase store the node it returns there, to
unreference the right one when the graph changes underneath. The cursor
is now left pointing at the last BlockBackend root, so the second phase
resumes from there instead of from the head of the list and never
returns a node added before it.

A skipped node drops out of the vm_stop and migration handover paths:
not flushed, not inactivated, not snapshotted. bdrv_inactivate_all()
still reports success, so the node keeps its image lock and the
migration target cannot open the image. Only detached nodes are hit in
practice, as these callers also recurse into children.

Reset the cursor when the second phase starts. old_bs is taken at the
top of the function, so f6d38c9f6d keeps working.

Fixes: f6d38c9f6d ("block-backend: fix edge case in bdrv_next() where BDS associated to BB changes")
Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Fiona Ebner <f.ebner@proxmox.com>
Cc: Kevin Wolf <kwolf@redhat.com>
---

Notes:
    The iterator was correct from its introduction in 2016 until 2024:
    it->bs was written only by the second phase, and the node to unreference
    came from the BlockBackend instead.

    f6d38c9f6d's own reproducer does not fire for me, 240 runs across 8
    timings, even with its fix reverted. Tried hard.

 block/block-backend.c                          |  1 +
 tests/qemu-iotests/tests/inactive-node-nbd     | 16 ++++++++++++++++
 tests/qemu-iotests/tests/inactive-node-nbd.out |  8 ++++++++
 3 files changed, 25 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 37ba7e9fc4..55497c4551 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -625,6 +625,7 @@ BlockDriverState *bdrv_next(BdrvNextIterator *it)
             return bs;
         }
         it->phase = BDRV_NEXT_MONITOR_OWNED;
+        it->bs = NULL;
     }
 
     /* Then return the monitor-owned BDSes without a BB attached. Ignore all
diff --git a/tests/qemu-iotests/tests/inactive-node-nbd b/tests/qemu-iotests/tests/inactive-node-nbd
index 664157bfd0..ef876ae6be 100755
--- a/tests/qemu-iotests/tests/inactive-node-nbd
+++ b/tests/qemu-iotests/tests/inactive-node-nbd
@@ -47,6 +47,7 @@ def node_is_active(_vm, node_name):
     return node['active']
 
 with iotests.FilePath('disk.img') as path, \
+     iotests.FilePath('detached.img') as detached_path, \
      iotests.FilePath('snap.qcow2') as snap_path, \
      iotests.FilePath('snap2.qcow2') as snap2_path, \
      iotests.FilePath('target.img') as target_path, \
@@ -58,6 +59,7 @@ with iotests.FilePath('disk.img') as path, \
     iotests.log('Preparing disk...')
     iotests.qemu_img_create('-f', iotests.imgfmt, path, img_size)
     iotests.qemu_img_create('-f', iotests.imgfmt, target_path, img_size)
+    iotests.qemu_img_create('-f', iotests.imgfmt, detached_path, img_size)
 
     iotests.qemu_img_create('-f', 'qcow2', '-b', path, '-F', iotests.imgfmt,
                             snap_path)
@@ -68,6 +70,9 @@ with iotests.FilePath('disk.img') as path, \
     vm.add_blockdev(f'file,node-name=disk-file,filename={path}')
     vm.add_blockdev(f'{iotests.imgfmt},file=disk-file,node-name=disk-fmt,'
                      'active=off')
+    vm.add_blockdev(f'file,node-name=detached-file,filename={detached_path}')
+    vm.add_blockdev(f'{iotests.imgfmt},file=detached-file,'
+                     'node-name=detached-fmt')
     vm.add_blockdev(f'file,node-name=target-file,filename={target_path}')
     vm.add_blockdev(f'{iotests.imgfmt},file=target-file,node-name=target-fmt')
     vm.add_blockdev(f'file,node-name=snap-file,filename={snap_path}')
@@ -297,6 +302,17 @@ with iotests.FilePath('disk.img') as path, \
     iotests.log('snap2-fmt active: %s' % node_is_active(vm, 'snap2-fmt'))
     iotests.log('target-fmt active: %s' % node_is_active(vm, 'target-fmt'))
 
+    iotests.log('\n=== Inactivating all nodes at once ===')
+
+    # detached-fmt has no parent and no BlockBackend, so nothing can reach it
+    # by recursion. It is only inactivated if bdrv_next() actually returns it.
+    vm.qmp_log('stop')
+    vm.qmp_log('blockdev-set-active', active=False)
+
+    iotests.log('detached-fmt active: %s' % node_is_active(vm, 'detached-fmt'))
+    iotests.log('detached-file active: %s'
+                % node_is_active(vm, 'detached-file'))
+
     iotests.log('\nShutting down...')
     vm.shutdown()
     log = vm.get_log()
diff --git a/tests/qemu-iotests/tests/inactive-node-nbd.out b/tests/qemu-iotests/tests/inactive-node-nbd.out
index a458b4fc05..48451dc0c5 100644
--- a/tests/qemu-iotests/tests/inactive-node-nbd.out
+++ b/tests/qemu-iotests/tests/inactive-node-nbd.out
@@ -235,5 +235,13 @@ snap-fmt active: True
 snap2-fmt active: True
 target-fmt active: True
 
+=== Inactivating all nodes at once ===
+{"execute": "stop", "arguments": {}}
+{"return": {}}
+{"execute": "blockdev-set-active", "arguments": {"active": false}}
+{"return": {}}
+detached-fmt active: False
+detached-file active: False
+
 Shutting down...
 
-- 
2.53.0