kern/39356: DMA memory leak in Ohci Driver (USB)

[andreas.jacobs%lancom.de@localhost]

>Number:         39356
>Category:       kern
>Synopsis:       DMA memory leak in Ohci Driver (USB)
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Aug 15 09:10:00 +0000 2008
>Originator:     Andreas Jacobs
>Release:        CVS Head from 2008-08-14
>Organization:
LANCOM Systems GmbH
>Environment:
I do not use the NetBSD operating system itself but only its USB-Stack ported 
to another OS.
>Description:
If you attach an Ohci and later detach it again, it does not free all the 
memory it has allocated (note that this is another leak than in PR 39322):

ohci_alloc_sed() in ohci.c allocates new ohci_soft_ed_t objects, taking them 
from a linked list rooted at sc_freeeds. If that list is empty, that function 
allocates a new chunk of memory in line 420 to make further OHCI_SED_CHUCK 
ohci_soft_ed_t's available. This allocated memory is never released although in 
my opinion it should be released in ohci_detach at latest.

An equivalent problem exists with the function ohci_alloc_std which allocates 
memory in line 459.

I've got a patch for that (see below) and I'm curious what you think about it.

Thank you in advance
Andreas Jacobs

>How-To-Repeat:
Attach and detach an Ohci over and over again. This is hard to do, if that chip 
is part of your PC, but is possible, if you use a UMTS/HSPA modem attached via 
PCMCIA.

>Fix:
The ohci_soft_ed_t and ohci_soft_td_t structures may be in use until 
ohci_detach runs, but surely not longer, so in my opinion the blocks that 
ohci_attach_sed() and ohci_attach_std() allocate should be released exactly in 
ohci_detach().

Unfortunately, ohci_attach_sed() and ohci_attach_std() do not save pointers to 
these blocks yet, so there is no easy way for ohci_detach() to release these. 
Therefore my patch proposal becomes a little bit complicated:
- add a SIMPLEQ to struct ohci_softc to hold the allocated usb_dma structures
- add a SIMPLEQ_ENTRY to usb_dma_t, s.t. usb_dma_t's can be linked
- initialize that queue in ohci_init
- in ohci_attach_sed() and ohci_attach_std() add the allocated usb_dma_t's to 
the queue; note that the usb_dma_t's must be allocated on the heap instead of 
the stack for that
- finally in ohci_detach(): iterate through that queue and deallocate all the 
blocks


