* Re: [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
2017-08-28 16:34 [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory Igor Mammedov
@ 2017-08-28 16:59 ` Philippe Mathieu-Daudé
2017-08-28 17:56 ` Thomas Huth
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-08-28 16:59 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: jsnow, qemu-block, thuth, mst
On 08/28/2017 01:34 PM, Igor Mammedov wrote:
> Fixes read after freeing error reported
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
>
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
>
> qdev_device_add()
> object_property_set_bool('realized', true)
> device_set_realized()
> ...
> pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
> ...
> s->dev = g_new0(AHCIDevice, ports);
> ...
> AHCIDevice *ad = &s->dev[i];
> ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
> ^^^ creates bus in memory allocated by above gnew()
> and adds it as child propety to ahci device
> ...
> hotplug_handler_plug(); -> goto post_realize_fail;
> pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
> ...
> g_free(s->dev);
> ^^^ free memory that holds children busses
>
> return with error from device_set_realized()
>
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/ide/ahci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>
> ide_exit(s);
> }
> + object_unparent(OBJECT(&ad->port));
> }
>
> g_free(s->dev);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
2017-08-28 16:34 [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory Igor Mammedov
2017-08-28 16:59 ` Philippe Mathieu-Daudé
@ 2017-08-28 17:56 ` Thomas Huth
2017-08-28 18:08 ` Michael S. Tsirkin
2017-08-28 22:41 ` John Snow
3 siblings, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2017-08-28 17:56 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: jsnow, qemu-block, f4bug, mst
On 28.08.2017 18:34, Igor Mammedov wrote:
> Fixes read after freeing error reported
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
>
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
>
> qdev_device_add()
> object_property_set_bool('realized', true)
> device_set_realized()
> ...
> pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
> ...
> s->dev = g_new0(AHCIDevice, ports);
> ...
> AHCIDevice *ad = &s->dev[i];
> ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
> ^^^ creates bus in memory allocated by above gnew()
> and adds it as child propety to ahci device
> ...
> hotplug_handler_plug(); -> goto post_realize_fail;
> pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
> ...
> g_free(s->dev);
> ^^^ free memory that holds children busses
>
> return with error from device_set_realized()
>
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/ide/ahci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>
> ide_exit(s);
> }
> + object_unparent(OBJECT(&ad->port));
> }
>
> g_free(s->dev);
>
Thanks, this fixes the problem for me with both, x86_64 and mips64el!
Tested-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
2017-08-28 16:34 [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory Igor Mammedov
2017-08-28 16:59 ` Philippe Mathieu-Daudé
2017-08-28 17:56 ` Thomas Huth
@ 2017-08-28 18:08 ` Michael S. Tsirkin
2017-08-28 22:41 ` John Snow
3 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2017-08-28 18:08 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, jsnow, qemu-block, thuth, f4bug
On Mon, Aug 28, 2017 at 06:34:45PM +0200, Igor Mammedov wrote:
> Fixes read after freeing error reported
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
>
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
>
> qdev_device_add()
> object_property_set_bool('realized', true)
> device_set_realized()
> ...
> pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
> ...
> s->dev = g_new0(AHCIDevice, ports);
> ...
> AHCIDevice *ad = &s->dev[i];
> ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
> ^^^ creates bus in memory allocated by above gnew()
> and adds it as child propety to ahci device
> ...
> hotplug_handler_plug(); -> goto post_realize_fail;
> pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
> ...
> g_free(s->dev);
> ^^^ free memory that holds children busses
>
> return with error from device_set_realized()
>
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Pls merge through ide tree.
> ---
> hw/ide/ahci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>
> ide_exit(s);
> }
> + object_unparent(OBJECT(&ad->port));
> }
>
> g_free(s->dev);
> --
> 2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
2017-08-28 16:34 [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory Igor Mammedov
` (2 preceding siblings ...)
2017-08-28 18:08 ` Michael S. Tsirkin
@ 2017-08-28 22:41 ` John Snow
3 siblings, 0 replies; 5+ messages in thread
From: John Snow @ 2017-08-28 22:41 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: thuth, f4bug, qemu-block, mst
On 08/28/2017 12:34 PM, Igor Mammedov wrote:
> Fixes read after freeing error reported
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
>
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
>
> qdev_device_add()
> object_property_set_bool('realized', true)
> device_set_realized()
> ...
> pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
> ...
> s->dev = g_new0(AHCIDevice, ports);
> ...
> AHCIDevice *ad = &s->dev[i];
> ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
> ^^^ creates bus in memory allocated by above gnew()
> and adds it as child propety to ahci device
> ...
> hotplug_handler_plug(); -> goto post_realize_fail;
> pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
> ...
> g_free(s->dev);
> ^^^ free memory that holds children busses
>
> return with error from device_set_realized()
>
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/ide/ahci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>
> ide_exit(s);
> }
> + object_unparent(OBJECT(&ad->port));
> }
>
> g_free(s->dev);
>
Nice, Thank you.
Reviewed-by: John Snow <jsnow@redhat.com>
Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git
--js
^ permalink raw reply [flat|nested] 5+ messages in thread