On Sat, Jun 01, 2002 at 12:49:51PM +0200, Andre Oppermann wrote: > 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. > No it won't be done in "done:", because a few lines later we assign rt = NULL:
create:
if (rt)
rt->rt_refcnt--;
flags |= RTF_GATEWAY | RTF_DYNAMIC;
bzero((caddr_t)&info, sizeof(info));
info.rti_info[RTAX_DST] = dst;
info.rti_info[RTAX_GATEWAY] = gateway;
info.rti_info[RTAX_NETMASK] = netmask;
info.rti_ifa = ifa;
info.rti_flags = flags;
rt = NULL;
> > > 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 second rt->rt_refcnt-- I added is indeed unnecessary, you're
right here.
> > > 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.
>
I will think about that.
> 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.
>
> --- 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 {
> /*
Heh, so you in fact tried to rtfree() "rt" in "done:" by adding
"rtn". And how *rtp (if rtp != NULL) will become "rtn" then?
What about this?
%%%
Index: route.c
===================================================================
RCS file: /home/ncvs/src/sys/net/route.c,v
retrieving revision 1.69
diff -u -p -r1.69 route.c
--- route.c 19 Mar 2002 21:54:18 -0000 1.69
+++ route.c 2 Jun 2002 12:49:16 -0000
@@ -345,7 +345,7 @@ rtredirect(dst, gateway, netmask, flags,
*/
create:
if (rt)
- rtfree(rt);
+ rt->rt_refcnt--;
flags |= RTF_GATEWAY | RTF_DYNAMIC;
bzero((caddr_t)&info, sizeof(info));
info.rti_info[RTAX_DST] = dst;
%%%
Cheers,
--
Ruslan Ermilov Sysadmin and DBA,
[EMAIL PROTECTED] Sunbay Software AG,
[EMAIL PROTECTED] FreeBSD committer,
+380.652.512.251 Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
msg06193/pgp00000.pgp
Description: PGP signature
