On Tue, Jun 04, 2002 at 12:05:51AM +0200, Andre Oppermann wrote: > Ruslan Ermilov wrote: > > > > 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 = 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. > > 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):
%%%
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.
> 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.
> > 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:
%%%
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 <[EMAIL PROTECTED]> 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.
Cheers,
--
Ruslan Ermilov Sysadmin and DBA,
[EMAIL PROTECTED] Sunbay Software AG,
[EMAIL PROTECTED] FreeBSD committer,
+380.652.512.251 Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
msg06227/pgp00000.pgp
Description: PGP signature
