Module Name: src
Committed By: ozaki-r
Date: Thu Mar 8 06:48:23 UTC 2018
Modified Files:
src/sys/netinet: if_arp.c
src/sys/netinet6: nd6_nbr.c
Log Message:
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.
To generate a diff of this commit:
cvs rdiff -u -r1.270 -r1.271 src/sys/netinet/if_arp.c
cvs rdiff -u -r1.151 -r1.152 src/sys/netinet6/nd6_nbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/netinet/if_arp.c
diff -u src/sys/netinet/if_arp.c:1.270 src/sys/netinet/if_arp.c:1.271
--- src/sys/netinet/if_arp.c:1.270 Tue Mar 6 07:24:01 2018
+++ src/sys/netinet/if_arp.c Thu Mar 8 06:48:23 2018
@@ -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
}
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
Index: src/sys/netinet6/nd6_nbr.c
diff -u src/sys/netinet6/nd6_nbr.c:1.151 src/sys/netinet6/nd6_nbr.c:1.152
--- src/sys/netinet6/nd6_nbr.c:1.151 Wed Mar 7 01:37:24 2018
+++ src/sys/netinet6/nd6_nbr.c Thu Mar 8 06:48:23 2018
@@ -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
}
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, str
/* 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, str
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);
}
