Memory leak in isp1760-hcd.c

Memory leak in isp1760-hcd.c

[Catalin Marinas]

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

Re: Memory leak in isp1760-hcd.c

[Catalin Marinas]

On 19 October 2011 16:21, Catalin Marinas <catalin.marinas@arm.com> wrote:
> I get the following kmemleak report coming from the ISP1760 driver:

BTW, that's Linux 3.1-rc9.

-- 
Catalin

Re: Memory leak in isp1760-hcd.c

[Arvid Brodin]

Hi, and sorry for the late reply.

Catalin Marinas wrote:
> Hi Arvid,
> 
> I get the following kmemleak report coming from the ISP1760 driver:
> 
*snip*
> 
> Is there a race condition between isp1760_endpoint_disable and
> isp1760_irq?
 
Yes, I think there is. Thanks for a great bug report!

Please try the patches in following emails.

-- 
Arvid Brodin
Enea Services Stockholm AB

Re: Memory leak in isp1760-hcd.c

[Catalin Marinas]

On 24 October 2011 14:50, Arvid Brodin <arvid.brodin@enea.com> wrote:
> Catalin Marinas wrote:
>> I get the following kmemleak report coming from the ISP1760 driver:
>>
> *snip*
>>
>> Is there a race condition between isp1760_endpoint_disable and
>> isp1760_irq?
>
> Yes, I think there is. Thanks for a great bug report!
>
> Please try the patches in following emails.

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

Re: Memory leak in isp1760-hcd.c

[Arvid Brodin]

Catalin Marinas wrote:
> On 24 October 2011 14:50, Arvid Brodin <arvid.brodin@enea.com> wrote:
>> Catalin Marinas wrote:
>>> I get the following kmemleak report coming from the ISP1760 driver:
>>>
>> *snip*
>>> Is there a race condition between isp1760_endpoint_disable and
>>> isp1760_irq?
>> Yes, I think there is. Thanks for a great bug report!
>>
>> Please try the patches in following emails.
> 
> 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 :)
> 

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

-- 
Arvid Brodin
Enea Services Stockholm AB

Re: Memory leak in isp1760-hcd.c - [PATCH 2/2] usb/isp1760: Fix race condition memory leak

[Arvid Brodin]

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(-)

diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
index 1adb21e..2df6631 100644
--- a/drivers/usb/host/isp1760-hcd.c
+++ b/drivers/usb/host/isp1760-hcd.c
@@ -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