The netlock removal within nd6_dad_timer() is wrong. It protects `ifa’, `ia6’ and ip6 output path.
> On 10 Dec 2022, at 15:16, Klemens Nanni <k...@openbsd.org> wrote: > > 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); > } > > /* >