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
_at__at_ -111,7 +111,7 _at__at_ 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);
_at__at_ -145,12 +145,26 _at__at_ 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");
_at__at_ -173,12 +187,15 _at__at_ 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
_at__at_ -186,8 +203,14 _at__at_ 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;
_at__at_ -305,12 +328,15 _at__at_ 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++) {
_at__at_ -324,14 +350,22 _at__at_ 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
_at__at_ -522,7 +522,13 _at__at_ 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