PERFORCE change 40792 for review
Sam Leffler
sam at FreeBSD.org
Wed Oct 29 16:46:01 PST 2003
http://perforce.freebsd.org/chv.cgi?CH=40792
Change 40792 by sam at 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);
}
}
More information about the p4-projects
mailing list