Date: Sat, 01 Jun 2002 12:49:51 +0200 From: Andre Oppermann <oppermann@pipeline.ch> To: Ruslan Ermilov <ru@FreeBSD.ORG> Cc: freebsd-net@FreeBSD.ORG, silby@silby.com Subject: Re: Bug in net/route.c function rtredirect() Message-ID: <3CF8A6CF.6033679A@pipeline.ch> References: <3CF81BC5.EAF0ACC@pipeline.ch> <20020601095707.GC50435@sunbay.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3CF8A6CF.6033679A>