From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40630) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wuko0-0007bu-EH for qemu-devel@nongnu.org; Wed, 11 Jun 2014 11:51:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wuknv-0000mc-Ff for qemu-devel@nongnu.org; Wed, 11 Jun 2014 11:51:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22883) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wuknv-0000mA-8H for qemu-devel@nongnu.org; Wed, 11 Jun 2014 11:51:11 -0400 From: Bandan Das References: <20140605161803.GB11292@redhat.com> <53918F6E.1020406@redhat.com> <20140608104626.GA26245@redhat.com> <539475F8.3060200@redhat.com> <53984595.3070602@suse.de> Date: Wed, 11 Jun 2014 11:51:05 -0400 In-Reply-To: <53984595.3070602@suse.de> ("Andreas \=\?utf-8\?Q\?F\=C3\=A4rber\=22\?\= \=\?utf-8\?Q\?'s\?\= message of "Wed, 11 Jun 2014 14:03:33 +0200") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] Use-after-free during unrealize in system_reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: peter.maydell@linaro.org, Paolo Bonzini , "Michael S. Tsirkin" , qemu-devel , Stefan Hajnoczi Andreas F=C3=A4rber writes: > Am 09.06.2014 19:02, schrieb Bandan Das: >> Paolo Bonzini writes: >>=20 >>> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto: >>>> Tested-by: Michael S. Tsirkin >>> >>> You probably tested the reversal, actually. :) >>> >>> Actually, there is a reason for it. "Unassembling" the device >>> (unparent) should come after "powering it down" (unrealize). >>=20 >> Yes, exactly. But to be honest, I was not sure if I was doing the=20 >> right thing and hence posted this series as a RFC. >>=20 >>> However, the bus is missing a recursive unrealization of the devices >>> below it prior to calling bc->unrealize: >>> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>> index e65a5aa..4282491 100644 >>> --- a/hw/core/qdev.c >>> +++ b/hw/core/qdev.c >>> @@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool >>> value, Error **errp) >>> { >>> BusState *bus =3D BUS(obj); >>> BusClass *bc =3D BUS_GET_CLASS(bus); >>> + BusChild *kid; >>> Error *local_err =3D NULL; >>> >>> if (value && !bus->realized) { >>> if (bc->realize) { >>> bc->realize(bus, &local_err); >>> - >>> - if (local_err !=3D NULL) { >>> - goto error; >>> - } >>> - >>> } >>> + >>> + /* TODO: recursive realization */ >>> } else if (!value && bus->realized) { >>> - if (bc->unrealize) { >>> + QTAILQ_FOREACH(kid, &bus->children, sibling) { >>> + DeviceState *dev =3D kid->child; >>> + object_property_set_bool(OBJECT(dev), false, "realized", >>> + &local_err); >>> + if (local_err !=3D NULL) { >>> + break; >>> + } >>> + } >>> + if (bc->unrealize && local_err =3D=3D NULL) { >>> bc->unrealize(bus, &local_err); >>=20 >> It also seems that my original patch included unrealizing children, but >> that didn't get in for some reason - >> https://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03204.html >> Andreas might have had a reason not to include it however.. > > Yes, we had talked about the future recursive (un)realization at KVM > Forum 2013, but your RFC patch implementing this for buses seemed to get > ahead of the game. I therefore took only the part of the patch that did > not contradict Paolo's objection to unchecked recursive realization - > and I was in a hurry to get it into 2.0 before the freeze, sorry. That's ok. I only regret that despite being posted as a RFC and way before the 2.0 freeze deadline, the patches received absolutely no feedback. If=20 we had discussed Paolo's objections when the patches were posted, someone might have pointed out what could go wrong. Bandan > Doing only recursive unrealization sounds like a good compromise to me, > since the concerns we had were all about recursive realization and > prereqs not being realized yet. Only suggestion would be to do the error > handling refactoring in a separate patch, but it'll better align with > the qdev.c code then. > > Still, isn't this an indication that devices relied on the PCI bus bug > of not unrealizing its state all the time and we may need to go back as > far as ~1.7 (the initial finalize based fix) for resolving it? > > Regards, > Andreas > >>=20 >>> - >>> - if (local_err !=3D NULL) { >>> - goto error; >>> - } >>> } >>> } >>> >>> + if (local_err !=3D NULL) { >>> + error_propagate(errp, local_err); >>> + return; >>> + } >>> + >>> bus->realized =3D value; >>> - return; >>> - >>> -error: >>> - error_propagate(errp, local_err); >>> } >>> >>> void qbus_create_inplace(void *bus, size_t size, const char *typename, >>> >>> >>> This seems to fix the bug too. >>> >>> Paolo >>=20