[Qemu-devel] Use-after-free during unrealize in system_reset

[Qemu-devel] Use-after-free during unrealize in system_reset

[Stefan Hajnoczi]

qemu-iotests ./check -qcow2 067 is entering an infinite loop during
system_reset.  This failure is a non-deterministic use-after-free and
the infinite loop happens maybe 1/3 of the time.

Michael Tsirkin found that pci_unregister_device() is called before
pci_find_capability_list() since the parent is destroyed before the
child.

Looks like a qdev/qom issue.  Any takers?

Thread 1 (Thread 0x7fd9104fda80 (LWP 11716)):
#0  pci_find_capability_list (pdev=0x7fd912e94fc0, prev_p=<synthetic
pointer>, cap_id=17 '\021') at hw/pci/pci.c:1839
#1  pci_del_capability (pdev=pdev@entry=0x7fd912e94fc0,
cap_id=cap_id@entry=17 '\021', size=size@entry=12 '\f') at
hw/pci/pci.c:2079
#2  0x00007fd9106d8e46 in msix_uninit (dev=0x7fd912e94fc0,
table_bar=0x7fd912e953b0, pba_bar=0x7fd912e953b0) at hw/pci/msix.c:357
#3  0x00007fd9106d8f13 in msix_uninit_exclusive_bar
(dev=0x7fd912e94fc0) at hw/pci/msix.c:377
#4  0x00007fd9107e6dcf in virtio_device_unrealize (dev=0x7fd912e958a8,
errp=0x7fff20cb8370) at /home/stefanha/qemu/hw/virtio/virtio.c:1199
#5  0x00007fd910692f93 in device_set_realized (obj=<optimized out>,
value=<optimized out>, errp=0x0) at hw/core/qdev.c:847
#6  0x00007fd910761c2e in property_set_bool (obj=0x7fd912e958a8,
v=<optimized out>, opaque=0x7fd912e96ad0, name=<optimized out>,
errp=0x0)
    at qom/object.c:1421
#7  0x00007fd9107641b7 in object_property_set_qobject
(obj=obj@entry=0x7fd912e958a8, value=value@entry=0x7fd912e6bd70,
    name=name@entry=0x7fd9108a9a3a "realized", errp=errp@entry=0x0) at
qom/qom-qobject.c:24
#8  0x00007fd910763090 in object_property_set_bool
(obj=obj@entry=0x7fd912e958a8, value=value@entry=false,
    name=name@entry=0x7fd9108a9a3a "realized", errp=errp@entry=0x0) at
qom/object.c:883
#9  0x00007fd910691958 in device_unparent (obj=0x7fd912e958a8) at
hw/core/qdev.c:946
#10 0x00007fd910762d41 in object_unparent (obj=0x7fd912e958a8) at
qom/object.c:400
#11 0x00007fd910691db8 in bus_unparent (obj=<optimized out>) at
hw/core/qdev.c:547
#12 0x00007fd910762d41 in object_unparent (obj=0x7fd912e95830) at
qom/object.c:400
#13 0x00007fd910691809 in device_unparent (obj=0x7fd912e94fc0) at
hw/core/qdev.c:950
#14 0x00007fd910762d41 in object_unparent (obj=0x7fd912e94fc0) at
qom/object.c:400
#15 0x00007fd91066b7b6 in acpi_pcihp_eject_slot
(s=s@entry=0x7fd912e8e048, bsel=bsel@entry=0, slots=<optimized out>)
at hw/acpi/pcihp.c:139
#16 0x00007fd91066b862 in acpi_pcihp_update_hotplug_bus (bsel=0,
s=0x7fd912e8e048) at hw/acpi/pcihp.c:152
#17 acpi_pcihp_update (s=0x7fd912e8e048) at hw/acpi/pcihp.c:176
#18 acpi_pcihp_reset (s=0x7fd912e8e048) at hw/acpi/pcihp.c:182
#19 0x00007fd910797abd in qemu_devices_reset () at vl.c:1893
---Type <return> to continue, or q <return> to quit---
#20 qemu_system_reset (report=<optimized out>) at vl.c:1906
#21 0x00007fd91060bfaf in main_loop_should_exit () at vl.c:2041
#22 main_loop () at vl.c:2081
#23 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized
out>) at vl.c:4565

