[Qemu-devel] [PATCH v3 0/3] usb: fix segfault when hot-unplugging usb host adapter

[Qemu-devel] [PATCH v3 0/3] usb: fix segfault when hot-unplugging usb host adapter

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

When hot-unplugging the usb controllers (ehci/uhci),
we have to clean all resouce of these devices,
involved registered reset handler. Otherwise, it
may cause NULL pointer access and/or segmentation fault
if we reboot the guest os after hot-unplugging.
    
Let's hook up reset via DeviceClass->reset() and drop
the qemu_register_reset() call. Then Qemu will register
and unregister the reset handler automatically.

Cc: qemu-stable <qemu-stable@nongnu.org>

v2 -> v3:
 - rewrite each patch's title

v1 -> v2:
 - hooking up reset via DeviceClass->reset 
 and drop the qemu_register_reset() calls.   (Gerd)


Gonglei (3):
  uhci: fix segfault when hot-unplugging uhci controller
  ehci: fix segfault when hot-unplugging ehci controller
  ohci: fix resource cleanup leak

 hw/usb/hcd-ehci-pci.c | 10 ++++++++++
 hw/usb/hcd-ehci.c     |  3 +--
 hw/usb/hcd-ehci.h     |  1 +
 hw/usb/hcd-ohci.c     | 11 ++++++++++-
 hw/usb/hcd-uhci.c     | 12 ++++++------
 5 files changed, 28 insertions(+), 9 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v3 1/3] uhci: fix segfault when hot-unplugging uhci controller

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

When hot-unplugging the usb controllers (ehci/uhci),
we have to clean all resouce of these devices,
involved registered reset handler. Otherwise, it
may cause NULL pointer access and/or segmentation fault
if we reboot the guest os after hot-unplugging.

Let's hook up reset via DeviceClass->reset() and drop
the qemu_register_reset() call. Then Qemu will register
and unregister the reset handler automatically.

Cc: qemu-stable <qemu-stable@nongnu.org>
Reported-by: Lidonglin <lidonglin@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/usb/hcd-uhci.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index f903de7..d8f0577 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -348,9 +348,10 @@ static void uhci_update_irq(UHCIState *s)
     pci_set_irq(&s->dev, level);
 }
 
-static void uhci_reset(void *opaque)
+static void uhci_reset(DeviceState *dev)
 {
-    UHCIState *s = opaque;
+    PCIDevice *d = PCI_DEVICE(dev);
+    UHCIState *s = DO_UPCAST(UHCIState, dev, d);
     uint8_t *pci_conf;
     int i;
     UHCIPort *port;
@@ -454,11 +455,11 @@ static void uhci_port_write(void *opaque, hwaddr addr,
                 port = &s->ports[i];
                 usb_device_reset(port->port.dev);
             }
-            uhci_reset(s);
+            uhci_reset(DEVICE(s));
             return;
         }
         if (val & UHCI_CMD_HCRESET) {
-            uhci_reset(s);
+            uhci_reset(DEVICE(s));
             return;
         }
         s->cmd = val;
@@ -1226,8 +1227,6 @@ static int usb_uhci_common_initfn(PCIDevice *dev)
     s->num_ports_vmstate = NB_PORTS;
     QTAILQ_INIT(&s->queues);
 
-    qemu_register_reset(uhci_reset, s);
-
     memory_region_init_io(&s->io_bar, OBJECT(s), &uhci_ioport_ops, s,
                           "uhci", 0x20);
 
@@ -1303,6 +1302,7 @@ static void uhci_class_init(ObjectClass *klass, void *data)
     k->revision  = info->revision;
     k->class_id  = PCI_CLASS_SERIAL_USB;
     dc->vmsd = &vmstate_uhci;
+    dc->reset = uhci_reset;
     if (!info->unplug) {
         /* uhci controllers in companion setups can't be hotplugged */
         dc->hotpluggable = false;
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 2/3] ehci: fix segfault when hot-unplugging ehci controller

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

When hot-unplugging the usb controllers (ehci/uhci),
we have to clean all resouce of these devices,
involved registered reset handler. Otherwise, it
may cause NULL pointer access and/or segmentation fault
if we reboot the guest os after hot-unplugging.

Let's hook up reset via DeviceClass->reset() and drop
the qemu_register_reset() call. Then Qemu will register
and unregister the reset handler automatically.

