memory leak in USB stack

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

Hi,
I have a USB device appearing as a ucom, that I open/close several
time per minute to collect data. After some time (several weeks) the
open(/dev/ttyU0) would fail with ENOMEM.

I tracked it down to a leak of a usbd_xfer_handle, one per open/close.
At open() time, a intr pipe is allocated via usbd_open_pipe_intr(),
which calls usbd_alloc_xfer() for pipe->intrxfer.
At close() time, usbd_abort_pipe() is called for the intr_pipe, then
usbd_close_pipe() which is supported to free pipe->intrxfer.
But usbd_abort_pipe() calls ehci_device_intr_abort() (but the same is true
for ehci and ohci) which, if the xfer to be aborted is the pipe's intrxfer,
sets pipe->intrxfer to NULL.  So usbd_close_pipe() has nothing to free any
more and the intrxfer is lost.
This happens with all 3 host drivers ([eou]hci) and a lot of
USB device drivers.

The attached patch fixes this by freeing the xfer in
[eou]hci_device_intr_abort() and [eou]hci_root_intr_abort()
when the xfer to be aborted is the pipe's intrxfer.
I went through the device drivers and it looks safe, as nothing
gets a cached value of pipe->intrxfer.

Comments ?

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: ehci.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/ehci.c,v
retrieving revision 1.154.4.1
diff -u -p -u -r1.154.4.1 ehci.c
--- ehci.c      29 Nov 2008 20:47:05 -0000      1.154.4.1
+++ ehci.c      12 Jan 2010 15:45:39 -0000
@@ -2454,15 +2454,19 @@ Static void
 ehci_root_intr_abort(usbd_xfer_handle xfer)
 {
        int s;
+       usbd_xfer_handle intrxfer = NULL;
 
        if (xfer->pipe->intrxfer == xfer) {
                DPRINTF(("ehci_root_intr_abort: remove\n"));
+               intrxfer = xfer->pipe->intrxfer;
                xfer->pipe->intrxfer = NULL;
        }
        xfer->status = USBD_CANCELLED;
        s = splusb();
        usb_transfer_complete(xfer);
        splx(s);
+       if (intrxfer)
+               usbd_free_xfer(intrxfer);
 }
 
 /* Close the root pipe. */
@@ -3686,9 +3690,11 @@ ehci_device_intr_start(usbd_xfer_handle 
 Static void
 ehci_device_intr_abort(usbd_xfer_handle xfer)
 {
+       usbd_xfer_handle intrxfer = NULL;
        DPRINTFN(1, ("ehci_device_intr_abort: xfer=%p\n", xfer));
        if (xfer->pipe->intrxfer == xfer) {
                DPRINTFN(1, ("echi_device_intr_abort: remove\n"));
+               intrxfer = xfer->pipe->intrxfer;
                xfer->pipe->intrxfer = NULL;
        }
        /*
@@ -3697,6 +3703,8 @@ ehci_device_intr_abort(usbd_xfer_handle 
         *       intr xfers are periodic, should not use this?
         */
        ehci_abort_xfer(xfer, USBD_CANCELLED);
+       if (intrxfer)
+               usbd_free_xfer(intrxfer);
 }
 
 Static void
Index: ohci.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/ohci.c,v
retrieving revision 1.196
diff -u -p -u -r1.196 ohci.c
--- ohci.c      13 Aug 2008 09:43:56 -0000      1.196
+++ ohci.c      12 Jan 2010 15:45:39 -0000
@@ -2838,15 +2838,19 @@ Static void
 ohci_root_intr_abort(usbd_xfer_handle xfer)
 {
        int s;
+       usbd_xfer_handle intrxfer = NULL;
 
        if (xfer->pipe->intrxfer == xfer) {
                DPRINTF(("ohci_root_intr_abort: remove\n"));
+               intrxfer = xfer->pipe->intrxfer;
                xfer->pipe->intrxfer = NULL;
        }
        xfer->status = USBD_CANCELLED;
        s = splusb();
        usb_transfer_complete(xfer);
        splx(s);
+       if (intrxfer)
+               usbd_free_xfer(intrxfer);
 }
 
 /* Close the root pipe. */
@@ -3185,11 +3189,15 @@ ohci_device_intr_start(usbd_xfer_handle 
 Static void
 ohci_device_intr_abort(usbd_xfer_handle xfer)
 {
+       usbd_xfer_handle intrxfer = NULL;
        if (xfer->pipe->intrxfer == xfer) {
                DPRINTF(("ohci_device_intr_abort: remove\n"));
+               intrxfer = xfer->pipe->intrxfer;
                xfer->pipe->intrxfer = NULL;
        }
        ohci_abort_xfer(xfer, USBD_CANCELLED);
+       if (intrxfer)
+               usbd_free_xfer(intrxfer);
 }
 
 /* Close a device interrupt pipe. */
Index: uhci.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/uhci.c,v
retrieving revision 1.223.6.1
diff -u -p -u -r1.223.6.1 uhci.c
--- uhci.c      4 Oct 2009 00:01:16 -0000       1.223.6.1
+++ uhci.c      12 Jan 2010 15:45:39 -0000
@@ -2377,12 +2377,16 @@ uhci_device_ctrl_close(usbd_pipe_handle 
 void
 uhci_device_intr_abort(usbd_xfer_handle xfer)
 {
+       usbd_xfer_handle intrxfer = NULL;
        DPRINTFN(1,("uhci_device_intr_abort: xfer=%p\n", xfer));
        if (xfer->pipe->intrxfer == xfer) {
                DPRINTFN(1,("uhci_device_intr_abort: remove\n"));
+               intrxfer = xfer->pipe->intrxfer;
                xfer->pipe->intrxfer = NULL;
        }
        uhci_abort_xfer(xfer, USBD_CANCELLED);
+       if (intrxfer)
+               usbd_free_xfer(intrxfer);
 }
 
 /* Close a device interrupt pipe. */
@@ -3816,6 +3820,7 @@ uhci_root_ctrl_close(usbd_pipe_handle pi
 void
 uhci_root_intr_abort(usbd_xfer_handle xfer)
 {
+       usbd_xfer_handle intrxfer = NULL;
        uhci_softc_t *sc = xfer->pipe->device->bus->hci_private;
 
        usb_uncallout(sc->sc_poll_handle, uhci_poll_hub, xfer);
@@ -3823,13 +3828,16 @@ uhci_root_intr_abort(usbd_xfer_handle xf
 
        if (xfer->pipe->intrxfer == xfer) {
                DPRINTF(("uhci_root_intr_abort: remove\n"));
-               xfer->pipe->intrxfer = 0;
+               intrxfer = xfer->pipe->intrxfer;
+               xfer->pipe->intrxfer = NULL;
        }
        xfer->status = USBD_CANCELLED;
 #ifdef DIAGNOSTIC
        UXFER(xfer)->iinfo.isdone = 1;
 #endif
        usb_transfer_complete(xfer);
+       if (intrxfer)
+               usbd_free_xfer(intrxfer);
 }
 
 usbd_status