Thread 2 (Thread 0x7fd8f9c83700 (LWP 11722)):
#0  __lll_lock_wait () at
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007fd90deea59c in _L_cond_lock_789 () from /lib64/libpthread.so.0
#2  0x00007fd90deea431 in __pthread_mutex_cond_lock
(mutex=0x7fd9110f6c00 <qemu_global_mutex>) at
../nptl/pthread_mutex_lock.c:79
#3  0x00007fd90dee4db0 in pthread_cond_wait@@GLIBC_2.3.2 () at
../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:259
#4  0x00007fd9108837f9 in qemu_cond_wait (cond=<optimized out>,
mutex=mutex@entry=0x7fd9110f6c00 <qemu_global_mutex>) at
util/qemu-thread-posix.c:135
#5  0x00007fd91079c8d8 in qemu_tcg_cpu_thread_fn (arg=<optimized out>)
at /home/stefanha/qemu/cpus.c:943
#6  0x00007fd90dee0f33 in start_thread (arg=0x7fd8f9c83700) at
pthread_create.c:309
#7  0x00007fd906b23ded in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Michael S. Tsirkin]

On Thu, Jun 05, 2014 at 05:31:45PM +0200, Stefan Hajnoczi wrote:
> qemu-iotests ./check -qcow2 067 is entering an infinite loop during
> system_reset.  This failure is a non-deterministic use-after-free and
> the infinite loop happens maybe 1/3 of the time.

This patch makes it fail deterministically.

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 22fe5ee..6815fad 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -790,6 +790,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
 static void pci_config_free(PCIDevice *pci_dev)
 {
     g_free(pci_dev->config);
+    pci_dev->config = NULL;
     g_free(pci_dev->cmask);
     g_free(pci_dev->wmask);
     g_free(pci_dev->w1cmask);


> Michael Tsirkin found that pci_unregister_device() is called before
> pci_find_capability_list() since the parent is destroyed before the
> child.
> 
> Looks like a qdev/qom issue.  Any takers?

With virtio, virtio pci is the bus and devices are children
behind it.
I suspect that bus is unrealized before children.


> Thread 1 (Thread 0x7fd9104fda80 (LWP 11716)):
> #0  pci_find_capability_list (pdev=0x7fd912e94fc0, prev_p=<synthetic
> pointer>, cap_id=17 '\021') at hw/pci/pci.c:1839
> #1  pci_del_capability (pdev=pdev@entry=0x7fd912e94fc0,
> cap_id=cap_id@entry=17 '\021', size=size@entry=12 '\f') at
> hw/pci/pci.c:2079
> #2  0x00007fd9106d8e46 in msix_uninit (dev=0x7fd912e94fc0,
> table_bar=0x7fd912e953b0, pba_bar=0x7fd912e953b0) at hw/pci/msix.c:357
> #3  0x00007fd9106d8f13 in msix_uninit_exclusive_bar
> (dev=0x7fd912e94fc0) at hw/pci/msix.c:377
> #4  0x00007fd9107e6dcf in virtio_device_unrealize (dev=0x7fd912e958a8,
> errp=0x7fff20cb8370) at /home/stefanha/qemu/hw/virtio/virtio.c:1199
> #5  0x00007fd910692f93 in device_set_realized (obj=<optimized out>,
> value=<optimized out>, errp=0x0) at hw/core/qdev.c:847
> #6  0x00007fd910761c2e in property_set_bool (obj=0x7fd912e958a8,
> v=<optimized out>, opaque=0x7fd912e96ad0, name=<optimized out>,
> errp=0x0)
>     at qom/object.c:1421
> #7  0x00007fd9107641b7 in object_property_set_qobject
> (obj=obj@entry=0x7fd912e958a8, value=value@entry=0x7fd912e6bd70,
>     name=name@entry=0x7fd9108a9a3a "realized", errp=errp@entry=0x0) at
> qom/qom-qobject.c:24
> #8  0x00007fd910763090 in object_property_set_bool
> (obj=obj@entry=0x7fd912e958a8, value=value@entry=false,
>     name=name@entry=0x7fd9108a9a3a "realized", errp=errp@entry=0x0) at
> qom/object.c:883
> #9  0x00007fd910691958 in device_unparent (obj=0x7fd912e958a8) at
> hw/core/qdev.c:946
> #10 0x00007fd910762d41 in object_unparent (obj=0x7fd912e958a8) at
> qom/object.c:400
> #11 0x00007fd910691db8 in bus_unparent (obj=<optimized out>) at
> hw/core/qdev.c:547
> #12 0x00007fd910762d41 in object_unparent (obj=0x7fd912e95830) at
> qom/object.c:400
> #13 0x00007fd910691809 in device_unparent (obj=0x7fd912e94fc0) at
> hw/core/qdev.c:950
> #14 0x00007fd910762d41 in object_unparent (obj=0x7fd912e94fc0) at
> qom/object.c:400
> #15 0x00007fd91066b7b6 in acpi_pcihp_eject_slot
> (s=s@entry=0x7fd912e8e048, bsel=bsel@entry=0, slots=<optimized out>)
> at hw/acpi/pcihp.c:139
> #16 0x00007fd91066b862 in acpi_pcihp_update_hotplug_bus (bsel=0,
> s=0x7fd912e8e048) at hw/acpi/pcihp.c:152
> #17 acpi_pcihp_update (s=0x7fd912e8e048) at hw/acpi/pcihp.c:176
> #18 acpi_pcihp_reset (s=0x7fd912e8e048) at hw/acpi/pcihp.c:182
> #19 0x00007fd910797abd in qemu_devices_reset () at vl.c:1893
> ---Type <return> to continue, or q <return> to quit---
> #20 qemu_system_reset (report=<optimized out>) at vl.c:1906
> #21 0x00007fd91060bfaf in main_loop_should_exit () at vl.c:2041
> #22 main_loop () at vl.c:2081
> #23 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized
> out>) at vl.c:4565
> 
> Thread 2 (Thread 0x7fd8f9c83700 (LWP 11722)):
> #0  __lll_lock_wait () at
> ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> #1  0x00007fd90deea59c in _L_cond_lock_789 () from /lib64/libpthread.so.0
> #2  0x00007fd90deea431 in __pthread_mutex_cond_lock
> (mutex=0x7fd9110f6c00 <qemu_global_mutex>) at
> ../nptl/pthread_mutex_lock.c:79
> #3  0x00007fd90dee4db0 in pthread_cond_wait@@GLIBC_2.3.2 () at
> ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:259
> #4  0x00007fd9108837f9 in qemu_cond_wait (cond=<optimized out>,
> mutex=mutex@entry=0x7fd9110f6c00 <qemu_global_mutex>) at
> util/qemu-thread-posix.c:135
> #5  0x00007fd91079c8d8 in qemu_tcg_cpu_thread_fn (arg=<optimized out>)
> at /home/stefanha/qemu/cpus.c:943
> #6  0x00007fd90dee0f33 in start_thread (arg=0x7fd8f9c83700) at
> pthread_create.c:309
> #7  0x00007fd906b23ded in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Stefan Hajnoczi]

