Date: Mon, 31 Oct 2011 18:40:13 GMT From: <Stevan_Markovic@McAfee.com> To: freebsd-net@FreeBSD.org Subject: RE: misc/162201: [patch] multicast forwarding cache hash always allocated with size 0, resulting in buffer overrun Message-ID: <201110311840.p9VIeDFX084145@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/162201; it has been noted by GNATS.
From: <Stevan_Markovic@McAfee.com>
To: <glebius@FreeBSD.org>
Cc: <freebsd-gnats-submit@FreeBSD.org>, <zec@FreeBSD.org>, <bms@FreeBSD.org>,
<bz@FreeBSD.org>
Subject: RE: misc/162201: [patch] multicast forwarding cache hash always
allocated with size 0, resulting in buffer overrun
Date: Mon, 31 Oct 2011 13:26:43 -0500
Hi,
Gleb, no, I have not tried to eliminate VNET_SYSINITS and I do not think it=
can be done. My understanding is that VNET_SYSINIT initializes virtual net=
work stack instance specific data. Eliminating it would prevent using multi=
cast in multiple virtual network stacks.=20
Stevan=20
-----Original Message-----
From: Gleb Smirnoff [mailto:glebius@FreeBSD.org]=20
Sent: Monday, October 31, 2011 2:10 PM
To: Markovic, Stevan
Cc: freebsd-gnats-submit@FreeBSD.org; zec@FreeBSD.org; bms@FreeBSD.org; bz@=
FreeBSD.org
Subject: Re: misc/162201: [patch] multicast forwarding cache hash always al=
located with size 0, resulting in buffer overrun
Hi,
On Mon, Oct 31, 2011 at 04:22:00PM +0000, Stevan Markovic wrote:
S> >Description:
S> Bug is observed as kernel panic shortly after stopping XORP (www.xorp.or=
g) configured for PIM/SM routing. Debugging discovered that at the time of =
MALLOC for V_nexpire in ip_mroute.c::vnet_mroute_init() size variable mfcha=
shsize always has value 0.=20
S>=20
S> Why: variable mfchashsize is initialized in module event handler which i=
s executed with SI_ORDER_ANY ordering tag which happens _after_ variable u=
sage in MALLOC in VNET_SYSINIT with SI_ORDER_MIDDLE.
S>=20
S> Fix simply moves variable initialization before its usage in vnet_mroute=
_init.
S>=20
S> This bug is discovered and fixed in McAfee Inc development.
S> >How-To-Repeat:
S> Hard to reproduce since system behavior after memory overwrite is unpred=
ictable. Multicast forwarding cashe hash overrun always happens after:
S> a) configuring xorp to use PIM/SM
S> b) starting xorp_rtrmgr
S> c) stopping xorp_rtrmgr.
S>=20
S> >Fix:
S> Fix simply moves mfchashsize variable initialization before its usage in=
vnet_mroute_init.
S>=20
S> Patch attached with submission follows:
S>=20
S> Index: ip_mroute.c
S> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
S> RCS file: /projects/freebsd/src_cvsup/src/sys/netinet/ip_mroute.c,v
S> retrieving revision 1.161
S> diff -u -r1.161 ip_mroute.c
S> --- ip_mroute.c 22 Nov 2010 19:32:54 -0000 1.161
S> +++ ip_mroute.c 31 Oct 2011 15:54:53 -0000
S> @@ -2814,7 +2814,13 @@
S> static void
S> vnet_mroute_init(const void *unused __unused)
S> {
S> -
S> + mfchashsize =3D MFCHASHSIZE;
S> + if (TUNABLE_ULONG_FETCH("net.inet.ip.mfchashsize", &mfchashsize) &&
S> + !powerof2(mfchashsize)) {
S> + printf("WARNING: %s not a power of 2; using default\n",
S> + "net.inet.ip.mfchashsize");
S> + mfchashsize =3D MFCHASHSIZE;
S> + }
S> MALLOC(V_nexpire, u_char *, mfchashsize, M_MRTABLE, M_WAITOK|M_ZERO);
S> bzero(V_bw_meter_timers, sizeof(V_bw_meter_timers));
S> callout_init(&V_expire_upcalls_ch, CALLOUT_MPSAFE);
S> @@ -2855,13 +2861,6 @@
S> MFC_LOCK_INIT();
S> VIF_LOCK_INIT();
S> =20
S> - mfchashsize =3D MFCHASHSIZE;
S> - if (TUNABLE_ULONG_FETCH("net.inet.ip.mfchashsize", &mfchashsize) &&
S> - !powerof2(mfchashsize)) {
S> - printf("WARNING: %s not a power of 2; using default\n",
S> - "net.inet.ip.mfchashsize");
S> - mfchashsize =3D MFCHASHSIZE;
S> - }
S> =20
S> pim_squelch_wholepkt =3D 0;
S> TUNABLE_ULONG_FETCH("net.inet.pim.squelch_wholepkt",
Have you tried to remove these VNET_SYSINITs at all and do all the
initialization in the ip_mroute_modevent() itself? From first glance
I see no reason for separate malloc() + callout_init()s.
I am putting guys, who made and reviewed the commit, into Cc.
--=20
Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201110311840.p9VIeDFX084145>