Date: Sat, 3 Apr 2004 11:56:51 +0300 From: Ruslan Ermilov <ru@freebsd.org> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/net if_gif.c Message-ID: <20040403085651.GA96868@ip.net.ua> In-Reply-To: <20040331072756.GA14337@ip.net.ua> References: <200403221424.i2MEOQYK057524@repoman.freebsd.org> <20040330220349.GA97921@ip.net.ua> <20040330232517.GA19416@Odin.AC.HMC.Edu> <20040331072756.GA14337@ip.net.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--v9Ux+11Zm5mwPlX6 Content-Type: multipart/mixed; boundary="a8Wt8u1KmwUX3Y2C" Content-Disposition: inline --a8Wt8u1KmwUX3Y2C Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Mar 31, 2004 at 10:27:56AM +0300, Ruslan Ermilov wrote: > On Tue, Mar 30, 2004 at 03:25:17PM -0800, Brooks Davis wrote: > > On Wed, Mar 31, 2004 at 01:03:49AM +0300, Ruslan Ermilov wrote: > > > On Mon, Mar 22, 2004 at 06:24:26AM -0800, Robert Watson wrote: > > > > rwatson 2004/03/22 06:24:26 PST > > > >=20 > > > > FreeBSD src repository > > > >=20 > > > > Modified files: > > > > sys/net if_gif.c=20 > > > > Log: > > > > Move "called", a static function variable used to detect recursive > > > > processing with gif interfaces, to a global variable named "gif_c= alled". > > > > Add an annotation that this approach will not work with a reentra= nt > > > > network stack, and that we should instead use packet tags to dete= ct > > > > excessive recursive processing. > > > > =20 > > > > Revision Changes Path > > > > 1.42 +11 -4 src/sys/net/if_gif.c > > > >=20 > > > Implemented this in the attached patch. Note when testing: setting > > > net.link.gif.max_nesting too high (>20 on my system) and triggering > > > the recursion causes the kernel stack exhaustion. > >=20 > > Why not just do what OpenBSD does and do actual loop detection? This > > gets rid of the nesting count hack which isn't really what you want to > > measure anyway. > >=20 > > http://www.openbsd.org/cgi-bin/cvsweb/src/sys/net/if_gif.c.diff?r1=3D1.= 18&r2=3D1.19 > >=20 > Good idea. I will implement it and repost the updated patch here. >=20 Actually, just replacing nesting limiter with loop detection was a bad idea, so I didn't follow it. It's a bad idea because you might have many nesting (but not looping) gif interfaces, and this will still cause panic by exhausting the kernel stack. Instead, I have combined both checks. Please review the attached patch. Cheers, --=20 Ruslan Ermilov ru@FreeBSD.org FreeBSD committer --a8Wt8u1KmwUX3Y2C Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=p Content-Transfer-Encoding: quoted-printable Index: if_gif.c =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 RCS file: /home/ncvs/src/sys/net/if_gif.c,v retrieving revision 1.43 diff -u -p -r1.43 if_gif.c --- if_gif.c 22 Mar 2004 15:43:14 -0000 1.43 +++ if_gif.c 3 Apr 2004 08:53:34 -0000 @@ -84,22 +84,13 @@ #define GIFNAME "gif" =20 /* - * gif_mtx protects the global gif_softc_list, and access to gif_called. - * XXX: See comment blow on gif_called. + * gif_mtx protects the global gif_softc_list. * XXX: Per-softc locking is still required. */ static struct mtx gif_mtx; static MALLOC_DEFINE(M_GIF, "gif", "Generic Tunnel Interface"); static LIST_HEAD(, gif_softc) gif_softc_list; =20 -/* - * XXX: gif_called is a recursion counter to prevent misconfiguration to - * cause unbounded looping in the network stack. However, this is a flawed - * approach as it assumes non-reentrance in the stack. This should be - * changed to use packet tags to track recusion.. - */ -static int gif_called =3D 0; - void (*ng_gif_input_p)(struct ifnet *ifp, struct mbuf **mp, int af); void (*ng_gif_input_orphan_p)(struct ifnet *ifp, struct mbuf *m, int af); void (*ng_gif_attach_p)(struct ifnet *ifp); @@ -347,7 +338,9 @@ gif_output(ifp, m, dst, rt) struct rtentry *rt; /* added in net2 */ { struct gif_softc *sc =3D (struct gif_softc*)ifp; + struct m_tag *mtag; int error =3D 0; + int gif_called; =20 #ifdef MAC error =3D mac_check_ifnet_transmit(ifp, m); @@ -359,14 +352,26 @@ gif_output(ifp, m, dst, rt) =20 /* * gif may cause infinite recursion calls when misconfigured. + * We'll prevent this by detecting loops. + * + * Many nesting levels may cause stack exhaustion. * We'll prevent this by introducing upper limit. - * XXX: this mechanism may introduce another problem about - * mutual exclusion of the variable CALLED, especially if we - * use kernel thread. */ - mtx_lock(&gif_mtx); - if (++gif_called > max_gif_nesting) { - mtx_unlock(&gif_mtx); + gif_called =3D 1; + mtag =3D m_tag_locate(m, MTAG_GIF, MTAG_GIF_CALLED, NULL); + while (mtag !=3D NULL) { + if (*(struct ifnet **)(mtag + 1) =3D=3D ifp) { + log(LOG_NOTICE, + "gif_output: loop detected on %s\n", + (*(struct ifnet **)(mtag + 1))->if_xname); + m_freem(m); + error =3D EIO; /* is there better errno? */ + goto end; + } + mtag =3D m_tag_locate(m, MTAG_GIF, MTAG_GIF_CALLED, mtag); + gif_called++; + } + if (gif_called > max_gif_nesting) { log(LOG_NOTICE, "gif_output: recursively called too many times(%d)\n", gif_called); @@ -374,7 +379,15 @@ gif_output(ifp, m, dst, rt) error =3D EIO; /* is there better errno? */ goto end; } - mtx_unlock(&gif_mtx); + mtag =3D m_tag_alloc(MTAG_GIF, MTAG_GIF_CALLED, sizeof(struct ifnet *), + M_NOWAIT); + if (mtag =3D=3D NULL) { + m_freem(m); + error =3D ENOMEM; + goto end; + } + *(struct ifnet **)(mtag + 1) =3D ifp; + m_tag_prepend(m, mtag); =20 m->m_flags &=3D ~(M_BCAST|M_MCAST); if (!(ifp->if_flags & IFF_UP) || @@ -414,9 +427,6 @@ gif_output(ifp, m, dst, rt) } =20 end: - mtx_lock(&gif_mtx); - gif_called =3D 0; /* reset recursion counter */ - mtx_unlock(&gif_mtx); if (error) ifp->if_oerrors++; return error; --a8Wt8u1KmwUX3Y2C-- --v9Ux+11Zm5mwPlX6 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (FreeBSD) iD8DBQFAbnxTUkv4P6juNwoRApE6AJ9q1tsooqhuLfwUOpCkT9xB98JaYQCfdkzF NXkipO4Q8VUr8felbjksoqA= =SixK -----END PGP SIGNATURE----- --v9Ux+11Zm5mwPlX6--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040403085651.GA96868>