On Thu, Jun 5, 2014 at 6:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jun 05, 2014 at 05:31:45PM +0200, Stefan Hajnoczi wrote:
>> qemu-iotests ./check -qcow2 067 is entering an infinite loop during
>> system_reset.  This failure is a non-deterministic use-after-free and
>> the infinite loop happens maybe 1/3 of the time.
>
> This patch makes it fail deterministically.
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 22fe5ee..6815fad 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -790,6 +790,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
>  static void pci_config_free(PCIDevice *pci_dev)
>  {
>      g_free(pci_dev->config);
> +    pci_dev->config = NULL;
>      g_free(pci_dev->cmask);
>      g_free(pci_dev->wmask);
>      g_free(pci_dev->w1cmask);

Thanks for the patch, I bisected the use-after-free to this commit:

commit 5c21ce77d7e5643089ceec556c0408445d017f32
Author: Bandan Das <bsd@redhat.com>
Date:   Wed Mar 12 21:02:12 2014 +0100

    qdev: Realize buses on device realization

    Integrate (un)realization of child buses with realization/unrealization
    of the device hosting them. Code in device_unparent() is reordered for
    unrealization of buses to work as part of device unrealization.

    That way no changes need to be made to bus instantiation.

    Signed-off-by: Bandan Das <bsd@redhat.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

Stefan

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Paolo Bonzini]

Il 06/06/2014 11:03, Stefan Hajnoczi ha scritto:
> commit 5c21ce77d7e5643089ceec556c0408445d017f32
> Author: Bandan Das <bsd@redhat.com>
> Date:   Wed Mar 12 21:02:12 2014 +0100
>
>     qdev: Realize buses on device realization
>
>     Integrate (un)realization of child buses with realization/unrealization
>     of the device hosting them. Code in device_unparent() is reordered for
>     unrealization of buses to work as part of device unrealization.
>
>     That way no changes need to be made to bus instantiation.
>
>     Signed-off-by: Bandan Das <bsd@redhat.com>
>     Signed-off-by: Andreas Färber <afaerber@suse.de>

