From owner-freebsd-current@FreeBSD.ORG Tue Nov 23 19:34:01 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id CE19916A4CE; Tue, 23 Nov 2004 19:34:01 +0000 (GMT) Received: from transport.cksoft.de (transport.cksoft.de [62.111.66.27]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9721343D45; Tue, 23 Nov 2004 19:33:59 +0000 (GMT) (envelope-from bzeeb-lists@lists.zabbadoz.net) Received: from transport.cksoft.de (localhost [127.0.0.1]) by transport.cksoft.de (Postfix) with ESMTP id 915831FFACA; Tue, 23 Nov 2004 20:33:57 +0100 (CET) Received: by transport.cksoft.de (Postfix, from userid 66) id 8E9311FF91D; Tue, 23 Nov 2004 20:33:55 +0100 (CET) Received: by mail.int.zabbadoz.net (Postfix, from userid 1060) id E893215667; Tue, 23 Nov 2004 19:31:10 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mail.int.zabbadoz.net (Postfix) with ESMTP id E526F15621; Tue, 23 Nov 2004 19:31:10 +0000 (UTC) Date: Tue, 23 Nov 2004 19:31:10 +0000 (UTC) From: "Bjoern A. Zeeb" X-X-Sender: bz@e0-0.zab2.int.zabbadoz.net To: John Baldwin In-Reply-To: <200411221644.57047.jhb@FreeBSD.org> Message-ID: References: <200411221644.57047.jhb@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Virus-Scanned: by AMaViS cksoft-s20020300-20031204bz on transport.cksoft.de cc: "Bjoern A. Zeeb" cc: freebsd-current@FreeBSD.org Subject: Re: mem leak in mii ? X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Nov 2004 19:34:02 -0000 On Mon, 22 Nov 2004, John Baldwin wrote: Hi, hope you won't get it twice; the first one didn't seem to go out... > On Friday 19 November 2004 06:49 pm, Bjoern A. Zeeb wrote: > > Hi, > > > > in sys/dev/mii/mii.c there are two calls to malloc for ivars; > > see for example mii_phy_probe: .. > > Where is the free for this malloc ? I cannot find it. > > > > analogous: miibus_probe ? > > It's a leak. It should be free'd when the miibus device is destroyed. Here's > a possible fix: could you please review this one ? Should plug both of the memleaks; also for more error cases. notes: * mii doesn't ssem to be very error corrective and reporting; as others currently also seem to be debugging problems with undetectable PHYs I added some error handling in those places that I touched anyway. * in miibus_probe in the loop there is the possibility - and the comment above the functions also talks about this - that we find more than one PHY ? I currrently doubt that but I don't know for sure. As device_add_child may return NULL we cannot check for that; I had seen some inconsistency while debugging the BMSR_MEDIAMASK check so I added the count variable for this to have a reliable state. * all PHY drivers currently seem to use mii_phy_detach for device_detach. If any implements his own function it will be responsible for freeing the ivars allocated in miibus_probe. This should perhaps be documented somewhere ? patch can also be found at http://sources.zabbadoz.net/freebsd/patchset/mii-memleaks.diff Index: mii.c =================================================================== RCS file: /local/mirror/FreeBSD/r/ncvs/src/sys/dev/mii/mii.c,v retrieving revision 1.20 diff -u -p -r1.20 mii.c --- mii.c 15 Aug 2004 06:24:40 -0000 1.20 +++ mii.c 23 Nov 2004 17:08:58 -0000 @@ -111,7 +111,7 @@ miibus_probe(dev) struct mii_attach_args ma, *args; struct mii_data *mii; device_t child = NULL, parent; - int bmsr, capmask = 0xFFFFFFFF; + int count = 0, bmsr, capmask = 0xFFFFFFFF; mii = device_get_softc(dev); parent = device_get_parent(dev); @@ -145,12 +145,26 @@ miibus_probe(dev) args = malloc(sizeof(struct mii_attach_args), M_DEVBUF, M_NOWAIT); + if (args == NULL) { + device_printf(dev, "%s: memory allocation failure, " + "phyno %d", __func__, ma.mii_phyno); + continue; + } bcopy((char *)&ma, (char *)args, sizeof(ma)); child = device_add_child(dev, NULL, -1); + if (child == NULL) { + free(args, M_DEVBUF); + device_printf(dev, "%s: device_add_child failed", + __func__); + continue; + } device_set_ivars(child, args); + count++; + /* XXX should we break here or is it really possible + * to find more then one PHY ? */ } - if (child == NULL) + if (count == 0) return(ENXIO); device_set_desc(dev, "MII bus"); @@ -173,12 +187,15 @@ miibus_attach(dev) */ mii->mii_ifp = device_get_softc(device_get_parent(dev)); v = device_get_ivars(dev); + if (v == NULL) + return (ENXIO); ifmedia_upd = v[0]; ifmedia_sts = v[1]; + device_set_ivars(dev, NULL); + free(v, M_DEVBUF); ifmedia_init(&mii->mii_media, IFM_IMASK, ifmedia_upd, ifmedia_sts); - bus_generic_attach(dev); - return(0); + return (bus_generic_attach(dev)); } int @@ -186,8 +203,14 @@ miibus_detach(dev) device_t dev; { struct mii_data *mii; + void *v; bus_generic_detach(dev); + v = device_get_ivars(dev); + if (v != NULL) { + device_set_ivars(dev, NULL); + free(v, M_DEVBUF); + } mii = device_get_softc(dev); ifmedia_removeall(&mii->mii_media); mii->mii_ifp = NULL; @@ -305,12 +328,15 @@ mii_phy_probe(dev, child, ifmedia_upd, i int bmsr, i; v = malloc(sizeof(vm_offset_t) * 2, M_DEVBUF, M_NOWAIT); - if (v == 0) { + if (v == NULL) return (ENOMEM); - } v[0] = ifmedia_upd; v[1] = ifmedia_sts; *child = device_add_child(dev, "miibus", -1); + if (*child == NULL) { + free(v, M_DEVBUF); + return (ENXIO); + } device_set_ivars(*child, v); for (i = 0; i < MII_NPHY; i++) { @@ -324,14 +350,22 @@ mii_phy_probe(dev, child, ifmedia_upd, i } if (i == MII_NPHY) { + device_set_ivars(dev, NULL); + free(v, M_DEVBUF); device_delete_child(dev, *child); *child = NULL; return(ENXIO); } - bus_generic_attach(dev); + i = bus_generic_attach(dev); - return(0); + v = device_get_ivars(*child); + if (v != NULL) { + device_set_ivars(*child, NULL); + free(v, M_DEVBUF); + } + + return (i); } /* Index: mii_physubr.c =================================================================== RCS file: /local/mirror/FreeBSD/r/ncvs/src/sys/dev/mii/mii_physubr.c,v retrieving revision 1.21 diff -u -p -r1.21 mii_physubr.c --- mii_physubr.c 29 May 2004 18:09:10 -0000 1.21 +++ mii_physubr.c 23 Nov 2004 17:07:30 -0000 @@ -522,7 +522,13 @@ int mii_phy_detach(device_t dev) { struct mii_softc *sc; + void *args; + args = device_get_ivars(dev); + if (args != NULL) { + device_set_ivars(dev, NULL); + free(args, M_DEVBUF); + } sc = device_get_softc(dev); mii_phy_down(sc); sc->mii_dev = NULL; -- Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT