Subject: Re: kern/3508: [dM] ipforward_rt cache broken
To: None <gnats-bugs@gnats.netbsd.org>
From: der Mouse <mouse@Rodents.Montreal.QC.CA>
List: netbsd-bugs
Date: 04/21/1997 10:56:02
> >Fix:
> 	Not sure what the right fix is.  [...]

Here's a patch I'm using.  It seems to cure the problem for me.  The
patch is relative to 1.2's source, $NetBSD: ip_input.c,v 1.30
1996/03/16 23:53:58 christos Exp $.

This patch amounts to clearing ipforward_rt once a second, but does it
in a way that doesn't carry the overhead of a heartbeat timeout or
anything similar: it simply tosses a cached route if time.tv_sec has
changed since it was cached.

This should be fixed up a little if this fix is adopted; in particular,
the declaration for ipforward_rt_stamp is gcc-specific.  (I just now
realized it can be written as "typeof(time.tv_sec)" instead of the
horrible mess I have there.)

--- OLD/sys/netinet/ip_input.c	Thu Jan  1 00:00:00 1970
+++ NEW/sys/netinet/ip_input.c	Thu Jan  1 00:00:00 1970
@@ -145,6 +145,7 @@
 
 struct	sockaddr_in ipaddr = { sizeof(ipaddr), AF_INET };
 struct	route ipforward_rt;
+typeof(((typeof(&time))0)->tv_sec) ipforward_rt_stamp;
 
 /*
  * Ip input routine.  Checksum and byte swap header.  If fragmented
@@ -830,28 +831,51 @@
 }
 
 /*
- * Given address of next destination (final or next hop),
- * return internet address info of interface to be used to get there.
+ * Get, into ipforward_rt, the route to *to.  If there is no route
+ *  already in ipforward_rt, or it's not to the same place, or it's too
+ *  old, toss it and re-lookup.  (The last clause is to limit the
+ *  damage done by caching a route and then having the routing tables
+ *  change.  Without a timeout here, or clearing this route when the
+ *  routing tables change, a route can be cached and then used much
+ *  later, long after it's been rendered bogus by a change to the
+ *  tables.)
  */
-struct in_ifaddr *
-ip_rtaddr(dst)
-	 struct in_addr dst;
+static void
+get_forward_rt(struct in_addr *to)
 {
 	register struct sockaddr_in *sin;
 
 	sin = satosin(&ipforward_rt.ro_dst);
 
-	if (ipforward_rt.ro_rt == 0 || dst.s_addr != sin->sin_addr.s_addr) {
+	if ((ipforward_rt.ro_rt == 0) ||
+	    (to->s_addr != sin->sin_addr.s_addr) ||
+	    (ipforward_rt_stamp != time.tv_sec)) {
+
 		if (ipforward_rt.ro_rt) {
 			RTFREE(ipforward_rt.ro_rt);
 			ipforward_rt.ro_rt = 0;
 		}
+
 		sin->sin_family = AF_INET;
 		sin->sin_len = sizeof(*sin);
-		sin->sin_addr = dst;
+		sin->sin_addr = *to;
 
 		rtalloc(&ipforward_rt);
+
+		ipforward_rt_stamp = time.tv_sec;
 	}
+}
+
+/*
+ * Given address of next destination (final or next hop),
+ * return internet address info of interface to be used to get there.
+ */
+struct in_ifaddr *
+ip_rtaddr(dst)
+	 struct in_addr dst;
+{
+	get_forward_rt(&dst);
+
 	if (ipforward_rt.ro_rt == 0)
 		return ((struct in_ifaddr *)0);
 	return (ifatoia(ipforward_rt.ro_rt->rt_ifa));
@@ -1005,7 +1029,6 @@
 	int srcrt;
 {
 	register struct ip *ip = mtod(m, struct ip *);
-	register struct sockaddr_in *sin;
 	register struct rtentry *rt;
 	int error, type = 0, code = 0;
 	struct mbuf *mcopy;
@@ -1030,23 +1053,12 @@
 	}
 	ip->ip_ttl -= IPTTLDEC;
 
-	sin = satosin(&ipforward_rt.ro_dst);
-	if ((rt = ipforward_rt.ro_rt) == 0 ||
-	    ip->ip_dst.s_addr != sin->sin_addr.s_addr) {
-		if (ipforward_rt.ro_rt) {
-			RTFREE(ipforward_rt.ro_rt);
-			ipforward_rt.ro_rt = 0;
-		}
-		sin->sin_family = AF_INET;
-		sin->sin_len = sizeof(*sin);
-		sin->sin_addr = ip->ip_dst;
+	get_forward_rt(&ip->ip_dst);
+	rt = ipforward_rt.ro_rt;
 
-		rtalloc(&ipforward_rt);
-		if (ipforward_rt.ro_rt == 0) {
-			icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_HOST, dest, 0);
-			return;
-		}
-		rt = ipforward_rt.ro_rt;
+	if (rt == 0) {
+		icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_HOST, dest, 0);
+		return;
 	}
 
 	/*

					der Mouse

			       mouse@rodents.montreal.qc.ca
		     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B