From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46952) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fexdM-0002uo-0N for qemu-devel@nongnu.org; Mon, 16 Jul 2018 03:09:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fexdK-00062p-Nj for qemu-devel@nongnu.org; Mon, 16 Jul 2018 03:09:23 -0400 References: <1531470464-21522-1-git-send-email-thuth@redhat.com> <1531470464-21522-4-git-send-email-thuth@redhat.com> <20180713212613.GU31657@localhost.localdomain> From: Thomas Huth Message-ID: Date: Mon, 16 Jul 2018 09:09:13 +0200 MIME-Version: 1.0 In-Reply-To: <20180713212613.GU31657@localhost.localdomain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 03/16] hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported machines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Peter Maydell , Paolo Bonzini , qemu-arm@nongnu.org, Markus Armbruster , Beniamino Galvani , Subbaraya Sundeep , Alistair Francis , "Edgar E. Iglesias" , =?UTF-8?Q?Andreas_F=c3=a4rber?= On 13.07.2018 23:26, Eduardo Habkost wrote: > On Fri, Jul 13, 2018 at 10:27:31AM +0200, Thomas Huth wrote: >> When trying to "device_add bcm2837" on a machine that is not suitable for >> this device, you can quickly crash QEMU afterwards, e.g. with "info qtree": >> >> echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \ >> "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " \ >> "'arguments': {'command-line': 'info qtree'}}" | \ >> aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp stdio >> >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, >> "package": "build-all"}, "capabilities": []}} >> {"return": {}} >> {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be >> hotplugged on this machine"}} >> Segmentation fault (core dumped) >> >> The qdev_set_parent_bus() from instance_init adds a link to the child devices >> which is not valid anymore after the bcm2837 instance has been destroyed. >> Unfortunately, the child devices do not get destroyed / unlinked correctly >> because both object_initialize() and object_property_add_child() increase >> the reference count of the child objects by one, but only one reference >> is dropped when the parent gets removed. So let's use the new functions >> object_initialize_child() and sysbus_init_child_obj() instead to create >> the objects, which will take care of creating the child objects with the >> correct reference count of one. >> >> Signed-off-by: Thomas Huth > > Reviewed-by: Eduardo Habkost > > The usage of &error_abort in code that can be triggered from > device-list-properties still makes me nervous, but that's a > separate issue. I first had similar thoughts, but I think it's a clear coding issue if the abort triggers here (and not something that the user should normally be able to trigger somehow), so error_abort should be ok in this case. Thomas