From c64391f264b7658c00515173cca58f5b054af1a2 Mon Sep 17 00:00:00 2001 From: Arvid Brodin Date: Wed, 23 Nov 2011 18:13:02 +0100 Subject: [PATCH] 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 Tested-by: Catalin Marinas Signed-off-by: Greg Kroah-Hartman --- drivers/usb/host/isp1760-hcd.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c index a760fbf18ebe..fc72d44bf787 100644 --- 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); -- 2.20.1