Date: Tue, 4 Jun 2002 10:00:53 +0300 From: Ruslan Ermilov <ru@FreeBSD.org> To: Andre Oppermann <oppermann@pipeline.ch>, Garrett Wollman <wollman@lcs.mit.edu> Cc: net@FreeBSD.org Subject: Re: Bug in net/route.c function rtredirect() Message-ID: <20020604070053.GA83399@sunbay.com> In-Reply-To: <200206032237.g53Mb0h1018954@khavrinen.lcs.mit.edu> <3CFBE83F.3E7025A4@pipeline.ch> References: <20020601095707.GC50435@sunbay.com> <3CF8A6CF.6033679A@pipeline.ch> <20020602125310.GD58857@sunbay.com> <3CFBE83F.3E7025A4@pipeline.ch> <200206032237.g53Mb0h1018954@khavrinen.lcs.mit.edu> <3CF81BC5.EAF0ACC@pipeline.ch> <20020601095707.GC50435@sunbay.com> <3CF8A6CF.6033679A@pipeline.ch> <20020602125310.GD58857@sunbay.com> <3CFBE83F.3E7025A4@pipeline.ch>
next in thread | previous in thread | raw e-mail | index | archive | help
--UlVJffcvxoiEqYs2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jun 04, 2002 at 12:05:51AM +0200, Andre Oppermann wrote: > Ruslan Ermilov wrote: > >=20 > > On Sat, Jun 01, 2002 at 12:49:51PM +0200, Andre Oppermann wrote: > > > 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 =3D NULL: > >=20 > > 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; > >=20 > > > > > 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. >=20 > After reading this whole redirect stuff a couple of time I've come to > the conclusion that the function is right as it is there. There is no > such bug as I described it. The rtalloc1() in rtredirect() is incre- > menting the refcount on the route found, the two rtfree() after > "create:" and "done:" will decrement it as it is must happen (we don't > have a reference to that route because we don't clone here). >=20 Right. There's just no point in calling rtfree() just to decrement the route's rt_refcnt (we were only looking up the route, we didn't need a reference to it). rtfree() does not free the route because it's still RTF_UP, and calling rtfree() also needlessly calls rnh_close() (in_clsroute() in the PF_INET case). Hence this would still be a slight optimization (tested this time): %%% Index: net/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 --- net/route.c 19 Mar 2002 21:54:18 -0000 1.69 +++ net/route.c 4 Jun 2002 06:41:42 -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; %%% > A bug is that host routes created by redirect are never being purged. > But that one has been present for a long (?) time. >=20 > I'm still try to track a problem with the following situation: 1. I > get a tcp syn from somewhere, 2. a host route is created by tcp, 3. > the synack is sent back, 4. we get a redirect from the router to use > a different router, 5. host route created from tcp is updated and > replaced by icmp redirect route, 6. I see a RTM_LOSING message and > the redirect route is being purged. >=20 This is handled by in_losing(). in_losing() has all the necessary comments explaining what's going on here. > This happens in, I think, some 5% of the cases. What I'm tracking > is why the rtfree() after "create:" is de-refcounting the default > route when we should have updated the host route created by tcp > before. Maybe this is a side-effect of the tcp syncache and the > flow has changed? I'll track what happens there. >=20 There may be only one reason: there's no (yet) route created by tcp. I can't reproduce it here. > > 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? >=20 > No. No bug as I said, no need to patch. Sorry for this touble. >=20 > For the expiration of redirects I'll port/adapt the NetBSD solution > and post the patch here. >=20 We could treat RTF_DYNAMIC routes just like RTF_WASCLONED ones. Seems to work just fine here: %%% Index: netinet/in_rmx.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/netinet/in_rmx.c,v retrieving revision 1.42 diff -u -p -r1.42 in_rmx.c --- netinet/in_rmx.c 19 Mar 2002 21:25:46 -0000 1.42 +++ netinet/in_rmx.c 4 Jun 2002 06:41:42 -0000 @@ -202,8 +202,10 @@ in_clsroute(struct radix_node *rn, struc if((rt->rt_flags & (RTF_LLINFO | RTF_HOST)) !=3D RTF_HOST) return; =20 - if((rt->rt_flags & (RTF_WASCLONED | RTPRF_OURS)) - !=3D RTF_WASCLONED) + if((rt->rt_flags & RTPRF_OURS) !=3D 0) + return; + + if((rt->rt_flags & (RTF_WASCLONED | RTF_DYNAMIC)) =3D=3D 0) return; =20 /* %%% On Mon, Jun 03, 2002 at 06:37:00PM -0400, Garrett Wollman wrote: > <<On Tue, 04 Jun 2002 00:05:51 +0200, Andre Oppermann <oppermann@pipeline= .ch> said: >=20 > > A bug is that host routes created by redirect are never being purged. > > But that one has been present for a long (?) time. >=20 > You are expected to be running a routing process (like `routed' in > router-discovery mode) which will delete them for you. I agree that > this is not a reasonable expectation, but that was the design intent. >=20 I think there's nothing wrong if dynamically created routes (by redirect) will be treated as "temporary" routes, just like protocol-cloned routes. The above patch for in_rmx.c implements this. Let me know what do you think. 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 --UlVJffcvxoiEqYs2 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (FreeBSD) iD8DBQE8/GWkUkv4P6juNwoRAildAJ9D3/LSc2AZNpbRv/l21U8JU3B/fwCdEnIi GDg0ZpO+rYY3DaS0bWg7jfA= =bIb9 -----END PGP SIGNATURE----- --UlVJffcvxoiEqYs2-- 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?20020604070053.GA83399>