This hunk seems wrong.  Bandan, what was the reason for it?


@@ -841,13 +858,13 @@ static void device_unparent(Object *obj)
      QObject *event_data;
      bool have_realized = dev->realized;

+    if (dev->realized) {
+        object_property_set_bool(obj, false, "realized", NULL);
+    }
      while (dev->num_child_bus) {
          bus = QLIST_FIRST(&dev->child_bus);
          object_unparent(OBJECT(bus));
      }
-    if (dev->realized) {
-        object_property_set_bool(obj, false, "realized", NULL);
-    }
      if (dev->parent_bus) {
          bus_remove_child(dev->parent_bus, dev);
          object_unref(OBJECT(dev->parent_bus));



Paolo

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Michael S. Tsirkin]

On Fri, Jun 06, 2014 at 11:52:46AM +0200, Paolo Bonzini wrote:
> Il 06/06/2014 11:03, Stefan Hajnoczi ha scritto:
> >commit 5c21ce77d7e5643089ceec556c0408445d017f32
> >Author: Bandan Das <bsd@redhat.com>
> >Date:   Wed Mar 12 21:02:12 2014 +0100
> >
> >    qdev: Realize buses on device realization
> >
> >    Integrate (un)realization of child buses with realization/unrealization
> >    of the device hosting them. Code in device_unparent() is reordered for
> >    unrealization of buses to work as part of device unrealization.
> >
> >    That way no changes need to be made to bus instantiation.
> >
> >    Signed-off-by: Bandan Das <bsd@redhat.com>
> >    Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> This hunk seems wrong.  Bandan, what was the reason for it?
> 
> 
> @@ -841,13 +858,13 @@ static void device_unparent(Object *obj)
>      QObject *event_data;
>      bool have_realized = dev->realized;
> 
> +    if (dev->realized) {
> +        object_property_set_bool(obj, false, "realized", NULL);
> +    }
>      while (dev->num_child_bus) {
>          bus = QLIST_FIRST(&dev->child_bus);
>          object_unparent(OBJECT(bus));
>      }
> -    if (dev->realized) {
> -        object_property_set_bool(obj, false, "realized", NULL);
> -    }
>      if (dev->parent_bus) {
>          bus_remove_child(dev->parent_bus, dev);
>          object_unref(OBJECT(dev->parent_bus));
> 

Tested-by: Michael S. Tsirkin <mst@redhat.com>



> 
> Paolo

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Paolo Bonzini]

Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
> Tested-by: Michael S. Tsirkin <mst@redhat.com>

You probably tested the reversal, actually. :)

Actually, there is a reason for it.  "Unassembling" the device
(unparent) should come after "powering it down" (unrealize).

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 = 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);
-
-            if (local_err != NULL) {
-                goto error;
-            }
-
          }
