Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/trunk]: src/sys Reform use of rt_refcnt
details: https://anonhg.NetBSD.org/src/rev/0cdc9e2a8e24
branches: trunk
changeset: 809502:0cdc9e2a8e24
user: ozaki-r <ozaki-r%NetBSD.org@localhost>
date: Fri Jul 17 02:21:08 2015 +0000
description:
Reform use of rt_refcnt
rt_refcnt of rtentry was used in bad manners, for example, direct rt_refcnt++
and rt_refcnt-- outside route.c, "rt->rt_refcnt++; rtfree(rt);" idiom, and
touching rt after rt->rt_refcnt--.
These abuses seem to be needed because rt_refcnt manages only references
between rtentry and doesn't take care of references during packet processing
(IOW references from local variables). In order to reduce the above abuses,
the latter cases should be counted by rt_refcnt as well as the former cases.
This change improves consistency of use of rt_refcnt:
- rtentry is always accessed with rt_refcnt incremented
- rtentry's rt_refcnt is decremented after use (rtfree is always used instead
of rt_refcnt--)
- functions returning rtentry increment its rt_refcnt (and caller rtfree it)
Note that rt_refcnt prevents rtentry from being freed but doesn't prevent
rtentry from being updated. Toward MP-safe, we need to provide another
protection for rtentry, e.g., locks. (Or introduce a better data structure
allowing concurrent readers during updates.)
diffstat:
sys/net/if.c | 19 ++++---
sys/net/route.c | 111 ++++++++++++++++++++++++++++----------------
sys/net/route.h | 13 ++++-
sys/net/rtsock.c | 11 ++--
sys/netinet/if_atm.c | 26 ++++++----
sys/netinet/ip_output.c | 34 +++++++++----
sys/netinet6/icmp6.c | 9 ++-
sys/netinet6/nd6.c | 119 +++++++++++++++++++++++++++++++----------------
sys/netinet6/nd6.h | 4 +-
sys/netinet6/nd6_nbr.c | 8 ++-
sys/netinet6/nd6_rtr.c | 103 ++++++++++++++--------------------------
11 files changed, 267 insertions(+), 190 deletions(-)
diffs (truncated from 1189 to 300 lines):
diff -r 67d44f680a9e -r 0cdc9e2a8e24 sys/net/if.c
--- a/sys/net/if.c Thu Jul 16 15:43:10 2015 +0000
+++ b/sys/net/if.c Fri Jul 17 02:21:08 2015 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: if.c,v 1.316 2015/06/29 09:40:36 ozaki-r Exp $ */
+/* $NetBSD: if.c,v 1.317 2015/07/17 02:21:08 ozaki-r Exp $ */
/*-
* Copyright (c) 1999, 2000, 2001, 2008 The NetBSD Foundation, Inc.
@@ -90,7 +90,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if.c,v 1.316 2015/06/29 09:40:36 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if.c,v 1.317 2015/07/17 02:21:08 ozaki-r Exp $");
#if defined(_KERNEL_OPT)
#include "opt_inet.h"
@@ -963,20 +963,23 @@
{
struct ifnet *ifp = (struct ifnet *)v;
int error;
+ struct rtentry *retrt;
if (rt->rt_ifp != ifp)
return 0;
/* Delete the entry. */
- ++rt->rt_refcnt;
error = rtrequest(RTM_DELETE, rt_getkey(rt), rt->rt_gateway,
- rt_mask(rt), rt->rt_flags, NULL);
- KASSERT((rt->rt_flags & RTF_UP) == 0);
- rt->rt_ifp = NULL;
- rtfree(rt);
- if (error != 0)
+ rt_mask(rt), rt->rt_flags, &retrt);
+ if (error == 0) {
+ KASSERT(retrt == rt);
+ KASSERT((retrt->rt_flags & RTF_UP) == 0);
+ retrt->rt_ifp = NULL;
+ rtfree(retrt);
+ } else {
printf("%s: warning: unable to delete rtentry @ %p, "
"error = %d\n", ifp->if_xname, rt, error);
+ }
return ERESTART;
}
diff -r 67d44f680a9e -r 0cdc9e2a8e24 sys/net/route.c
--- a/sys/net/route.c Thu Jul 16 15:43:10 2015 +0000
+++ b/sys/net/route.c Fri Jul 17 02:21:08 2015 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: route.c,v 1.145 2015/06/08 08:21:49 roy Exp $ */
+/* $NetBSD: route.c,v 1.146 2015/07/17 02:21:08 ozaki-r Exp $ */
/*-
* Copyright (c) 1998, 2008 The NetBSD Foundation, Inc.
@@ -94,7 +94,7 @@
#include "opt_route.h"
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: route.c,v 1.145 2015/06/08 08:21:49 roy Exp $");
+__KERNEL_RCSID(0, "$NetBSD: route.c,v 1.146 2015/07/17 02:21:08 ozaki-r Exp $");
#include <sys/param.h>
#ifdef RTFLUSH_DEBUG
@@ -355,7 +355,8 @@
}
/*
- * Packet routing routines.
+ * Packet routing routines. If success, refcnt of a returned rtentry
+ * will be incremented. The caller has to rtfree it by itself.
*/
struct rtentry *
rtalloc1(const struct sockaddr *dst, int report)
@@ -378,6 +379,7 @@
}
KASSERT(newrt != NULL);
rt = newrt;
+ rt->rt_refcnt++;
if (rt->rt_flags & RTF_XRESOLVE) {
msgtype = RTM_RESOLVE;
goto miss;
@@ -533,13 +535,15 @@
}
/*
- * Delete a route and generate a message
+ * Delete a route and generate a message.
+ * It doesn't free a passed rt.
*/
static int
rtdeletemsg(struct rtentry *rt)
{
int error;
struct rt_addrinfo info;
+ struct rtentry *retrt;
/*
* Request the new route so that the entry is not actually
@@ -551,15 +555,12 @@
info.rti_info[RTAX_NETMASK] = rt_mask(rt);
info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
info.rti_flags = rt->rt_flags;
- error = rtrequest1(RTM_DELETE, &info, &rt);
+ error = rtrequest1(RTM_DELETE, &info, &retrt);
rt_missmsg(RTM_DELETE, &info, info.rti_flags, error);
- /* Adjust the refcount */
- if (error == 0 && rt->rt_refcnt <= 0) {
- rt->rt_refcnt++;
- rtfree(rt);
- }
+ if (error == 0)
+ rtfree(retrt);
return error;
}
@@ -617,8 +618,9 @@
struct rtentry *rt = rtalloc1(dst, 0);
if (rt == NULL)
return NULL;
- rt->rt_refcnt--;
- if ((ifa = rt->rt_ifa) == NULL)
+ ifa = rt->rt_ifa;
+ rtfree(rt);
+ if (ifa == NULL)
return NULL;
}
if (ifa->ifa_addr->sa_family != dst->sa_family) {
@@ -630,6 +632,10 @@
return ifa;
}
+/*
+ * If it suceeds and ret_nrt isn't NULL, refcnt of ret_nrt is incremented.
+ * The caller has to rtfree it by itself.
+ */
int
rtrequest(int req, const struct sockaddr *dst, const struct sockaddr *gateway,
const struct sockaddr *netmask, int flags, struct rtentry **ret_nrt)
@@ -644,6 +650,32 @@
return rtrequest1(req, &info, ret_nrt);
}
+/*
+ * It's a utility function to add/remove a route to/from the routing table
+ * and tell user processes the addition/removal on success.
+ */
+int
+rtrequest_newmsg(const int req, const struct sockaddr *dst,
+ const struct sockaddr *gateway, const struct sockaddr *netmask,
+ const int flags)
+{
+ int error;
+ struct rtentry *ret_nrt = NULL;
+
+ KASSERT(req == RTM_ADD || req == RTM_DELETE);
+
+ error = rtrequest(req, dst, gateway, netmask, flags, &ret_nrt);
+ if (error != 0)
+ return error;
+
+ KASSERT(ret_nrt != NULL);
+
+ rt_newmsg(req, ret_nrt); /* tell user process */
+ rtfree(ret_nrt);
+
+ return 0;
+}
+
int
rt_getifa(struct rt_addrinfo *info)
{
@@ -688,6 +720,10 @@
return 0;
}
+/*
+ * If it suceeds and ret_nrt isn't NULL, refcnt of ret_nrt is incremented.
+ * The caller has to rtfree it by itself.
+ */
int
rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
{
@@ -743,9 +779,11 @@
ifa->ifa_rtrequest(RTM_DELETE, rt, info);
}
rttrash++;
- if (ret_nrt)
+ if (ret_nrt) {
*ret_nrt = rt;
- else if (rt->rt_refcnt <= 0) {
+ rt->rt_refcnt++;
+ } else if (rt->rt_refcnt <= 0) {
+ /* Adjust the refcount */
rt->rt_refcnt++;
rtfree(rt);
}
@@ -974,10 +1012,12 @@
rt_maskedcopy(odst, dst, ifa->ifa_netmask);
}
if ((rt = rtalloc1(dst, 0)) != NULL) {
- rt->rt_refcnt--;
- if (rt->rt_ifa != ifa)
+ if (rt->rt_ifa != ifa) {
+ rtfree(rt);
return (flags & RTF_HOST) ? EHOSTUNREACH
: ENETUNREACH;
+ }
+ rtfree(rt);
}
}
memset(&info, 0, sizeof(info));
@@ -996,17 +1036,14 @@
error = rtrequest1((cmd == RTM_LLINFO_UPD) ? RTM_GET : cmd, &info,
&nrt);
if (error != 0 || (rt = nrt) == NULL)
- ;
- else switch (cmd) {
+ return error;
+
+ switch (cmd) {
case RTM_DELETE:
- rt_newmsg(cmd, nrt);
- if (rt->rt_refcnt <= 0) {
- rt->rt_refcnt++;
- rtfree(rt);
- }
+ rt_newmsg(cmd, rt);
+ rtfree(rt);
break;
case RTM_LLINFO_UPD:
- rt->rt_refcnt--;
RT_DPRINTF("%s: updating%s\n", __func__,
((rt->rt_flags & RTF_LLINFO) == 0) ? " (no llinfo)" : "");
@@ -1023,10 +1060,10 @@
if (cmd == RTM_LLINFO_UPD && ifa->ifa_rtrequest != NULL)
ifa->ifa_rtrequest(RTM_LLINFO_UPD, rt, &info);
- rt_newmsg(RTM_CHANGE, nrt);
+ rt_newmsg(RTM_CHANGE, rt);
+ rtfree(rt);
break;
case RTM_ADD:
- rt->rt_refcnt--;
if (rt->rt_ifa != ifa) {
printf("rtinit: wrong ifa (%p) was (%p)\n", ifa,
rt->rt_ifa);
@@ -1039,7 +1076,8 @@
if (ifa->ifa_rtrequest != NULL)
ifa->ifa_rtrequest(RTM_ADD, rt, &info);
}
- rt_newmsg(cmd, nrt);
+ rt_newmsg(cmd, rt);
+ rtfree(rt);
break;
}
return error;
@@ -1085,18 +1123,9 @@
rt_replace_ifa(nrt, ifa);
rt_newaddrmsg(cmd, ifa, e, nrt);
- if (nrt) {
- if (cmd == RTM_DELETE) {
- if (nrt->rt_refcnt <= 0) {
- /* XXX: we should free the entry ourselves. */
- nrt->rt_refcnt++;
- rtfree(nrt);
- }
- } else {
- /* the cmd must be RTM_ADD here */
- nrt->rt_refcnt--;
- }
- }
+ if (nrt != NULL)
+ rtfree(nrt);
+
return e;
}
@@ -1120,7 +1149,7 @@
rt_newaddrmsg(RTM_NEWADDR, ifa, 0, NULL);
}
if (rt != NULL)
- rt->rt_refcnt--;
+ rtfree(rt);
return e;
}
@@ -1161,7 +1190,7 @@
} else
rt_newaddrmsg(RTM_DELADDR, ifa, 0, NULL);
if (rt != NULL)
- rt->rt_refcnt--;
+ rtfree(rt);
return e;
Home |
Main Index |
Thread Index |
Old Index