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 <address@hidden>

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

> Cc: address@hidden
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  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