From owner-freebsd-net Sun Jun 2 5:53:56 2002 Delivered-To: freebsd-net@freebsd.org Received: from whale.sunbay.crimea.ua (whale.sunbay.crimea.ua [212.110.138.65]) by hub.freebsd.org (Postfix) with ESMTP id 0B21637B405 for ; Sun, 2 Jun 2002 05:53:36 -0700 (PDT) Received: (from ru@localhost) by whale.sunbay.crimea.ua (8.11.6/8.11.2) id g52CrAY62954; Sun, 2 Jun 2002 15:53:10 +0300 (EEST) (envelope-from ru) Date: Sun, 2 Jun 2002 15:53:10 +0300 From: Ruslan Ermilov To: Andre Oppermann Cc: freebsd-net@FreeBSD.ORG, silby@silby.com Subject: Re: Bug in net/route.c function rtredirect() Message-ID: <20020602125310.GD58857@sunbay.com> References: <3CF81BC5.EAF0ACC@pipeline.ch> <20020601095707.GC50435@sunbay.com> <3CF8A6CF.6033679A@pipeline.ch> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="KdquIMZPjGJQvRdI" Content-Disposition: inline In-Reply-To: <3CF8A6CF.6033679A@pipeline.ch> User-Agent: Mutt/1.3.99i 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 --KdquIMZPjGJQvRdI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jun 01, 2002 at 12:49:51PM +0200, Andre Oppermann wrote: > Ruslan Ermilov wrote: > >=20 > > 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. > >=20 > > > 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. >=20 > Yes. >=20 > > > 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. >=20 > 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. >=20 No it won't be done in "done:", because a few lines later we assign rt =3D NULL: create: if (rt) rt->rt_refcnt--; flags |=3D RTF_GATEWAY | RTF_DYNAMIC; bzero((caddr_t)&info, sizeof(info)); info.rti_info[RTAX_DST] =3D dst; info.rti_info[RTAX_GATEWAY] =3D gateway; info.rti_info[RTAX_NETMASK] =3D netmask; info.rti_ifa =3D ifa; info.rti_flags =3D flags; rt =3D 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. >=20 > 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. >=20 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. >=20 > Sorry, Silby fixed the previous bug so I sent this to him again. >=20 > > > 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. > >=20 > > > 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. > >=20 > > Please try this patch, it's believed to fix both problems. It is > > completely untested: >=20 > I fail to see how you purge the redirect host routes. >=20 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. >=20 > Anyway, patch attached which I think (at the moment) would be correct. >=20 > --- 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 =3D 0; > short *stat =3D 0; > struct rt_addrinfo info; > @@ -344,8 +344,6 @@ > * Create new route, rather than smashing route to net. > */ > create: > - if (rt) > - rtfree(rt); > flags |=3D RTF_GATEWAY | RTF_DYNAMIC; > bzero((caddr_t)&info, sizeof(info)); > info.rti_info[RTAX_DST] =3D dst; > @@ -353,10 +351,10 @@ > info.rti_info[RTAX_NETMASK] =3D netmask; > info.rti_ifa =3D ifa; > info.rti_flags =3D flags; > - rt =3D NULL; > - error =3D rtrequest1(RTM_ADD, &info, &rt); > - if (rt !=3D NULL) > - flags =3D rt->rt_flags; > + rtn =3D NULL; > + error =3D rtrequest1(RTM_ADD, &info, &rtn); > + if (rtn !=3D NULL) > + flags =3D rtn->rt_flags; > stat =3D &rtstat.rts_dynamic; > } else { > /* Heh, so you in fact tried to rtfree() "rt" in "done:" by adding "rtn". And how *rtp (if rtp !=3D NULL) will become "rtn" then? What about this? %%% Index: route.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D 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 |=3D RTF_GATEWAY | RTF_DYNAMIC; bzero((caddr_t)&info, sizeof(info)); info.rti_info[RTAX_DST] =3D dst; %%% Cheers, --=20 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 --KdquIMZPjGJQvRdI Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (FreeBSD) iD8DBQE8+hU2Ukv4P6juNwoRAnlUAKCB95oIsFB7PgklcPqyzV+br7BjtQCcDQPH XkraGXh4yJ0IDFtXA+R9qsI= =KrTi -----END PGP SIGNATURE----- --KdquIMZPjGJQvRdI-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message