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
--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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020602125310.GD58857>