summaryrefslogtreecommitdiff
path: root/sys
diff options
context:
space:
mode:
authorozaki-r <ozaki-r@NetBSD.org>2018-03-08 06:48:23 +0000
committerozaki-r <ozaki-r@NetBSD.org>2018-03-08 06:48:23 +0000
commit2e2e6cfb68a87a0e554ca3e71f6bf46a3094953e (patch)
tree202de0e55712d0cad26fe7aed64874f10afa74d3 /sys
parent56bf90645a5c5dc332348268c384da29181a08fe (diff)
Fix a race condition on DAD destructions (again)
The previous fix to DAD timers was wrong; it avoided a use-after-free but instead introduced a memory leak. The destruction method had delegated a destruction of a DAD timer to the timer itself and told that by setting NULL to dp->dad_ifa. However, the previous fix made DAD timers do nothing on the sign. Fixing the issue with using callout_stop isn't easy. One approach is to have a refcount on dp but it introduces extra complexity that we want to avoid. The new fix falls back to using callout_halt, which was abandoned because of softnet_lock. Fortunately now the network stack is protected by KERNEL_LOCK so we can remove softnet_lock from DAD timers (callout) and use callout_halt safely.
Diffstat (limited to 'sys')
-rw-r--r--sys/netinet/if_arp.c46
-rw-r--r--sys/netinet6/nd6_nbr.c84
2 files changed, 77 insertions, 53 deletions
diff --git a/sys/netinet/if_arp.c b/sys/netinet/if_arp.c
index b7f3521de4b..91f740cf72f 100644
--- a/sys/netinet/if_arp.c
+++ b/sys/netinet/if_arp.c
@@ -1,4 +1,4 @@
-/* $NetBSD: if_arp.c,v 1.270 2018/03/06 07:24:01 ozaki-r Exp $ */
+/* $NetBSD: if_arp.c,v 1.271 2018/03/08 06:48:23 ozaki-r Exp $ */
/*
* Copyright (c) 1998, 2000, 2008 The NetBSD Foundation, Inc.
@@ -68,7 +68,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_arp.c,v 1.270 2018/03/06 07:24:01 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_arp.c,v 1.271 2018/03/08 06:48:23 ozaki-r Exp $");
#ifdef _KERNEL_OPT
#include "opt_ddb.h"
@@ -1526,14 +1526,24 @@ arp_dad_starttimer(struct dadq *dp, int ticks)
}
static void
-arp_dad_destroytimer(struct dadq *dp)
+arp_dad_stoptimer(struct dadq *dp)
{
+ KASSERT(mutex_owned(&arp_dad_lock));
+
TAILQ_REMOVE(&dadq, dp, dad_list);
- /* Request the timer to destroy dp. */
+ /* Tell the timer that dp is being destroyed. */
dp->dad_ifa = NULL;
- callout_reset(&dp->dad_timer_ch, 0,
- (void (*)(void *))arp_dad_timer, dp);
+ callout_halt(&dp->dad_timer_ch, &arp_dad_lock);
+}
+
+static void
+arp_dad_destroytimer(struct dadq *dp)
+{
+
+ callout_destroy(&dp->dad_timer_ch);
+ KASSERT(dp->dad_ifa == NULL);
+ kmem_intr_free(dp, sizeof(*dp));
}
static void
@@ -1652,11 +1662,11 @@ arp_dad_stop(struct ifaddr *ifa)
return;
}
- /* Prevent the timer from running anymore. */
- arp_dad_destroytimer(dp);
+ arp_dad_stoptimer(dp);
mutex_exit(&arp_dad_lock);
+ arp_dad_destroytimer(dp);
ifafree(ifa);
}
@@ -1668,15 +1678,12 @@ arp_dad_timer(struct dadq *dp)
char ipbuf[INET_ADDRSTRLEN];
bool need_free = false;
- SOFTNET_KERNEL_LOCK_UNLESS_NET_MPSAFE();
+ KERNEL_LOCK_UNLESS_NET_MPSAFE();
mutex_enter(&arp_dad_lock);
ifa = dp->dad_ifa;
if (ifa == NULL) {
- /*
- * ifa is being deleted and the callout is already scheduled
- * again, so we cannot destroy dp in this run.
- */
+ /* dp is being destroyed by someone. Do nothing. */
goto done;
}
@@ -1700,7 +1707,7 @@ arp_dad_timer(struct dadq *dp)
ARPLOG(LOG_INFO, "%s: could not run DAD, driver problem?\n",
if_name(ifa->ifa_ifp));
- TAILQ_REMOVE(&dadq, dp, dad_list);
+ arp_dad_stoptimer(dp);
need_free = true;
goto done;
}
@@ -1749,19 +1756,18 @@ announce:
if_name(ifa->ifa_ifp), ARPLOGADDR(&ia->ia_addr.sin_addr));
}
- TAILQ_REMOVE(&dadq, dp, dad_list);
+ arp_dad_stoptimer(dp);
need_free = true;
done:
mutex_exit(&arp_dad_lock);
if (need_free) {
- callout_destroy(&dp->dad_timer_ch);
- kmem_intr_free(dp, sizeof(*dp));
- if (ifa != NULL)
- ifafree(ifa);
+ arp_dad_destroytimer(dp);
+ KASSERT(ifa != NULL);
+ ifafree(ifa);
}
- SOFTNET_KERNEL_UNLOCK_UNLESS_NET_MPSAFE();
+ KERNEL_UNLOCK_UNLESS_NET_MPSAFE();
}
static void
diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c
index f2e55ab8e14..0b3da02434d 100644
--- a/sys/netinet6/nd6_nbr.c
+++ b/sys/netinet6/nd6_nbr.c
@@ -1,4 +1,4 @@
-/* $NetBSD: nd6_nbr.c,v 1.151 2018/03/07 01:37:24 ozaki-r Exp $ */
+/* $NetBSD: nd6_nbr.c,v 1.152 2018/03/08 06:48:23 ozaki-r Exp $ */
/* $KAME: nd6_nbr.c,v 1.61 2001/02/10 16:06:14 jinmei Exp $ */
/*
@@ -31,7 +31,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: nd6_nbr.c,v 1.151 2018/03/07 01:37:24 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: nd6_nbr.c,v 1.152 2018/03/08 06:48:23 ozaki-r Exp $");
#ifdef _KERNEL_OPT
#include "opt_inet.h"
@@ -1145,17 +1145,24 @@ nd6_dad_starttimer(struct dadq *dp, int ticks)
}
static void
-nd6_dad_destroytimer(struct dadq *dp)
+nd6_dad_stoptimer(struct dadq *dp)
{
- struct ifaddr *ifa;
+
+ KASSERT(mutex_owned(&nd6_dad_lock));
TAILQ_REMOVE(&dadq, dp, dad_list);
- /* Request the timer to destroy dp. */
- ifa = dp->dad_ifa;
+ /* Tell the timer that dp is being destroyed. */
dp->dad_ifa = NULL;
- ifafree(ifa);
- callout_reset(&dp->dad_timer_ch, 0,
- (void (*)(void *))nd6_dad_timer, dp);
+ callout_halt(&dp->dad_timer_ch, &nd6_dad_lock);
+}
+
+static void
+nd6_dad_destroytimer(struct dadq *dp)
+{
+
+ KASSERT(dp->dad_ifa == NULL);
+ callout_destroy(&dp->dad_timer_ch);
+ kmem_intr_free(dp, sizeof(*dp));
}
/*
@@ -1268,9 +1275,12 @@ nd6_dad_stop(struct ifaddr *ifa)
}
/* Prevent the timer from running anymore. */
- nd6_dad_destroytimer(dp);
+ nd6_dad_stoptimer(dp);
mutex_exit(&nd6_dad_lock);
+
+ nd6_dad_destroytimer(dp);
+ ifafree(ifa);
}
static void
@@ -1282,15 +1292,12 @@ nd6_dad_timer(struct dadq *dp)
char ip6buf[INET6_ADDRSTRLEN];
bool need_free = false;
- SOFTNET_KERNEL_LOCK_UNLESS_NET_MPSAFE();
+ KERNEL_LOCK_UNLESS_NET_MPSAFE();
mutex_enter(&nd6_dad_lock);
ifa = dp->dad_ifa;
if (ifa == NULL) {
- /*
- * ifa is being deleted and the callout is already scheduled
- * again, so we cannot destroy dp in this run.
- */
+ /* dp is being destroyed by someone. Do nothing. */
goto done;
}
@@ -1315,7 +1322,7 @@ nd6_dad_timer(struct dadq *dp)
nd6log(LOG_INFO, "%s: could not run DAD, driver problem?\n",
if_name(ifa->ifa_ifp));
- TAILQ_REMOVE(&dadq, dp, dad_list);
+ nd6_dad_stoptimer(dp);
need_free = true;
goto done;
}
@@ -1348,8 +1355,8 @@ nd6_dad_timer(struct dadq *dp)
if (duplicate) {
nd6_dad_duplicated(dp);
- /* (*dp) has been freed in nd6_dad_duplicated() */
- dp = NULL;
+ nd6_dad_stoptimer(dp);
+ need_free = true;
} else {
/*
* We are done with DAD. No NA came, no NS came.
@@ -1363,7 +1370,7 @@ nd6_dad_timer(struct dadq *dp)
if_name(ifa->ifa_ifp),
IN6_PRINT(ip6buf, &ia->ia_addr.sin6_addr));
- TAILQ_REMOVE(&dadq, dp, dad_list);
+ nd6_dad_stoptimer(dp);
need_free = true;
}
}
@@ -1371,13 +1378,12 @@ done:
mutex_exit(&nd6_dad_lock);
if (need_free) {
- callout_destroy(&dp->dad_timer_ch);
- kmem_intr_free(dp, sizeof(*dp));
- if (ifa != NULL)
- ifafree(ifa);
+ nd6_dad_destroytimer(dp);
+ KASSERT(ifa != NULL);
+ ifafree(ifa);
}
- SOFTNET_KERNEL_UNLOCK_UNLESS_NET_MPSAFE();
+ KERNEL_UNLOCK_UNLESS_NET_MPSAFE();
}
static void
@@ -1441,9 +1447,6 @@ nd6_dad_duplicated(struct dadq *dp)
break;
}
}
-
- /* We are done with DAD, with duplicated address found. (failure) */
- nd6_dad_destroytimer(dp);
}
static void
@@ -1499,8 +1502,10 @@ nd6_dad_ns_input(struct ifaddr *ifa, struct nd_opt_nonce *nonce)
/* XXX more checks for loopback situation - see nd6_dad_timer too */
if (duplicate) {
- if (dp)
+ if (dp) {
nd6_dad_duplicated(dp);
+ nd6_dad_stoptimer(dp);
+ }
} else {
/*
* not sure if I got a duplicate.
@@ -1510,6 +1515,11 @@ nd6_dad_ns_input(struct ifaddr *ifa, struct nd_opt_nonce *nonce)
dp->dad_ns_icount++;
}
mutex_exit(&nd6_dad_lock);
+
+ if (duplicate && dp) {
+ nd6_dad_destroytimer(dp);
+ ifafree(ifa);
+ }
}
static void
@@ -1520,13 +1530,21 @@ nd6_dad_na_input(struct ifaddr *ifa)
KASSERT(ifa != NULL);
mutex_enter(&nd6_dad_lock);
- dp = nd6_dad_find(ifa, NULL);
- if (dp) {
- dp->dad_na_icount++;
- /* remove the address. */
- nd6_dad_duplicated(dp);
+ dp = nd6_dad_find(ifa, NULL);
+ if (dp == NULL) {
+ mutex_exit(&nd6_dad_lock);
+ return;
}
+ dp->dad_na_icount++;
+
+ /* remove the address. */
+ nd6_dad_duplicated(dp);
+ nd6_dad_stoptimer(dp);
+
mutex_exit(&nd6_dad_lock);
+
+ nd6_dad_destroytimer(dp);
+ ifafree(ifa);
}