Date: Tue, 04 Jun 2002 10:24:49 +0200 From: Andre Oppermann <oppermann@pipeline.ch> To: Ruslan Ermilov <ru@FreeBSD.org> Cc: Garrett Wollman <wollman@lcs.mit.edu>, net@FreeBSD.org Subject: Re: Bug in net/route.c function rtredirect() Message-ID: <3CFC7951.57285201@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> <20020604070053.GA83399@sunbay.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Ruslan Ermilov wrote: > > On Tue, Jun 04, 2002 at 12:05:51AM +0200, Andre Oppermann wrote: > > 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). > > > 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): This breaks if someone hacks up the routing table. The vast majority is using rtfree() (actually the RTFREE macro which checks ==0 first). To be entirely correct and clean the macro RTFREE would be the right thing to do here. > %%% > Index: net/route.c > =================================================================== > 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 |= RTF_GATEWAY | RTF_DYNAMIC; > bzero((caddr_t)&info, sizeof(info)); > info.rti_info[RTAX_DST] = dst; > %%% > > > A bug is that host routes created by redirect are never being purged. > > But that one has been present for a long (?) time. > > > > 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. > > > This is handled by in_losing(). in_losing() has all the necessary > comments explaining what's going on here. Yes, I see what and why in_losing() is doing it. What I'm wondering is why are the packets lost so in_losing() is being triggered. In my net- work such packet loss is not supposed to happen. Otherwise I could only imagine getting a bugus redirect. That would be a problem on the router (which is also FreeBSD based). Anyway, I'll investigate my problem here further and see where that takes me... > > 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. > > > There may be only one reason: there's no (yet) route created by tcp. > I can't reproduce it here. In tcp_input.c is a route lookup (via tcp_rtlookup) in the function tcp_mssopt(). tcp_mssopt is supposed to look up the outgoing inter- face to find out the MSS this host supports. tcp_rtlookup rtalloc's a route and clones it if neccessary. This is before any redirect can happen because it's before we've sent out the packet which triggers the redirect. syncache_respond is doing the same. So there should be a route existing prior to the redirect?! (UDP is not doing this, it does not request a host route but simply takes whichever routes matches, usually default route). > > > 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? > > > > No. No bug as I said, no need to patch. Sorry for this touble. > > > > For the expiration of redirects I'll port/adapt the NetBSD solution > > and post the patch here. > > > We could treat RTF_DYNAMIC routes just like RTF_WASCLONED ones. > Seems to work just fine here: Ah, an even smarter solution! I'll try this on my box this evening! > %%% > Index: netinet/in_rmx.c > =================================================================== > 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)) != RTF_HOST) > return; > > - if((rt->rt_flags & (RTF_WASCLONED | RTPRF_OURS)) > - != RTF_WASCLONED) > + if((rt->rt_flags & RTPRF_OURS) != 0) > + return; > + > + if((rt->rt_flags & (RTF_WASCLONED | RTF_DYNAMIC)) == 0) > return; > > /* > %%% > > 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: > > > > > A bug is that host routes created by redirect are never being purged. > > > But that one has been present for a long (?) time. > > > > 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. > > > 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. /me thinks this is fine. This reminds of a problem I had in August 2001 with the .CH secondary nameserver ccTLD.tix.ch where I've got more than 303'000 redirects in the routing table. The thread was named "303,000 routes in kernel". Thankfully the network code is quite robust and didn't fall over when it could no longer allocate memory for new routes. -- Andre 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?3CFC7951.57285201>