[Qemu-devel] [PATCH 0/2] xen: bug fixes in Xen backend handling

[Qemu-devel] [PATCH 0/2] xen: bug fixes in Xen backend handling

[Juergen Gross]

When testing qemu based pvusb backend two bugs have been discovered:
- detaching of a usb controller leads to memory clobbering in qemu
- detaching of a usb device with active I/O requests could result in
  crash of qemu

Juergen Gross (2):
  xen: when removing a backend don't remove many of them
  xen: drain submit queue in xen-usb before removing device

 hw/usb/xen-usb.c     | 93 +++++++++++++++++++++++++++++++++-------------------
 hw/xen/xen_backend.c | 58 +++++++++++---------------------
 2 files changed, 79 insertions(+), 72 deletions(-)

-- 
2.6.6

[Qemu-devel] [PATCH 1/2] xen: when removing a backend don't remove many of them

[Juergen Gross]

When a Xenstore watch fires indicating a backend has to be removed
don't remove all backends for that domain with the specified device
index, but just the one which has the correct type.

The easiest way to achieve this is to use the already determined
xendev as parameter for xen_be_del_xendev() instead of only the domid
and device index.

This at once removes the open coded QTAILQ_FOREACH_SAVE() in
xen_be_del_xendev() as there is no need to search for the correct
xendev any longer.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 hw/xen/xen_backend.c | 58 +++++++++++++++++-----------------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index bab79b1..3ceb778 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -321,48 +321,28 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
 /*
  * release xen backend device.
  */
