Re: mem leak in mii ? (fwd)

bzeeb-lists_at_lists.zabbadoz.net

Date: Sat, 22 Jan 2005 09:41:25 +0000 (UTC)
To: FreeBSD current mailing list <current@freebsd.org>


-- 
Bjoern A. Zeeb				bzeeb at Zabbadoz dot NeT
---------- Forwarded message ----------
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)
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;
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"


Re: mem leak in mii ? (fwd)

Relevant Pages