[Qemu-devel] [PATCH 0/2] qdev: fix pci use-after-free

[Qemu-devel] [PATCH 0/2] qdev: fix pci use-after-free

[Paolo Bonzini]

See "Use-after-free during unrealize in system_reset" thread
and individual patches.

Paolo

Paolo Bonzini (2):
  qdev: reorganize error reporting in bus_set_realized
  qdev: recursively unrealize devices when unrealizing bus

 hw/core/qdev.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] qdev: reorganize error reporting in bus_set_realized

[Paolo Bonzini]

No semantic change.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/qdev.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e65a5aa..5efa251 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -572,27 +572,19 @@ static void bus_set_realized(Object *obj, bool value, Error **errp)
     if (value && !bus->realized) {
         if (bc->realize) {
             bc->realize(bus, &local_err);
-
-            if (local_err != NULL) {
-                goto error;
-            }
-
         }
     } else if (!value && bus->realized) {
         if (bc->unrealize) {
             bc->unrealize(bus, &local_err);
-
-            if (local_err != NULL) {
-                goto error;
-            }
         }
     }
 
-    bus->realized = value;
-    return;
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
-error:
-    error_propagate(errp, local_err);
+    bus->realized = value;
 }
 
 void qbus_create_inplace(void *bus, size_t size, const char *typename,
-- 
1.8.3.1

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

[Paolo Bonzini]

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: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/qdev.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5efa251..4282491 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -567,14 +567,25 @@ static void bus_set_realized(Object *obj, bool value, Error **errp)
 {
     BusState *bus = BUS(obj);
     BusClass *bc = BUS_GET_CLASS(bus);
+    BusChild *kid;
     Error *local_err = NULL;
 
     if (value && !bus->realized) {
         if (bc->realize) {
             bc->realize(bus, &local_err);
         }
+
+        /* TODO: recursive realization */
     } else if (!value && bus->realized) {
-        if (bc->unrealize) {
+        QTAILQ_FOREACH(kid, &bus->children, sibling) {
+            DeviceState *dev = kid->child;
+            object_property_set_bool(OBJECT(dev), false, "realized",
+                                     &local_err);
+            if (local_err != NULL) {
+                break;
+            }
+        }
+        if (bc->unrealize && local_err == NULL) {
             bc->unrealize(bus, &local_err);
         }
     }
-- 
1.8.3.1

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

[Michael S. Tsirkin]

On Wed, Jun 11, 2014 at 02:52:09PM +0200, Paolo Bonzini wrote:
> 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.

functionality-wise, patch looks good to me.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

object_unparent is called in many places, we really
should have some documentation for this API.

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/qdev.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5efa251..4282491 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -567,14 +567,25 @@ static void bus_set_realized(Object *obj, bool value, Error **errp)
>  {
>      BusState *bus = BUS(obj);
>      BusClass *bc = BUS_GET_CLASS(bus);
> +    BusChild *kid;
>      Error *local_err = NULL;
>  
>      if (value && !bus->realized) {
>          if (bc->realize) {
>              bc->realize(bus, &local_err);
>          }
> +
> +        /* TODO: recursive realization */
>      } else if (!value && bus->realized) {
> -        if (bc->unrealize) {
> +        QTAILQ_FOREACH(kid, &bus->children, sibling) {
> +            DeviceState *dev = kid->child;
> +            object_property_set_bool(OBJECT(dev), false, "realized",
> +                                     &local_err);
> +            if (local_err != NULL) {
> +                break;
> +            }
> +        }
> +        if (bc->unrealize && local_err == NULL) {
>              bc->unrealize(bus, &local_err);
>          }
>      }
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] qdev: fix pci use-after-free

[Michael S. Tsirkin]

On Wed, Jun 11, 2014 at 02:52:07PM +0200, Paolo Bonzini wrote:
> See "Use-after-free during unrealize in system_reset" thread
> and individual patches.
> 
> Paolo

As this is blocking testing of hotplug, I applied this
on the pci tree.

Thanks!

> Paolo Bonzini (2):
>   qdev: reorganize error reporting in bus_set_realized
>   qdev: recursively unrealize devices when unrealizing bus
> 
>  hw/core/qdev.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] qdev: fix pci use-after-free

[Andreas Färber]

Am 15.06.2014 12:02, schrieb Michael S. Tsirkin:
> On Wed, Jun 11, 2014 at 02:52:07PM +0200, Paolo Bonzini wrote:
>> See "Use-after-free during unrealize in system_reset" thread
>> and individual patches.
>>
>> Paolo
> 
> As this is blocking testing of hotplug, I applied this
> on the pci tree.

Reviewed-by: Andreas Färber <afaerber@suse.de>

Only slowly catching up with my mail, please go ahead.

Andreas

> 
> Thanks!
> 
>> Paolo Bonzini (2):
>>   qdev: reorganize error reporting in bus_set_realized
>>   qdev: recursively unrealize devices when unrealizing bus
>>
>>  hw/core/qdev.c | 27 +++++++++++++++------------
>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> -- 
>> 1.8.3.1

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> 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: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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.