From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wuj1L-0008Sc-95 for qemu-devel@nongnu.org; Wed, 11 Jun 2014 09:57:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wuj1F-0003qZ-Ty for qemu-devel@nongnu.org; Wed, 11 Jun 2014 09:56:54 -0400 Date: Wed, 11 Jun 2014 16:57:15 +0300 From: "Michael S. Tsirkin" Message-ID: <20140611135715.GA8362@redhat.com> References: <1402491129-21933-1-git-send-email-pbonzini@redhat.com> <1402491129-21933-3-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1402491129-21933-3-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2] qdev: recursively unrealize devices when unrealizing bus List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-stable@nongnu.org, qemu-devel@nongnu.org, afaerber@suse.de 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 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 > --- > 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