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 <m...@redhat.com>
object_unparent is called in many places, we really
should have some documentation for this API.
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonz...@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