-static struct XenDevice *xen_be_del_xendev(int dom, int dev)
+static void xen_be_del_xendev(struct XenDevice *xendev)
 {
-    struct XenDevice *xendev, *xnext;
-
-    /*
-     * This is pretty much like QTAILQ_FOREACH(xendev, &xendevs, next) but
-     * we save the next pointer in xnext because we might free xendev.
-     */
-    xnext = xendevs.tqh_first;
-    while (xnext) {
-        xendev = xnext;
-        xnext = xendev->next.tqe_next;
-
-        if (xendev->dom != dom) {
-            continue;
-        }
-        if (xendev->dev != dev && dev != -1) {
-            continue;
-        }
-
-        if (xendev->ops->free) {
-            xendev->ops->free(xendev);
-        }
-
-        if (xendev->fe) {
-            char token[XEN_BUFSIZE];
-            snprintf(token, sizeof(token), "fe:%p", xendev);
-            xs_unwatch(xenstore, xendev->fe, token);
-            g_free(xendev->fe);
-        }
+    if (xendev->ops->free) {
+        xendev->ops->free(xendev);
+    }
 
-        if (xendev->evtchndev != NULL) {
-            xenevtchn_close(xendev->evtchndev);
-        }
-        if (xendev->gnttabdev != NULL) {
-            xengnttab_close(xendev->gnttabdev);
-        }
+    if (xendev->fe) {
+        char token[XEN_BUFSIZE];
+        snprintf(token, sizeof(token), "fe:%p", xendev);
+        xs_unwatch(xenstore, xendev->fe, token);
+        g_free(xendev->fe);
+    }
 
-        QTAILQ_REMOVE(&xendevs, xendev, next);
-        g_free(xendev);
+    if (xendev->evtchndev != NULL) {
+        xenevtchn_close(xendev->evtchndev);
     }
-    return NULL;
+    if (xendev->gnttabdev != NULL) {
+        xengnttab_close(xendev->gnttabdev);
+    }
+
+    QTAILQ_REMOVE(&xendevs, xendev, next);
+    g_free(xendev);
 }
 
 /*
@@ -682,7 +662,7 @@ static void xenstore_update_be(char *watch, char *type, int dom,
     if (xendev != NULL) {
         bepath = xs_read(xenstore, 0, xendev->be, &len);
         if (bepath == NULL) {
-            xen_be_del_xendev(dom, dev);
+            xen_be_del_xendev(xendev);
         } else {
             free(bepath);
             xen_be_backend_changed(xendev, path);
-- 
2.6.6

Re: [Qemu-devel] [PATCH 1/2] xen: when removing a backend don't remove many of them

[Stefano Stabellini]

On Fri, 29 Jul 2016, Juergen Gross wrote:
> When a Xenstore watch fires indicating a backend has to be removed
> don't remove all backends for that domain with the specified device
> index, but just the one which has the correct type.
> 
> The easiest way to achieve this is to use the already determined
> xendev as parameter for xen_be_del_xendev() instead of only the domid
> and device index.
> 
> This at once removes the open coded QTAILQ_FOREACH_SAVE() in
> xen_be_del_xendev() as there is no need to search for the correct
> xendev any longer.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>  hw/xen/xen_backend.c | 58 +++++++++++++++++-----------------------------------
>  1 file changed, 19 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index bab79b1..3ceb778 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -321,48 +321,28 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
>  /*
>   * release xen backend device.
>   */
> -static struct XenDevice *xen_be_del_xendev(int dom, int dev)
> +static void xen_be_del_xendev(struct XenDevice *xendev)
>  {
> -    struct XenDevice *xendev, *xnext;
> -
> -    /*
> -     * This is pretty much like QTAILQ_FOREACH(xendev, &xendevs, next) but
> -     * we save the next pointer in xnext because we might free xendev.
> -     */
> -    xnext = xendevs.tqh_first;
> -    while (xnext) {
> -        xendev = xnext;
> -        xnext = xendev->next.tqe_next;
> -
> -        if (xendev->dom != dom) {
> -            continue;
> -        }
> -        if (xendev->dev != dev && dev != -1) {
> -            continue;
> -        }
> -
> -        if (xendev->ops->free) {
> -            xendev->ops->free(xendev);
> -        }
> -
> -        if (xendev->fe) {
> -            char token[XEN_BUFSIZE];
> -            snprintf(token, sizeof(token), "fe:%p", xendev);
> -            xs_unwatch(xenstore, xendev->fe, token);
> -            g_free(xendev->fe);
> -        }
> +    if (xendev->ops->free) {
> +        xendev->ops->free(xendev);
> +    }
>  
> -        if (xendev->evtchndev != NULL) {
> -            xenevtchn_close(xendev->evtchndev);
> -        }
> -        if (xendev->gnttabdev != NULL) {
> -            xengnttab_close(xendev->gnttabdev);
> -        }
> +    if (xendev->fe) {
> +        char token[XEN_BUFSIZE];
> +        snprintf(token, sizeof(token), "fe:%p", xendev);
> +        xs_unwatch(xenstore, xendev->fe, token);
> +        g_free(xendev->fe);
> +    }
>  
> -        QTAILQ_REMOVE(&xendevs, xendev, next);
> -        g_free(xendev);
> +    if (xendev->evtchndev != NULL) {
> +        xenevtchn_close(xendev->evtchndev);
>      }
> -    return NULL;
> +    if (xendev->gnttabdev != NULL) {
> +        xengnttab_close(xendev->gnttabdev);
> +    }
> +
> +    QTAILQ_REMOVE(&xendevs, xendev, next);
> +    g_free(xendev);
>  }
>  
>  /*
> @@ -682,7 +662,7 @@ static void xenstore_update_be(char *watch, char *type, int dom,
>      if (xendev != NULL) {
>          bepath = xs_read(xenstore, 0, xendev->be, &len);
>          if (bepath == NULL) {
> -            xen_be_del_xendev(dom, dev);
> +            xen_be_del_xendev(xendev);
>          } else {
>              free(bepath);
>              xen_be_backend_changed(xendev, path);
> -- 
> 2.6.6
> 

[Qemu-devel] [PATCH 2/2] xen: drain submit queue in xen-usb before removing device

[Juergen Gross]

When unplugging a device in the Xen pvusb backend drain the submit
queue before deallocation of the control structures. Otherwise there
will be bogus memory accesses when I/O contracts are finished.

Correlated to this issue is the handling of cancel requests: a packet
cancelled will still lead to the call of complete, so add a flag
to the request indicating it should be just dropped on complete.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 hw/usb/xen-usb.c | 93 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 33 deletions(-)

diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 7992456..6ad666e 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -90,6 +90,8 @@ struct usbback_req {
     void                     *buffer;
     void                     *isoc_buffer;
     struct libusb_transfer   *xfer;
+
+    bool                     cancelled;
 };
 
 struct usbback_hotplug {
@@ -301,20 +303,23 @@ static void usbback_do_response(struct usbback_req *usbback_req, int32_t status,
         usbback_req->isoc_buffer = NULL;
     }
 
-    res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
-    res->id = usbback_req->req.id;
-    res->status = status;
-    res->actual_length = actual_length;
-    res->error_count = error_count;
-    res->start_frame = 0;
-    usbif->urb_ring.rsp_prod_pvt++;
-    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&usbif->urb_ring, notify);
+    if (usbif->urb_sring) {
+        res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
+        res->id = usbback_req->req.id;
+        res->status = status;
+        res->actual_length = actual_length;
+        res->error_count = error_count;
+        res->start_frame = 0;
+        usbif->urb_ring.rsp_prod_pvt++;
+        RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&usbif->urb_ring, notify);
 
-    if (notify) {
-        xen_be_send_notify(xendev);
+        if (notify) {
+            xen_be_send_notify(xendev);
+        }
     }
 
-    usbback_put_req(usbback_req);
+    if (!usbback_req->cancelled)
+        usbback_put_req(usbback_req);
 }
 
 static void usbback_do_response_ret(struct usbback_req *usbback_req,
@@ -366,15 +371,14 @@ static void usbback_set_address(struct usbback_info *usbif,
     }
 }
 
-static bool usbback_cancel_req(struct usbback_req *usbback_req)
+static void usbback_cancel_req(struct usbback_req *usbback_req)
 {
-    bool ret = false;
-
     if (usb_packet_is_inflight(&usbback_req->packet)) {
         usb_cancel_packet(&usbback_req->packet);
-        ret = true;
+        QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
+        usbback_req->cancelled = true;
+        usbback_do_response_ret(usbback_req, -EPROTO);
     }
-    return ret;
 }
 
 static void usbback_process_unlink_req(struct usbback_req *usbback_req)
@@ -391,7 +395,7 @@ static void usbback_process_unlink_req(struct usbback_req *usbback_req)
     devnum = usbif_pipedevice(usbback_req->req.pipe);
     if (unlikely(devnum == 0)) {
         usbback_req->stub = usbif->ports +
-                            usbif_pipeportnum(usbback_req->req.pipe);
+                            usbif_pipeportnum(usbback_req->req.pipe) - 1;
         if (unlikely(!usbback_req->stub)) {
             ret = -ENODEV;
             goto fail_response;
@@ -406,9 +410,7 @@ static void usbback_process_unlink_req(struct usbback_req *usbback_req)
 
     QTAILQ_FOREACH(unlink_req, &usbback_req->stub->submit_q, q) {
         if (unlink_req->req.id == id) {
-            if (usbback_cancel_req(unlink_req)) {
-                usbback_do_response_ret(unlink_req, -EPROTO);
-            }
+            usbback_cancel_req(unlink_req);
             break;
         }
     }
@@ -681,6 +683,31 @@ static void usbback_hotplug_enq(struct usbback_info *usbif, unsigned port)
     usbback_hotplug_notify(usbif);
 }
 
+static void usbback_portid_drain(struct usbback_info *usbif, unsigned port)
+{
+    struct usbback_req *req, *tmp;
+    bool sched = false;
+
+    QTAILQ_FOREACH_SAFE(req, &usbif->ports[port - 1].submit_q, q, tmp) {
+        usbback_cancel_req(req);
+        sched = true;
+    }
+
+    if (sched)
+        qemu_bh_schedule(usbif->bh);
+}
+
+static void usbback_portid_detach(struct usbback_info *usbif, unsigned port)
+{
+    if (!usbif->ports[port - 1].attached)
+        return;
+
+    usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
+    usbif->ports[port - 1].attached = false;
+    usbback_portid_drain(usbif, port);
+    usbback_hotplug_enq(usbif, port);
+}
+
 static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
 {
     USBPort *p;
@@ -694,9 +721,7 @@ static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
 
     object_unparent(OBJECT(usbif->ports[port - 1].dev));
     usbif->ports[port - 1].dev = NULL;
-    usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
-    usbif->ports[port - 1].attached = false;
-    usbback_hotplug_enq(usbif, port);
+    usbback_portid_detach(usbif, port);
 
     TR_BUS(&usbif->xendev, "port %d removed\n", port);
 }
@@ -801,7 +826,6 @@ static void usbback_process_port(struct usbback_info *usbif, unsigned port)
 static void usbback_disconnect(struct XenDevice *xendev)
 {
     struct usbback_info *usbif;
-    struct usbback_req *req, *tmp;
     unsigned int i;
 
     TR_BUS(xendev, "start\n");
@@ -820,12 +844,8 @@ static void usbback_disconnect(struct XenDevice *xendev)
     }
 
     for (i = 0; i < usbif->num_ports; i++) {
-        if (!usbif->ports[i].dev) {
-            continue;
-        }
-        QTAILQ_FOREACH_SAFE(req, &usbif->ports[i].submit_q, q, tmp) {
-            usbback_cancel_req(req);
-        }
+        if (usbif->ports[i].dev)
+            usbback_portid_drain(usbif, i + 1);
     }
 
     TR_BUS(xendev, "finished\n");
@@ -944,8 +964,7 @@ static void xen_bus_detach(USBPort *port)
 
     usbif = port->opaque;
     TR_BUS(&usbif->xendev, "\n");
-    usbif->ports[port->index].attached = false;
-    usbback_hotplug_enq(usbif, port->index + 1);
+    usbback_portid_detach(usbif, port->index + 1);
 }
 
 static void xen_bus_child_detach(USBPort *port, USBDevice *child)
@@ -958,9 +977,16 @@ static void xen_bus_child_detach(USBPort *port, USBDevice *child)
 
 static void xen_bus_complete(USBPort *port, USBPacket *packet)
 {
+    struct usbback_req *usbback_req;
     struct usbback_info *usbif;
 
-    usbif = port->opaque;
+    usbback_req = container_of(packet, struct usbback_req, packet);
+    if (usbback_req->cancelled) {
+        g_free(usbback_req);
+        return;
+    }
+
+    usbif = usbback_req->usbif;
     TR_REQ(&usbif->xendev, "\n");
     usbback_packet_complete(packet);
 }
@@ -1037,6 +1063,7 @@ static int usbback_free(struct XenDevice *xendev)
     }
 
     usb_bus_release(&usbif->bus);
+    object_unparent(OBJECT(&usbif->bus));
 
     TR_BUS(xendev, "finished\n");
 
-- 
2.6.6

Re: [Qemu-devel] [PATCH 2/2] xen: drain submit queue in xen-usb before removing device

[Gerd Hoffmann]

On Fr, 2016-07-29 at 13:17 +0200, Juergen Gross wrote:
> When unplugging a device in the Xen pvusb backend drain the submit
> queue before deallocation of the control structures. Otherwise there
> will be bogus memory accesses when I/O contracts are finished.
> 
> Correlated to this issue is the handling of cancel requests: a packet
> cancelled will still lead to the call of complete, so add a flag
> A

=== checkpatch complains ===
WARNING: braces {} are necessary for all arms of this statement
#105: FILE: hw/usb/xen-usb.c:696:
+    if (sched)
[...]

WARNING: braces {} are necessary for all arms of this statement
#111: FILE: hw/usb/xen-usb.c:702:
+    if (!usbif->ports[port - 1].attached)
[...]

WARNING: braces {} are necessary for all arms of this statement
#152: FILE: hw/usb/xen-usb.c:847:
+        if (usbif->ports[i].dev)
[...]

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] xen: drain submit queue in xen-usb before removing device

[Juergen Gross]

On 02/08/16 13:37, Gerd Hoffmann wrote:
> On Fr, 2016-07-29 at 13:17 +0200, Juergen Gross wrote:
>> When unplugging a device in the Xen pvusb backend drain the submit
>> queue before deallocation of the control structures. Otherwise there
>> will be bogus memory accesses when I/O contracts are finished.
>>
>> Correlated to this issue is the handling of cancel requests: a packet
>> cancelled will still lead to the call of complete, so add a flag
>> A
> 
> === checkpatch complains ===

Aargh, sorry! Wrote too many Xen and Linux patches recently. I'll
correct those.


Juergen