Memory leak in isp1760-hcd.c

Hi Arvid,

I get the following kmemleak report coming from the ISP1760 driver:

unreferenced object 0xef42d000 (size 28):
comm "khubd", pid 189, jiffies 4294937550 (age 1421.040s) hex dump (first 28 bytes):

    00 01 10 00 00 02 20 00 08 d0 42 ef 08 d0 42 ef  ...... ...B...B.           
    00 00 00 00 00 00 00 00 ff ff ff ff              ............               
  backtrace:                                                                    
    [<c0080fe1>] create_object+0xa1/0x1ac                                       
    [<c007eac5>] kmem_cache_alloc+0x8d/0xdc                                     
    [<c01a9617>] isp1760_urb_enqueue+0x2ab/0x2f8                                
    [<c019bbbd>] usb_hcd_submit_urb+0x75/0x574                                  
    [<c019d8f1>] usb_start_wait_urb+0x29/0x80                                   
    [<c019daad>] usb_control_msg+0x89/0xac                                      
    [<c0197f43>] hub_port_init+0x4fb/0x9c8                                      
    [<c0199c75>] hub_thread+0x5a1/0xd74                                         
    [<c0035acd>] kthread+0x69/0x6c                                              
    [<c000dc6d>] kernel_thread_exit+0x1/0x8                                     

After some investigation, it looks like 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.

Is there a race condition between isp1760_endpoint_disable and isp1760_irq?

Thanks.

Catalin

BTW, that's Linux 3.1-rc9.

Catalin

Hi, and sorry for the late reply.

*snip*

Yes, I think there is. Thanks for a great bug report!

Please try the patches in following emails.

Arvid Brodin
Enea Services Stockholm AB

This fixes a condition 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.

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 (2011-07-20).

---
 drivers/usb/host/isp1760-hcd.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)

index 1adb21e..2df6631 100644
@@ -922,7 +922,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;

@@ -940,17 +939,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);
-				}
-			}
 		}
 	}
 

@@ -1692,8 +1683,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);
 

@@ -1701,14 +1692,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);
 

1.6.3.3

-- 
Arvid Brodin
Enea Services Stockholm AB

Thanks. I'll try them next week as I'm at the KS now.

The good news, I'm using this leak as an example in my kmemleak presentations :)

Catalin

Hehe, what can I say... I'm honoured to be made an example of! ;D

Arvid Brodin
Enea Services Stockholm AB