usb/isp1760: Fix race condition memory leak

This fixes a memory leak reported by Catalin Marinas:

schedule_ptds() is called from isp1760_irq() and removes the qh from the
controlqhs queue but ep->hcpriv still points to the qh and therefore it is not
freed.

Shortly after this, the isp1760_endpoint_disable() function sets ep->hcpriv to
NULL and calls schedule_ptds() but since the corresponding qh is no longer in
the queue, it is simply forgotten and reported by kmemleak.

With this patch, the qh is always freed at endpoint_disable, instead, and the
corresponding entry removed from the queue head list.

While I was at it, I also replaced the lines in isp1760_endpoint_disable()
that removed remaining qtds from the qh with a WARN_ON check for non-empty qh,
in line with earlier comments from Alan Stern (linux-usb list, 2011-07-20).

Signed-off-by: Arvid Brodin <arvid.brodin@enea.com>
Tested-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
index a760fbf18ebedf1697411ccef66c788e55c7b270..fc72d44bf787a7be1907e5736cd3c11cc955e462 100644 (file)
--- a/drivers/usb/host/isp1760-hcd.c
+++ b/drivers/usb/host/isp1760-hcd.c
@@ -937,7 +937,6 @@ void schedule_ptds(struct usb_hcd *hcd)
        struct isp1760_hcd *priv;
        struct isp1760_qh *qh, *qh_next;
        struct list_head *ep_queue;
-       struct usb_host_endpoint *ep;
        LIST_HEAD(urb_list);
        struct urb_listitem *urb_listitem, *urb_listitem_next;
        int i;
@@ -955,17 +954,9 @@ void schedule_ptds(struct usb_hcd *hcd)
        for (i = 0; i < QH_END; i++) {
                ep_queue = &priv->qh_list[i];
                list_for_each_entry_safe(qh, qh_next, ep_queue, qh_list) {
-                       ep = list_entry(qh->qtd_list.next, struct isp1760_qtd,
-                                                       qtd_list)->urb->ep;
                        collect_qtds(hcd, qh, &urb_list);
-                       if (list_empty(&qh->qtd_list)) {
+                       if (list_empty(&qh->qtd_list))
                                list_del(&qh->qh_list);
-                               if (ep->hcpriv == NULL) {
-                                       /* Endpoint has been disabled, so we
-                                       can free the associated queue head. */
-                                       qh_free(qh);
-                               }
-                       }
                }
        }
 
@@ -1708,8 +1699,8 @@ static void isp1760_endpoint_disable(struct usb_hcd *hcd,
 {
        struct isp1760_hcd *priv = hcd_to_priv(hcd);
        unsigned long spinflags;
-       struct isp1760_qh *qh;
-       struct isp1760_qtd *qtd;
+       struct isp1760_qh *qh, *qh_iter;
+       int i;
 
        spin_lock_irqsave(&priv->lock, spinflags);
 
@@ -1717,14 +1708,17 @@ static void isp1760_endpoint_disable(struct usb_hcd *hcd,
        if (!qh)
                goto out;
 
-       list_for_each_entry(qtd, &qh->qtd_list, qtd_list)
-               if (qtd->status != QTD_RETIRE) {
-                       dequeue_urb_from_qtd(hcd, qh, qtd);
-                       qtd->urb->status = -ECONNRESET;
-               }
+       WARN_ON(!list_empty(&qh->qtd_list));
 
+       for (i = 0; i < QH_END; i++)
+               list_for_each_entry(qh_iter, &priv->qh_list[i], qh_list)
+                       if (qh_iter == qh) {
+                               list_del(&qh_iter->qh_list);
+                               i = QH_END;
+                               break;
+                       }
+       qh_free(qh);
        ep->hcpriv = NULL;
-       /* Cannot free qh here since it will be parsed by schedule_ptds() */
 
        schedule_ptds(hcd);