Cc: qemu-stable <qemu-stable@nongnu.org>
Reported-by: Lidonglin <lidonglin@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/usb/hcd-ehci-pci.c | 10 ++++++++++
 hw/usb/hcd-ehci.c     |  3 +--
 hw/usb/hcd-ehci.h     |  1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 4c80707..7afa5f9 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -101,6 +101,15 @@ static void usb_ehci_pci_exit(PCIDevice *dev)
     }
 }
 
+static void usb_ehci_pci_reset(DeviceState *dev)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    EHCIPCIState *i = PCI_EHCI(pci_dev);
+    EHCIState *s = &i->ehci;
+
+    ehci_reset(s);
+}
+
 static void usb_ehci_pci_write_config(PCIDevice *dev, uint32_t addr,
                                       uint32_t val, int l)
 {
@@ -143,6 +152,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
     k->config_write = usb_ehci_pci_write_config;
     dc->vmsd = &vmstate_ehci_pci;
     dc->props = ehci_pci_properties;
+    dc->reset = usb_ehci_pci_reset;
 }
 
 static const TypeInfo ehci_pci_type_info = {
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index ccf54b6..2a84f46 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -845,7 +845,7 @@ static USBDevice *ehci_find_device(EHCIState *ehci, uint8_t addr)
 }
 
 /* 4.1 host controller initialization */
-static void ehci_reset(void *opaque)
+void ehci_reset(void *opaque)
 {
     EHCIState *s = opaque;
     int i;
@@ -2471,7 +2471,6 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp)
     s->async_bh = qemu_bh_new(ehci_frame_timer, s);
     s->device = dev;
 
-    qemu_register_reset(ehci_reset, s);
     s->vmstate = qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
 }
 
diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index 2bc259c..87b240f 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -325,6 +325,7 @@ extern const VMStateDescription vmstate_ehci;
 void usb_ehci_init(EHCIState *s, DeviceState *dev);
 void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp);
 void usb_ehci_unrealize(EHCIState *s, DeviceState *dev, Error **errp);
+void ehci_reset(void *opaque);
 
 #define TYPE_PCI_EHCI "pci-ehci-usb"
 #define PCI_EHCI(obj) OBJECT_CHECK(EHCIPCIState, (obj), TYPE_PCI_EHCI)
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 3/3] ohci: fix resource cleanup leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

When hot-unplugging the usb controllers (ehci/uhci),
we have to clean all resouce of these devices,
involved registered reset handler. Otherwise, it
may cause NULL pointer access and/or segmentation fault
if we reboot the guest os after hot-unplugging.

Let's hook up reset via DeviceClass->reset() and drop
the qemu_register_reset() call. Then Qemu will register
and unregister the reset handler automatically.

Ohci does't support hotplugging/hotunplugging yet, but
existing resource cleanup leak logic likes ehci/uhci.

Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/usb/hcd-ohci.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index a0d478e..5fa2f06 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1879,7 +1879,6 @@ static int usb_ohci_init(OHCIState *ohci, DeviceState *dev,
     usb_packet_init(&ohci->usb_packet);
 
     ohci->async_td = 0;
-    qemu_register_reset(ohci_reset, ohci);
 
     return 0;
 }
@@ -1951,6 +1950,15 @@ static void usb_ohci_exit(PCIDevice *dev)
     }
 }
 
+static void usb_ohci_reset_pci(DeviceState *d)
+{
+    PCIDevice *dev = PCI_DEVICE(d);
+    OHCIPCIState *ohci = PCI_OHCI(dev);
+    OHCIState *s = &ohci->state;
+
+    ohci_reset(s);
+}
+
 #define TYPE_SYSBUS_OHCI "sysbus-ohci"
 #define SYSBUS_OHCI(obj) OBJECT_CHECK(OHCISysBusState, (obj), TYPE_SYSBUS_OHCI)
 
@@ -2097,6 +2105,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data)
     dc->props = ohci_pci_properties;
     dc->hotpluggable = false;
     dc->vmsd = &vmstate_ohci;
+    dc->reset = usb_ohci_reset_pci;
 }
 
 static const TypeInfo ohci_pci_info = {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 1/3] uhci: fix segfault when hot-unplugging uhci controller

[Gerd Hoffmann]

  Hi,

