Date: Mon, 20 Dec 2004 12:23:39 +0000 (UTC) From: "Bjoern A. Zeeb" <bzeeb-lists@lists.zabbadoz.net> To: FreeBSD current mailing list <current@freebsd.org> Cc: FreeBSD net mailing list <net@freebsd.org> Subject: Re: mem leak in mii ? (fwd) Message-ID: <Pine.BSF.4.53.0412201219150.35048@e0-0.zab2.int.zabbadoz.net>
next in thread | raw e-mail | index | archive | help
Hi,
haven't had any feedback on this....
Can someone please review?
Also answers to the questions would be welcome.
Thanks.
---------- Forwarded message ----------
Date: Tue, 23 Nov 2004 19:31:10 +0000 (UTC)
From: Bjoern A. Zeeb <bzeeb-lists@lists.zabbadoz.net>
To: John Baldwin <jhb@FreeBSD.org>
Cc: Bjoern A. Zeeb <bzeeb-lists@lists.zabbadoz.net>,
freebsd-current@FreeBSD.org
Subject: Re: mem leak in mii ?
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
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.53.0412201219150.35048>