+
+        /* 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);
-
-            if (local_err != NULL) {
-                goto error;
-            }
          }
      }

+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
      bus->realized = 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

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Michael S. Tsirkin]

On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
> >Tested-by: Michael S. Tsirkin <mst@redhat.com>
> 
> You probably tested the reversal, actually. :)

I guess so, maybe patch asked me about it.

> Actually, there is a reason for it.  "Unassembling" the device
> (unparent) should come after "powering it down" (unrealize).
> 
> 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 = 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);
> -
> -            if (local_err != NULL) {
> -                goto error;
> -            }
> -
>          }
> +
> +        /* 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);
> -
> -            if (local_err != NULL) {
> -                goto error;
> -            }
>          }
>      }
> 
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      bus->realized = 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

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Michael S. Tsirkin]

On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
> >Tested-by: Michael S. Tsirkin <mst@redhat.com>
> 
> You probably tested the reversal, actually. :)
> 
> Actually, there is a reason for it.  "Unassembling" the device
> (unparent) should come after "powering it down" (unrealize).
> 
> 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 = 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);
> -
> -            if (local_err != NULL) {
> -                goto error;
> -            }
> -
>          }
> +
> +        /* 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);
> -
> -            if (local_err != NULL) {
> -                goto error;
> -            }
>          }
>      }
> 
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      bus->realized = 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

Want to submit as a proper patch?

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Paolo Bonzini]

Il 08/06/2014 16:52, Michael S. Tsirkin ha scritto:
> On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
>> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> You probably tested the reversal, actually. :)
>>
>> Actually, there is a reason for it.  "Unassembling" the device
>> (unparent) should come after "powering it down" (unrealize).
>>
>> 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 = 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);
>> -
>> -            if (local_err != NULL) {
>> -                goto error;
>> -            }
>> -
>>          }
>> +
>> +        /* 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);
>> -
>> -            if (local_err != NULL) {
>> -                goto error;
>> -            }
>>          }
>>      }
>>
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>>      bus->realized = 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
>
> Want to submit as a proper patch?

I can, but I suppose Andreas will watch this when he's back and comment.

Paolo

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Michael S. Tsirkin]

On Mon, Jun 09, 2014 at 09:51:56AM +0200, Paolo Bonzini wrote:
> Il 08/06/2014 16:52, Michael S. Tsirkin ha scritto:
> >On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
> >>Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
> >>>Tested-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >>You probably tested the reversal, actually. :)
> >>
> >>Actually, there is a reason for it.  "Unassembling" the device
> >>(unparent) should come after "powering it down" (unrealize).
> >>
> >>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 = 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);
> >>-
> >>-            if (local_err != NULL) {
> >>-                goto error;
> >>-            }
> >>-
> >>         }
> >>+
> >>+        /* 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);
> >>-
> >>-            if (local_err != NULL) {
> >>-                goto error;
> >>-            }
> >>         }
> >>     }
> >>
> >>+    if (local_err != NULL) {
> >>+        error_propagate(errp, local_err);
> >>+        return;
> >>+    }
> >>+
> >>     bus->realized = 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
> >
> >Want to submit as a proper patch?
> 
> I can, but I suppose Andreas will watch this when he's back and comment.
> 
> Paolo

I don't think we want to keep such serious memory corruptors out of
tree. We can always revert the fix if necessary, for now
it's impossible for people to test hotplug properly,
this blocks progress.

-- 
MST

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>
> You probably tested the reversal, actually. :)
>
> Actually, there is a reason for it.  "Unassembling" the device
> (unparent) should come after "powering it down" (unrealize).

Yes, exactly. But to be honest, I was not sure if I was doing the 
right thing and hence posted this series as a RFC.

> 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 = 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);
> -
> -            if (local_err != NULL) {
> -                goto error;
> -            }
> -
>          }
> +
> +        /* 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);

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..

> -
> -            if (local_err != NULL) {
> -                goto error;
> -            }
>          }
>      }
>
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      bus->realized = 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

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Andreas Färber]

Am 09.06.2014 19:02, schrieb Bandan Das:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> You probably tested the reversal, actually. :)
>>
>> Actually, there is a reason for it.  "Unassembling" the device
>> (unparent) should come after "powering it down" (unrealize).
> 
> Yes, exactly. But to be honest, I was not sure if I was doing the 
> right thing and hence posted this series as a RFC.
> 
>> 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 = 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);
>> -
>> -            if (local_err != NULL) {
>> -                goto error;
>> -            }
>> -
>>          }
>> +
>> +        /* 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);
> 
> 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.

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

> 
>> -
>> -            if (local_err != NULL) {
>> -                goto error;
>> -            }
>>          }
>>      }
>>
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>>      bus->realized = 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
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Paolo Bonzini]

Il 11/06/2014 14:03, Andreas Färber ha scritto:
> 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?

No, I don't think so.  The devices rely on being unrealized at a time 
when the PCI bus is still valid, but that's it.  It's just an ordering 
problem introduced by 2.0.

I will repost the patch as toplevel, split and with Cc to qemu-stable.

Paolo

Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Bandan Das]

Andreas Färber <afaerber@suse.de> writes:

> Am 09.06.2014 19:02, schrieb Bandan Das:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>>>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> You probably tested the reversal, actually. :)
>>>
>>> Actually, there is a reason for it.  "Unassembling" the device
>>> (unparent) should come after "powering it down" (unrealize).
>> 
>> Yes, exactly. But to be honest, I was not sure if I was doing the 
>> right thing and hence posted this series as a RFC.
>> 
>>> 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 = 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);
>>> -
>>> -            if (local_err != NULL) {
>>> -                goto error;
>>> -            }
>>> -
>>>          }
>>> +
>>> +        /* 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);
>> 
>> 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 
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
>
>> 
>>> -
>>> -            if (local_err != NULL) {
>>> -                goto error;
>>> -            }
>>>          }
>>>      }
>>>
>>> +    if (local_err != NULL) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>>      bus->realized = 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
>>