> -static void uhci_reset(void *opaque)
> +static void uhci_reset(DeviceState *dev)
>  {
> -    UHCIState *s = opaque;
> +    PCIDevice *d = PCI_DEVICE(dev);
> +    UHCIState *s = DO_UPCAST(UHCIState, dev, d);

Uh, oh, DO_UPCAST() is long deprecated.  There are other instances of
this in the uhci emulation though, so we need a cleanup & qom-ify pass
for the code anyway.  So I think it's ok for a bugfix patch.

I'll queue it up (and the other two too of course).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 1/3] uhci: fix segfault when hot-unplugging uhci controller

[Gonglei]

On 2015/3/18 15:02, Gerd Hoffmann wrote:
>   Hi,
> 
>> -static void uhci_reset(void *opaque)
>> +static void uhci_reset(DeviceState *dev)
>>  {
>> -    UHCIState *s = opaque;
>> +    PCIDevice *d = PCI_DEVICE(dev);
>> +    UHCIState *s = DO_UPCAST(UHCIState, dev, d);
> 
> Uh, oh, DO_UPCAST() is long deprecated.  There are other instances of
> this in the uhci emulation though, so we need a cleanup & qom-ify pass
> for the code anyway.  So I think it's ok for a bugfix patch.
> 
Yes, I noticed that, but I haven't a good idea for qom-ifing uhci. :)
May we refer to the realization of ehci ?

> I'll queue it up (and the other two too of course).
> 
Thanks.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v3 1/3] uhci: fix segfault when hot-unplugging uhci controller

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> -static void uhci_reset(void *opaque)
>> +static void uhci_reset(DeviceState *dev)
>>  {
>> -    UHCIState *s = opaque;
>> +    PCIDevice *d = PCI_DEVICE(dev);
>> +    UHCIState *s = DO_UPCAST(UHCIState, dev, d);
>
> Uh, oh, DO_UPCAST() is long deprecated.  There are other instances of
[...]

Only 378 instances left in the code %-}

Re: [Qemu-devel] [PATCH v3 1/3] uhci: fix segfault when hot-unplugging uhci controller

[Gonglei]

On 2015/3/18 15:35, Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
>>   Hi,
>>
>>> -static void uhci_reset(void *opaque)
>>> +static void uhci_reset(DeviceState *dev)
>>>  {
>>> -    UHCIState *s = opaque;
>>> +    PCIDevice *d = PCI_DEVICE(dev);
>>> +    UHCIState *s = DO_UPCAST(UHCIState, dev, d);
>>
>> Uh, oh, DO_UPCAST() is long deprecated.  There are other instances of
> [...]
> 
> Only 378 instances left in the code %-}
> 
Maybe I can do some cleanup work for 2.4 ;)

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v3 2/3] ehci: fix segfault when hot-unplugging ehci controller

[Gerd Hoffmann]

On Mi, 2015-03-18 at 09:49 +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> When hot-unplugging the usb controllers (ehci/uhci),
> we have to clean all resouce of these devices,
> involved registered reset handler. Otherwise, it
> may cause NULL pointer access and/or segmentation fault
> if we reboot the guest os after hot-unplugging.
> 
> Let's hook up reset via DeviceClass->reset() and drop
> the qemu_register_reset() call. Then Qemu will register
> and unregister the reset handler automatically.

Fails "make check" (for aarch64).  My guess is the sysbus variants lost
the reset hookup.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 2/3] ehci: fix segfault when hot-unplugging ehci controller

[Gonglei]

On 2015/3/18 16:23, Gerd Hoffmann wrote:
> On Mi, 2015-03-18 at 09:49 +0800, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> When hot-unplugging the usb controllers (ehci/uhci),
>> we have to clean all resouce of these devices,
>> involved registered reset handler. Otherwise, it
>> may cause NULL pointer access and/or segmentation fault
>> if we reboot the guest os after hot-unplugging.
>>
>> Let's hook up reset via DeviceClass->reset() and drop
>> the qemu_register_reset() call. Then Qemu will register
>> and unregister the reset handler automatically.
> 
> Fails "make check" (for aarch64).  My guess is the sysbus variants lost
> the reset hookup.
> 
Actually, these fails were introduced by the follow patch:

commit c3cf77cb63b71618224129df41f114488e0f74e4
Author: David Gibson <david@gibson.dropbear.id.au>
Date:   Wed Feb 18 16:01:01 2015 +1100

    Make sysbus EHCI devices ARM only by default

    A number of ARM embedded boards include EHCI USB host controllers which
    appear as directly mapped devices, rather than sitting on a PCI bus.

    At present code to emulate such devices is included whenever EHCI support
    is included.  This patch adjusts teh config options to only include them
    in builds targetting ARM by default.

But on the other hand, the sysbus variants lost the reset hookup is a real bug,
I will fix them in the next version. thanks!

Regards,
-Gonglei