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);
> }
>
> /*
>
