Date: Sun, 2 Jun 2002 15:53:10 +0300 From: Ruslan Ermilov <ru@FreeBSD.ORG> To: Andre Oppermann <oppermann@pipeline.ch> Cc: freebsd-net@FreeBSD.ORG, silby@silby.com Subject: Re: Bug in net/route.c function rtredirect() Message-ID: <20020602125310.GD58857@sunbay.com> In-Reply-To: <3CF8A6CF.6033679A@pipeline.ch> References: <3CF81BC5.EAF0ACC@pipeline.ch> <20020601095707.GC50435@sunbay.com> <3CF8A6CF.6033679A@pipeline.ch>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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,
ru@sunbay.com Sunbay Software AG,
ru@FreeBSD.org FreeBSD committer,
+380.652.512.251 Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (FreeBSD)
iD8DBQE8+hU2Ukv4P6juNwoRAnlUAKCB95oIsFB7PgklcPqyzV+br7BjtQCcDQPH
XkraGXh4yJ0IDFtXA+R9qsI=
=KrTi
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020602125310.GD58857>