Re: [Qemu-devel] [PATCH 2/2] qdev: recursively unrealize devices when unrealizing bus

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> When the patch was posted that became 5c21ce7 (qdev: Realize buses
> on device realization, 2014-03-12), it included recursive realization
> and unrealization of devices when the bus's "realized" property
> was toggled.
>
> However, due to the same old worries about recursive realization
> and prerequisites not being realized yet, those hunks were dropped when
> committing the patch.  Unfortunately, this causes a use-after-free bug
> (easily reproduced by a PCI hot-unplug action).
>
> Before the patch, device_unparent behaved as follows:
>
>    for each child bus
>      unparent bus ----------------------------.
>      | for each child device                  |
>      |   unparent device ---------------.     |
>      |   | unrealize device             |     |
>      |   | call dc->unparent            |     |
>      |   '-------------------------------     |
>      '----------------------------------------'
>    unrealize device
>
> After the patch, it behaves as follows instead:
>
>    unrealize device --------------------.
>    | for each child bus                 |
>    |   unrealize bus               (A)  |
>    '------------------------------------'
>    for each child bus
>      unparent bus ----------------------.
>      | for each child device            |
>      |   unrealize device          (B)  |
>      |   call dc->unparent              |
>      '----------------------------------'
>
> At the step marked (B) the device might use data from the bus that is
> not available anymore due to step (A).
>
> To fix this, we need to unrealize devices before step (A).  To sidestep
> concerns about recursive realization, only do recursive unrealization
> and leave the "value && !bus->realized" case as it is.
>
> The resulting flow is:
>
>    for each child bus
>      unrealize bus ---------------------.
>      | for each child device            |
>      |   unrealize device          (B)  |
>      | call bc->unrealize          (A)  |
>      '----------------------------------'
>    unrealize device
>    for each child bus
>      unparent bus ----------------------.
>      | for each child device            |
>      |   unparent device                |
>      '----------------------------------'
>
> where everything is "powered down" before it is unassembled.
>
> Cc: address@hidden
> Signed-off-by: Paolo Bonzini <address@hidden>

This broke tests/qemu-iotests/067.  First of four similar diff hunks:

--- tests/qemu-iotests/067.out  2014-06-06 13:51:21.231106345 +0200
+++ tests/qemu-iotests/067.out.bad      2014-06-26 13:11:36.826825145 +0200
@@ -9,7 +9,6 @@
 {"return": [{"io-status": "ok", "device": "disk", "locked": false, 
"removable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": 
{"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 
65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": 
"qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": 
false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", 
"iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": 
"TEST_DIR/t.qcow2", "encryption_key_missing": false}, "type": "unknown"}, 
{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, 
"tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, 
"removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", 
"locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"DEVICE_DELETED", "data": {"path": 
"/machine/peripheral/virtio0/virtio-backend"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"DEVICE_DELETED", "data": {"device": "virtio0", "path": 
"/machine/peripheral/virtio0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"RESET"}
 {"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, 
"removable": true, "tray_open": false, "type": "unknown"}, {"device": 
"floppy0", "locked": false, "removable": true, "tray_open": false, "type": 
"unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": 
false, "type": "unknown"}]}

The '{"return": {}}' immediately before the deleted line is the reply to
QMP command '{ "execute": "device_del", "arguments": { "id": "virtio0" }
}', which deletes a virtio-blk-pci device.

Before the patch, we get a DEVICE_DELETED event both for the
virtio-blk-pci device (second event) and its virtio-blk-device child
(first event).  The patch suppresses the event for the child.

Intentional?

If yes, I'll post the obvious patch to the expected test output.