From owner-freebsd-net Sat Jun 1 3:51:12 2002 Delivered-To: freebsd-net@freebsd.org Received: from mailtoaster1.pipeline.ch (mailtoaster1.pipeline.ch [62.48.0.70]) by hub.freebsd.org (Postfix) with SMTP id 2C74937B405 for ; Sat, 1 Jun 2002 03:51:04 -0700 (PDT) Received: (qmail 58573 invoked from network); 1 Jun 2002 10:50:41 -0000 Received: from unknown (HELO pipeline.ch) ([62.48.0.54]) (envelope-sender ) by mailtoaster1.pipeline.ch (qmail-ldap-1.03) with SMTP for ; 1 Jun 2002 10:50:41 -0000 Message-ID: <3CF8A6CF.6033679A@pipeline.ch> Date: Sat, 01 Jun 2002 12:49:51 +0200 From: Andre Oppermann X-Mailer: Mozilla 4.76 [en] (Windows NT 5.0; U) X-Accept-Language: en MIME-Version: 1.0 To: Ruslan Ermilov Cc: freebsd-net@FreeBSD.ORG, silby@silby.com Subject: Re: Bug in net/route.c function rtredirect() References: <3CF81BC5.EAF0ACC@pipeline.ch> <20020601095707.GC50435@sunbay.com> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org Ruslan Ermilov wrote: > > On Sat, Jun 01, 2002 at 02:56:37AM +0200, Andre Oppermann wrote: > > Hi all, > > > > I've found another bug in net/route.c in the function rtredirect(). > > > > When learning a new gateway from a ICMP redirect icmp_input calls > > rtredirect() in net/route.c. rtredirect is doing some sanity checks > > and then creates, if it did not find an existing host route, a new > > host route tagged with the RTF_DYNAMIC flag. If there was an existing > > host route it'll simply adjust the gateway and set the RTF_MODIFIED > > flag. > > > OK. > > > If no pre-existing host route was found either the default route or > > an network route is being returned by the route table lookup in the > > beginning. Neither of these should be modified. It should simply add > > the new host route. > > > This route lookup also increments rt_refcnt. Yes. > > The bug is that the jump to "create:" (host route) tries to rtfree() > > the found default route or network route. If the refcount on those > > routes is one it frees it, otherwise it decrements it by one. This is > > wrong. There is no reason to decrement the refcount or to free the > > routes. The bug is even dangerous when you happen to have many > > redirects, one too much and you'll loose your default route. The > > refcount drifts away one decrement more with every valid redirecet > > received. > > > There _is_ the reason to decrement the refcount, because nothing is > going to point to it after we modify it, and if we don't decrement > it, this will create an "unremovable" route. You're right but the refcount decrementation will be done, just in a different place. In the "done:" section the rtfree() will be done also in the case when an existing host route was already present. > > The solution is to remove the rtfree() in rtredirect. Diff against > > -STABLE is attached. Should apply to -CURRENT with some fuzz. > > > No, please try the below patch instead. The rtfree() I removed is one too many and still has to go. Your patch doesn't change the problem, it just makes it different. Routes could go below zero refcount in your version. > > The bug has been introduced by ru in commit 1.67 "Pull post-4.4BSD > > change from BSD/OS 4.2". This bug is also present in NetBSD and was > > introduced there before here (1.67 commit is a copy from NetBSD). > > > Why you're not mailing me then? The bug was introduced first in > BSD/OS, FWIW. Sorry, Silby fixed the previous bug so I sent this to him again. > > I found this by observing a problem on a machine on my network where > > I have two routers. One is the default router for the machine. Some- > > times it'll get a redirect to use the second router and then the > > inconsistencies begun. I only really noticed this problem by using > > and watching the numbers of the tcp statistics code I posted yesterday > > (plus route -n monior). This is side-work for an overhaul of the > > kernel routing table. > > > I will see if I can reproduce this, on Monday. > > > While being in this area I noticed that host routes created by a > > redirect never time out and never get removed in a regular fashion. > > So if you get many of them they clutter the whole routing table. This > > can become dangerous if you get a lot tcp sessions and your default > > router is sub-optimal and redirects you all the time to the "real > > default" router on your network. There can be a redirected route for > > every host on the Internet. Bad if a new code-red or nimda wave comes > > by. NetBSD has implemented a purger for the redirect host routes. > > I'll have a look at it and provide a patch for that for FreeBSD next > > week. > > > Nice catch. > > Please try this patch, it's believed to fix both problems. It is > completely untested: I fail to see how you purge the redirect host routes. Actually, at the moment I have got some problems following the very twisted logic of all this... ;-) Lets have another look on monday. Anyway, patch attached which I think (at the moment) would be correct. -- Andre --- route.c.old Fri May 31 21:17:50 2002 +++ route.c Sat Jun 1 12:34:33 2002 @@ -299,7 +299,7 @@ int flags; struct rtentry **rtp; { - struct rtentry *rt; + struct rtentry *rt, *rtn; int error = 0; short *stat = 0; struct rt_addrinfo info; @@ -344,8 +344,6 @@ * Create new route, rather than smashing route to net. */ create: - if (rt) - rtfree(rt); flags |= RTF_GATEWAY | RTF_DYNAMIC; bzero((caddr_t)&info, sizeof(info)); info.rti_info[RTAX_DST] = dst; @@ -353,10 +351,10 @@ info.rti_info[RTAX_NETMASK] = netmask; info.rti_ifa = ifa; info.rti_flags = flags; - rt = NULL; - error = rtrequest1(RTM_ADD, &info, &rt); - if (rt != NULL) - flags = rt->rt_flags; + rtn = NULL; + error = rtrequest1(RTM_ADD, &info, &rtn); + if (rtn != NULL) + flags = rtn->rt_flags; stat = &rtstat.rts_dynamic; } else { /* To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message