cvs commit: src/sys/net if_gif.c
Ruslan Ermilov
ru at freebsd.org
Sat Apr 3 00:57:01 PST 2004
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
> > > >
> > > > FreeBSD src repository
> > > >
> > > > Modified files:
> > > > sys/net if_gif.c
> > > > Log:
> > > > Move "called", a static function variable used to detect recursive
> > > > processing with gif interfaces, to a global variable named "gif_called".
> > > > Add an annotation that this approach will not work with a reentrant
> > > > network stack, and that we should instead use packet tags to detect
> > > > excessive recursive processing.
> > > >
> > > > Revision Changes Path
> > > > 1.42 +11 -4 src/sys/net/if_gif.c
> > > >
> > > 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.
> >
> > 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.
> >
> > http://www.openbsd.org/cgi-bin/cvsweb/src/sys/net/if_gif.c.diff?r1=1.18&r2=1.19
> >
> Good idea. I will implement it and repost the updated patch here.
>
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,
--
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
Index: if_gif.c
===================================================================
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"
/*
- * 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;
-/*
- * 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 = 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 = (struct gif_softc*)ifp;
+ struct m_tag *mtag;
int error = 0;
+ int gif_called;
#ifdef MAC
error = mac_check_ifnet_transmit(ifp, m);
@@ -359,14 +352,26 @@ gif_output(ifp, m, dst, rt)
/*
* 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 = 1;
+ mtag = m_tag_locate(m, MTAG_GIF, MTAG_GIF_CALLED, NULL);
+ while (mtag != NULL) {
+ if (*(struct ifnet **)(mtag + 1) == ifp) {
+ log(LOG_NOTICE,
+ "gif_output: loop detected on %s\n",
+ (*(struct ifnet **)(mtag + 1))->if_xname);
+ m_freem(m);
+ error = EIO; /* is there better errno? */
+ goto end;
+ }
+ mtag = 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 = EIO; /* is there better errno? */
goto end;
}
- mtx_unlock(&gif_mtx);
+ mtag = m_tag_alloc(MTAG_GIF, MTAG_GIF_CALLED, sizeof(struct ifnet *),
+ M_NOWAIT);
+ if (mtag == NULL) {
+ m_freem(m);
+ error = ENOMEM;
+ goto end;
+ }
+ *(struct ifnet **)(mtag + 1) = ifp;
+ m_tag_prepend(m, mtag);
m->m_flags &= ~(M_BCAST|M_MCAST);
if (!(ifp->if_flags & IFF_UP) ||
@@ -414,9 +427,6 @@ gif_output(ifp, m, dst, rt)
}
end:
- mtx_lock(&gif_mtx);
- gif_called = 0; /* reset recursion counter */
- mtx_unlock(&gif_mtx);
if (error)
ifp->if_oerrors++;
return error;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20040403/8591aee0/attachment.bin
More information about the cvs-src
mailing list