Here is my patch as a universal diff:
Index: ohci.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/ohci.c,v
retrieving revision 1.196
diff -U3 -r1.196 ohci.c
--- ohci.c      13 Aug 2008 09:43:56 -0000      1.196
+++ ohci.c      15 Aug 2008 08:26:37 -0000
@@ -385,6 +385,7 @@
 ohci_detach(struct ohci_softc *sc, int flags)
 {
        int rv = 0;
+       usb_dma_t *dma;
        usbd_xfer_handle xfer;
 
        if (sc->sc_child != NULL)
@@ -397,6 +398,11 @@
 
        usb_delay_ms(&sc->sc_bus, 300); /* XXX let stray task complete */
 
+       while((dma = SIMPLEQ_FIRST(&sc->sc_allocated_dmas)) != NULL) {
+               SIMPLEQ_REMOVE_HEAD(&sc->sc_allocated_dmas, next);
+               usb_freemem(&sc->sc_bus, dma);
+               free(dma, M_USB);
+       }
        usb_freemem(&sc->sc_bus, &sc->sc_hccadma);
        while((xfer = SIMPLEQ_FIRST(&sc->sc_free_xfers)) != NULL) {
                SIMPLEQ_REMOVE_HEAD(&sc->sc_free_xfers, next);
@@ -413,18 +419,24 @@
        ohci_soft_ed_t *sed;
        usbd_status err;
        int i, offs;
-       usb_dma_t dma;
+       usb_dma_t *dma;
 
        if (sc->sc_freeeds == NULL) {
                DPRINTFN(2, ("ohci_alloc_sed: allocating chunk\n"));
+               dma = malloc(sizeof *dma, M_USB, M_NOWAIT);
+               if(dma == NULL)
+                       return (0);
                err = usb_allocmem(&sc->sc_bus, OHCI_SED_SIZE * OHCI_SED_CHUNK,
-                         OHCI_ED_ALIGN, &dma);
-               if (err)
+                         OHCI_ED_ALIGN, dma);
+               if (err) {
+                       free(dma, M_USB);
                        return (0);
+               }
+               SIMPLEQ_INSERT_HEAD(&sc->sc_allocated_dmas, dma, next);
                for(i = 0; i < OHCI_SED_CHUNK; i++) {
                        offs = i * OHCI_SED_SIZE;
-                       sed = KERNADDR(&dma, offs);
-                       sed->physaddr = DMAADDR(&dma, offs);
+                       sed = KERNADDR(dma, offs);
+                       sed->physaddr = DMAADDR(dma, offs);
                        sed->dma = dma;
                        sed->offs = offs;
                        sed->next = sc->sc_freeeds;
@@ -451,20 +463,26 @@
        ohci_soft_td_t *std;
        usbd_status err;
        int i, offs;
-       usb_dma_t dma;
+       usb_dma_t *dma;
        int s;
 
        if (sc->sc_freetds == NULL) {
                DPRINTFN(2, ("ohci_alloc_std: allocating chunk\n"));
+               dma = malloc(sizeof *dma, M_USB, M_NOWAIT);
+               if(dma == NULL)
+                       return (0);
                err = usb_allocmem(&sc->sc_bus, OHCI_STD_SIZE * OHCI_STD_CHUNK,
-                         OHCI_TD_ALIGN, &dma);
-               if (err)
+                         OHCI_TD_ALIGN, dma);
+               if (err) {
+                       free(dma, M_USB);
                        return (NULL);
+               }
                s = splusb();
+               SIMPLEQ_INSERT_HEAD(&sc->sc_allocated_dmas, dma, next);
                for(i = 0; i < OHCI_STD_CHUNK; i++) {
                        offs = i * OHCI_STD_SIZE;
-                       std = KERNADDR(&dma, offs);
-                       std->physaddr = DMAADDR(&dma, offs);
+                       std = KERNADDR(dma, offs);
+                       std->physaddr = DMAADDR(dma, offs);
                        std->dma = dma;
                        std->offs = offs;
                        std->nexttd = sc->sc_freetds;
@@ -708,6 +726,7 @@
        for (i = 0; i < OHCI_HASH_SIZE; i++)
                LIST_INIT(&sc->sc_hash_itds[i]);
 
+       SIMPLEQ_INIT(&sc->sc_allocated_dmas);
        SIMPLEQ_INIT(&sc->sc_free_xfers);
 
 #ifdef __NetBSD__
Index: ohcivar.h
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/ohcivar.h,v
retrieving revision 1.45
diff -U3 -r1.45 ohcivar.h
--- ohcivar.h   28 Jun 2008 17:42:53 -0000      1.45
+++ ohcivar.h   15 Aug 2008 08:26:37 -0000
@@ -117,6 +117,7 @@
        ohci_soft_td_t *sc_freetds;
        ohci_soft_itd_t *sc_freeitds;
 
+       SIMPLEQ_HEAD(, usb_dma) sc_allocated_dmas;
        SIMPLEQ_HEAD(, usbd_xfer) sc_free_xfers; /* free xfers */
 
        usbd_xfer_handle sc_intrxfer;
Index: usb_port.h
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/usb_port.h,v
retrieving revision 1.85
diff -U3 -r1.85 usb_port.h
--- usb_port.h  28 Jun 2008 09:06:20 -0000      1.85
+++ usb_port.h  15 Aug 2008 08:26:37 -0000
@@ -102,9 +102,10 @@
 
 #define DECLARE_USB_DMA_T \
        struct usb_dma_block; \
-       typedef struct { \
+       typedef struct usb_dma { \
                struct usb_dma_block *block; \
                u_int offs; \
+               SIMPLEQ_ENTRY(usb_dma) next; \
        } usb_dma_t
 
 typedef struct callout usb_callout_t;