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

[Qemu-devel] [PATCH v2 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

V2: remove checkpatch warnings for patch 2 as requested by Gerd Hoffmann

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     | 94 ++++++++++++++++++++++++++++++++++------------------
 hw/xen/xen_backend.c | 58 +++++++++++---------------------
 2 files changed, 81 insertions(+), 71 deletions(-)

-- 
2.6.6

[Qemu-devel] [PATCH v2 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>
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 v2 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 | 94 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 32 deletions(-)

diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 7992456..174d715 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,33 @@ 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 +723,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 +828,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,11 +846,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);
         }
     }
 
@@ -944,8 +967,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 +980,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 +1066,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 v2 2/2] xen: drain submit queue in xen-usb before removing device

[Anthony PERARD]

On Tue, Aug 02, 2016 at 02:14:04PM +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
> to the request indicating it should be just dropped on complete.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD