From owner-freebsd-net@FreeBSD.ORG Mon Oct 31 18:40:13 2011 Return-Path: Delivered-To: freebsd-net@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 71F501065673 for ; Mon, 31 Oct 2011 18:40:13 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 60E4A8FC18 for ; Mon, 31 Oct 2011 18:40:13 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id p9VIeDV3084146 for ; Mon, 31 Oct 2011 18:40:13 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.4/8.14.4/Submit) id p9VIeDFX084145; Mon, 31 Oct 2011 18:40:13 GMT (envelope-from gnats) Date: Mon, 31 Oct 2011 18:40:13 GMT Message-Id: <201110311840.p9VIeDFX084145@freefall.freebsd.org> To: freebsd-net@FreeBSD.org From: Cc: Subject: RE: misc/162201: [patch] multicast forwarding cache hash always allocated with size 0, resulting in buffer overrun X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Stevan_Markovic@McAfee.com List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 31 Oct 2011 18:40:13 -0000 The following reply was made to PR kern/162201; it has been noted by GNATS. From: To: Cc: , , , 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.