NetBSD Problem Report #39356

From www@NetBSD.org  Fri Aug 15 09:05:45 2008
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id DBFCD63B853
	for <gnats-bugs@gnats.netbsd.org>; Fri, 15 Aug 2008 09:05:44 +0000 (UTC)
Message-Id: <20080815090544.8FA0663B11D@narn.NetBSD.org>
Date: Fri, 15 Aug 2008 09:05:44 +0000 (UTC)
From: andreas.jacobs@lancom.de
Reply-To: andreas.jacobs@lancom.de
To: gnats-bugs@NetBSD.org
Subject: DMA memory leak in Ohci Driver (USB)
X-Send-Pr-Version: www-1.0

>Number:         39356
>Category:       kern
>Synopsis:       DMA memory leak in Ohci Driver (USB)
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    mrg
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Aug 15 09:10:00 +0000 2008
>Last-Modified:  Tue Sep 18 02:33:34 +0000 2018
>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;

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->mrg
Responsible-Changed-By: mrg@NetBSD.org
Responsible-Changed-When: Tue, 18 Sep 2018 02:33:34 +0000
Responsible-Changed-Why:
i'll take a look.  we can easily force detach/reattach cycle with drvctl today.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.