nd6_nbr.c's static list of IPs to perform Duplicate Address Detection on
is protected by the too broad exclusive net lock.
Switch nd6_dad_timer() and helpers to a dedicated dad_lk rwlock(9) to
take pressure of the net lock.
A mutex(9) must not be used as this code path may sleep:
nd6_dad_timer() -> rtm_addr() -> route_input() -> solock().
Document all struct dadq's fields and their protection accordingly.
This makes DAD no longer grab the net lock itself; existing assertions
must remain, though, and have been annotated to ease review.
Static helper functions in this file accessing the static list do assert
that the new lock is taken for now clarify the intention and ease
testing/review.
I'd like to commit without net lock assert comment and new lock assert.
WITNESS, regress, daily usage and manual testing are fine for me.
More tests?
Feedback? OK?
diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c
index acced08b3db..fedd3fdb42d 100644
--- a/sys/netinet6/nd6_nbr.c
+++ b/sys/netinet6/nd6_nbr.c
@@ -42,6 +42,7 @@
#include <sys/syslog.h>
#include <sys/queue.h>
#include <sys/timeout.h>
+#include <sys/rwlock.h>
#include <net/if.h>
#include <net/if_var.h>
@@ -62,17 +63,25 @@
#include <netinet/ip_carp.h>
#endif
+/*
+ * Locks used to protect struct members in this file:
+ * D DAD lock
+ */
+
+static struct rwlock dad_lk =
+ RWLOCK_INITIALIZER("dad");
+
static TAILQ_HEAD(, dadq) dadq =
TAILQ_HEAD_INITIALIZER(dadq); /* list of addresses to run DAD on */
struct dadq {
- TAILQ_ENTRY(dadq) dad_list;
- struct ifaddr *dad_ifa;
- int dad_count; /* max NS to send */
- int dad_ns_tcount; /* # of trials to send NS */
- int dad_ns_ocount; /* NS sent so far */
- int dad_ns_icount;
- int dad_na_icount;
- struct timeout dad_timer_ch;
+ TAILQ_ENTRY(dadq) dad_list; /* [D] all struct dadqs are chained */
+ struct ifaddr *dad_ifa; /* [D] IP address to perform DAD on */
+ int dad_count; /* [D] max NS to send */
+ int dad_ns_tcount; /* [D] # of trials to send NS */
+ int dad_ns_ocount; /* [D] NS sent so far */
+ int dad_ns_icount; /* [D] NS received so far */
+ int dad_na_icount; /* [D] NA received so far */
+ struct timeout dad_timer_ch; /* [D] */
};
struct dadq *nd6_dad_find(struct ifaddr *);
@@ -575,6 +584,8 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
struct nd_opts ndopts;
char addr[INET6_ADDRSTRLEN], addr0[INET6_ADDRSTRLEN];
+ /* only called from icmp6_input() aka. .pr_input which is called from
+ * ip_deliver() with NET_ASSERT_LOCKED_EXCLUSIVE() */
NET_ASSERT_LOCKED();
ifp = if_get(m->m_pkthdr.ph_ifidx);
@@ -652,6 +663,7 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
if (ifa && (ifatoia6(ifa)->ia6_flags & IN6_IFF_TENTATIVE)) {
struct dadq *dp;
+ rw_enter_write(&dad_lk);
dp = nd6_dad_find(ifa);
if (dp) {
dp->dad_na_icount++;
@@ -659,6 +671,7 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
/* remove the address. */
nd6_dad_duplicated(dp);
}
+ rw_exit_write(&dad_lk);
goto freeit;
}
@@ -1040,6 +1053,8 @@ nd6_dad_find(struct ifaddr *ifa)
{
struct dadq *dp;
+ rw_assert_anylock(&dad_lk);
+
TAILQ_FOREACH(dp, &dadq, dad_list) {
if (dp->dad_ifa == ifa)
return dp;
@@ -1050,6 +1065,8 @@ nd6_dad_find(struct ifaddr *ifa)
void
nd6_dad_starttimer(struct dadq *dp)
{
+ rw_assert_wrlock(&dad_lk);
+
timeout_set_proc(&dp->dad_timer_ch, nd6_dad_timer, dp->dad_ifa);
timeout_add_msec(&dp->dad_timer_ch, RETRANS_TIMER);
}
@@ -1057,6 +1074,8 @@ nd6_dad_starttimer(struct dadq *dp)
void
nd6_dad_stoptimer(struct dadq *dp)
{
+ rw_assert_wrlock(&dad_lk);
+
timeout_del(&dp->dad_timer_ch);
}
@@ -1070,6 +1089,9 @@ nd6_dad_start(struct ifaddr *ifa)
struct dadq *dp;
char addr[INET6_ADDRSTRLEN];
+ /* only called from
+ * - in6_ifattach_linklocal() with NET_ASSERT_LOCKED()
+ * - in6_ioctl_change_ifaddr() with NET_LOCK() */
NET_ASSERT_LOCKED();
/*
@@ -1088,8 +1110,9 @@ nd6_dad_start(struct ifaddr *ifa)
}
/* DAD already in progress */
+ rw_enter_write(&dad_lk);
if (nd6_dad_find(ifa) != NULL)
- return;
+ goto out;
dp = malloc(sizeof(*dp), M_IP6NDP, M_NOWAIT | M_ZERO);
if (dp == NULL) {
@@ -1097,7 +1120,7 @@ nd6_dad_start(struct ifaddr *ifa)
__func__, inet_ntop(AF_INET6, &ia6->ia_addr.sin6_addr,
addr, sizeof(addr)),
ifa->ifa_ifp ? ifa->ifa_ifp->if_xname : "???");
- return;
+ goto out;
}
bzero(&dp->dad_timer_ch, sizeof(dp->dad_timer_ch));
@@ -1119,6 +1142,9 @@ nd6_dad_start(struct ifaddr *ifa)
dp->dad_ns_ocount = dp->dad_ns_tcount = 0;
nd6_dad_ns_output(dp, ifa);
nd6_dad_starttimer(dp);
+
+out:
+ rw_exit_write(&dad_lk);
}
/*
@@ -1129,10 +1155,11 @@ nd6_dad_stop(struct ifaddr *ifa)
{
struct dadq *dp;
+ rw_enter_write(&dad_lk);
dp = nd6_dad_find(ifa);
if (!dp) {
/* DAD wasn't started yet */
- return;
+ goto done;
}
nd6_dad_stoptimer(dp);
@@ -1142,6 +1169,9 @@ nd6_dad_stop(struct ifaddr *ifa)
dp = NULL;
ifafree(ifa);
ip6_dad_pending--;
+
+done:
+ rw_exit_write(&dad_lk);
}
void
@@ -1152,13 +1182,13 @@ nd6_dad_timer(void *xifa)
struct dadq *dp;
char addr[INET6_ADDRSTRLEN];
- NET_LOCK();
-
/* Sanity check */
if (ia6 == NULL) {
log(LOG_ERR, "%s: called with null parameter\n", __func__);
- goto done;
+ return;
}
+
+ rw_enter_write(&dad_lk);
dp = nd6_dad_find(ifa);
if (dp == NULL) {
log(LOG_ERR, "%s: DAD structure not found\n", __func__);
@@ -1229,7 +1259,7 @@ nd6_dad_timer(void *xifa)
}
done:
- NET_UNLOCK();
+ rw_exit_write(&dad_lk);
}
void
@@ -1238,6 +1268,8 @@ nd6_dad_duplicated(struct dadq *dp)
struct in6_ifaddr *ia6 = ifatoia6(dp->dad_ifa);
char addr[INET6_ADDRSTRLEN];
+ rw_assert_wrlock(&dad_lk);
+
log(LOG_ERR, "%s: DAD detected duplicate IPv6 address %s: "
"NS in/out=%d/%d, NA in=%d\n",
ia6->ia_ifp->if_xname,
@@ -1271,6 +1303,8 @@ nd6_dad_ns_output(struct dadq *dp, struct ifaddr *ifa)
struct in6_ifaddr *ia6 = ifatoia6(ifa);
struct ifnet *ifp = ifa->ifa_ifp;
+ rw_assert_wrlock(&dad_lk);
+
dp->dad_ns_tcount++;
if ((ifp->if_flags & IFF_UP) == 0) {
#if 0
@@ -1297,10 +1331,11 @@ nd6_dad_ns_input(struct ifaddr *ifa)
if (!ifa)
panic("%s: ifa == NULL", __func__);
+ rw_enter_write(&dad_lk);
dp = nd6_dad_find(ifa);
if (dp == NULL) {
log(LOG_ERR, "%s: DAD structure not found\n", __func__);
- return;
+ goto out;
}
/*
@@ -1318,6 +1353,9 @@ nd6_dad_ns_input(struct ifaddr *ifa)
*/
dp->dad_ns_icount++;
}
+
+out:
+ rw_exit_write(&dad_lk);
}
/*
