Date: Wed, 29 Oct 2003 16:45:59 -0800 (PST) From: Sam Leffler <sam@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 40792 for review Message-ID: <200310300045.h9U0jxxP068743@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=40792 Change 40792 by sam@sam_ebb on 2003/10/29 16:45:01 Overhaul routing table entry cleanup. The previous logic was flawed. In some cases the reference to the table entry could be used after calling rtrequest(RTM_DELETE) when the entry might have been reclaimed. Also, in some cases, we needed to drop the lock on the entry in order to avoid a recursive lock in order to delete the entry. To correct these problems we introduce a new rtexpunge routine that takes a locked routing table reference and removes all traces to the entry. Normal callbacks to the protocol-specific close methods handle cleanup of ancillary state. This also fixes problems where the close routine might try to immediately delete the entry which would invalidate the callers' references (unbeknownst to them). One remaining issue is the test in the ARP code to reclaim dynamic references specially. This will trigger an assertion failure because it calls rtexpunge with rt_refcnt 1. It's unclear why this call is needed at all as the RTFREE call that immediately follows should call the close method to reclaim state. It may be that we need to move the test for the dynamic entry into the close method instead of doing it in the ARP code. Awaiting feedback on that. Affected files ... .. //depot/projects/netperf/sys/net/route.c#21 edit .. //depot/projects/netperf/sys/net/route.h#10 edit .. //depot/projects/netperf/sys/netinet/if_ether.c#13 edit .. //depot/projects/netperf/sys/netinet/in_pcb.c#8 edit .. //depot/projects/netperf/sys/netinet/in_rmx.c#10 edit .. //depot/projects/netperf/sys/netinet6/in6_ifattach.c#10 edit .. //depot/projects/netperf/sys/netinet6/in6_pcb.c#12 edit .. //depot/projects/netperf/sys/netinet6/in6_rmx.c#11 edit Differences ... ==== //depot/projects/netperf/sys/net/route.c#21 (text+ko) ==== @@ -31,7 +31,7 @@ * SUCH DAMAGE. * * @(#)route.c 8.3.1.1 (Berkeley) 2/23/95 - * $FreeBSD: src/sys/net/route.c,v 1.87 2003/10/16 16:17:17 sam Exp $ + * $FreeBSD: src/sys/net/route.c,v 1.88 2003/10/29 23:01:37 sam Exp $ */ #include "opt_inet.h" @@ -230,7 +230,17 @@ */ if (--rt->rt_refcnt > 0) goto done; - /* XXX refcount==0? */ + + /* + * On last reference give the "close method" a chance + * cleanup private state. This also permits (for IPv4 + * and IPv6) a chance to decide if the routing table + * entry should be purge immediately or at a later time. + * When an immediate purge is to happen the close routine + * typically call rtexpunge which clears the RTF_UP flag + * on the entry so that the following code reclaims the + * storage. + */ if (rt->rt_refcnt == 0 && rnh->rnh_close) rnh->rnh_close((struct radix_node *)rt, rnh); @@ -524,6 +534,82 @@ return (error); } +/* + * Expunges references to a route that's about to be reclaimed. + * The route must be locked and have no held references. + */ +void +rtexpunge(struct rtentry *rt) +{ + struct radix_node *rn; + struct radix_node_head *rnh; + struct ifaddr *ifa; + + RT_LOCK_ASSERT(rt); + KASSERT(rt->rt_refcnt == 0, ("bogus refcnt %ld", rt->rt_refcnt)); + + /* + * Find the correct routing tree to use for this Address Family + */ + rnh = rt_tables[rt_key(rt)->sa_family]; + KASSERT(rnh != 0, ("no table for af %u", rt_key(rt)->sa_family)); + + RADIX_NODE_HEAD_LOCK(rnh); + + /* + * Remove the item from the tree; it must be there. + */ + rn = rnh->rnh_deladdr(rt_key(rt), rt_mask(rt), rnh); + KASSERT(rn != 0, ("no table entry")); + KASSERT((rn->rn_flags & (RNF_ACTIVE | RNF_ROOT)) == 0, + ("unexpected flags 0x%x", rn->rn_flags)); + KASSERT(rt == (struct rtentry *)rn, + ("lookup mismatch, rt %p rn %p", rt, rn)); + + rt->rt_flags &= ~RTF_UP; + + /* + * Now search what's left of the subtree for any cloned + * routes which might have been formed from this node. + */ + if ((rt->rt_flags & (RTF_CLONING | RTF_PRCLONING)) && rt_mask(rt)) + rnh->rnh_walktree_from(rnh, rt_key(rt), rt_mask(rt), + rt_fixdelete, rt); + + /* + * Remove any external references we may have. + * This might result in another rtentry being freed if + * we held its last reference. + */ + if (rt->rt_gwroute) { + struct rtentry *gwrt = rt->rt_gwroute; + RTFREE(gwrt); + rt->rt_gwroute = 0; + } + + /* + * give the protocol a chance to keep things in sync. + */ + if ((ifa = rt->rt_ifa) && ifa->ifa_rtrequest) { + struct rt_addrinfo info; + + bzero((caddr_t)&info, sizeof(info)); + info.rti_flags = rt->rt_flags; + info.rti_info[RTAX_DST] = rt_key(rt); + info.rti_info[RTAX_GATEWAY] = rt->rt_gateway; + info.rti_info[RTAX_NETMASK] = rt_mask(rt); + ifa->ifa_rtrequest(RTM_DELETE, rt, &info); + } + + /* + * one more rtentry floating around that is not + * linked to the routing table. + */ + rttrash++; + + RADIX_NODE_HEAD_UNLOCK(rnh); +} + int rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt) { @@ -684,12 +770,8 @@ */ rt2 = rtalloc1(dst, 0, RTF_PRCLONING); if (rt2 && rt2->rt_parent) { - RT_UNLOCK(rt2); /* XXX recursive lock */ - rtrequest(RTM_DELETE, - rt_key(rt2), - rt2->rt_gateway, - rt_mask(rt2), rt2->rt_flags, 0); - RTFREE(rt2); + rtexpunge(rt2); + RT_UNLOCK(rt2); rn = rnh->rnh_addaddr(ndst, netmask, rnh, rt->rt_nodes); } else if (rt2) { ==== //depot/projects/netperf/sys/net/route.h#10 (text+ko) ==== @@ -297,6 +297,7 @@ void rtalloc_ign(struct route *, u_long); /* NB: the rtentry is returned locked */ struct rtentry *rtalloc1(struct sockaddr *, int, u_long); +void rtexpunge(struct rtentry *); void rtfree(struct rtentry *); int rtinit(struct ifaddr *, int, int); int rtioctl(u_long, caddr_t); ==== //depot/projects/netperf/sys/netinet/if_ether.c#13 (text+ko) ==== @@ -949,14 +949,9 @@ * arplookup() is creating the route, then purge * it from the routing table as it is probably bogus. */ - RT_UNLOCK(rt); - if (rt->rt_refcnt == 1 && ISDYNCLONE(rt)) { - rtrequest(RTM_DELETE, - (struct sockaddr *)rt_key(rt), - rt->rt_gateway, rt_mask(rt), - rt->rt_flags, 0); - } - RTFREE(rt); + if (rt->rt_refcnt == 1 && ISDYNCLONE(rt)) + rtexpunge(rt); + RTFREE_LOCKED(rt); return (0); #undef ISDYNCLONE } else { ==== //depot/projects/netperf/sys/netinet/in_pcb.c#8 (text+ko) ==== @@ -871,11 +871,9 @@ info.rti_info[RTAX_GATEWAY] = rt->rt_gateway; info.rti_info[RTAX_NETMASK] = rt_mask(rt); rt_missmsg(RTM_LOSING, &info, rt->rt_flags, 0); - if (rt->rt_flags & RTF_DYNAMIC) { - RT_UNLOCK(rt); /* XXX refcnt? */ - (void) rtrequest1(RTM_DELETE, &info, NULL); - } else - rtfree(rt); + if (rt->rt_flags & RTF_DYNAMIC) + rtexpunge(rt); + RTFREE_LOCKED(rt); /* * A new route can be allocated * the next time output is attempted. ==== //depot/projects/netperf/sys/netinet/in_rmx.c#10 (text+ko) ==== @@ -125,17 +125,12 @@ rt2->rt_flags & RTF_HOST && rt2->rt_gateway && rt2->rt_gateway->sa_family == AF_LINK) { - /* NB: must unlock to avoid recursion */ - RT_UNLOCK(rt2); - rtrequest(RTM_DELETE, - (struct sockaddr *)rt_key(rt2), - rt2->rt_gateway, rt_mask(rt2), - rt2->rt_flags, 0); + rtexpunge(rt2); + RTFREE_LOCKED(rt2); ret = rn_addroute(v_arg, n_arg, head, treenodes); - RT_LOCK(rt2); - } - RTFREE_LOCKED(rt2); + } else + RTFREE_LOCKED(rt2); } } @@ -211,13 +206,7 @@ rt->rt_flags |= RTPRF_OURS; rt->rt_rmx.rmx_expire = time_second + rtq_reallyold; } else { - /* NB: must unlock to avoid recursion */ - RT_UNLOCK(rt); - rtrequest(RTM_DELETE, - (struct sockaddr *)rt_key(rt), - rt->rt_gateway, rt_mask(rt), - rt->rt_flags, 0); - RT_LOCK(rt); + rtexpunge(rt); } } @@ -385,7 +374,6 @@ { struct in_ifadown_arg *ap = xap; struct rtentry *rt = (struct rtentry *)rn; - int err; if (rt->rt_ifa == ap->ifa && (ap->del || !(rt->rt_flags & RTF_STATIC))) { @@ -399,12 +387,8 @@ */ RT_LOCK(rt); rt->rt_flags &= ~(RTF_CLONING | RTF_PRCLONING); - RT_UNLOCK(rt); - err = rtrequest(RTM_DELETE, (struct sockaddr *)rt_key(rt), - rt->rt_gateway, rt_mask(rt), rt->rt_flags, 0); - if (err) { - log(LOG_WARNING, "in_ifadownkill: error %d\n", err); - } + rtexpunge(rt); + RTFREE_LOCKED(rt); } return 0; } ==== //depot/projects/netperf/sys/netinet6/in6_ifattach.c#10 (text+ko) ==== @@ -906,13 +906,9 @@ sin6.sin6_addr.s6_addr16[1] = htons(ifp->if_index); rt = rtalloc1((struct sockaddr *)&sin6, 0, 0UL); if (rt) { - if (rt->rt_ifp == ifp) { - RT_UNLOCK(rt); - rtrequest(RTM_DELETE, (struct sockaddr *)rt_key(rt), - rt->rt_gateway, rt_mask(rt), rt->rt_flags, 0); - RTFREE(rt); - } else - rtfree(rt); + if (rt->rt_ifp == ifp) + rtexpunge(rt); + RTFREE_LOCKED(rt); } } ==== //depot/projects/netperf/sys/netinet6/in6_pcb.c#12 (text+ko) ==== @@ -842,11 +842,9 @@ info.rti_info[RTAX_GATEWAY] = rt->rt_gateway; info.rti_info[RTAX_NETMASK] = rt_mask(rt); rt_missmsg(RTM_LOSING, &info, rt->rt_flags, 0); - if (rt->rt_flags & RTF_DYNAMIC) { - RT_UNLOCK(rt); /* XXX refcnt? */ - (void)rtrequest1(RTM_DELETE, &info, NULL); - } else - rtfree(rt); + if (rt->rt_flags & RTF_DYNAMIC) + rtexpunge(rt); + RTFREE_LOCKED(rt); /* * A new route can be allocated * the next time output is attempted. ==== //depot/projects/netperf/sys/netinet6/in6_rmx.c#11 (text+ko) ==== @@ -167,17 +167,12 @@ rt2->rt_flags & RTF_HOST && rt2->rt_gateway && rt2->rt_gateway->sa_family == AF_LINK) { - /* NB: must unlock to avoid recursion */ - RT_UNLOCK(rt2); - rtrequest(RTM_DELETE, - (struct sockaddr *)rt_key(rt2), - rt2->rt_gateway, - rt_mask(rt2), rt2->rt_flags, 0); + rtexpunge(rt2); + RTFREE_LOCKED(rt2); ret = rn_addroute(v_arg, n_arg, head, treenodes); - RT_LOCK(rt2); - } - RTFREE_LOCKED(rt2); + } else + RTFREE_LOCKED(rt2); } } else if (ret == NULL && rt->rt_flags & RTF_CLONING) { struct rtentry *rt2; @@ -276,13 +271,7 @@ rt->rt_flags |= RTPRF_OURS; rt->rt_rmx.rmx_expire = time_second + rtq_reallyold; } else { - /* NB: must unlock to avoid recursion */ - RT_UNLOCK(rt); - rtrequest(RTM_DELETE, - (struct sockaddr *)rt_key(rt), - rt->rt_gateway, rt_mask(rt), - rt->rt_flags, 0); - RT_LOCK(rt); + rtexpunge(rt); } }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200310300045.h9U0jxxP068743>