Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/trunk]: src/sys Fix a race condition on DAD destructions (again)
details: https://anonhg.NetBSD.org/src/rev/acf619cdb521
branches: trunk
changeset: 321262:acf619cdb521
user: ozaki-r <ozaki-r%NetBSD.org@localhost>
date: Thu Mar 08 06:48:23 2018 +0000
description:
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:
sys/netinet/if_arp.c | 48 ++++++++++++++++------------
sys/netinet6/nd6_nbr.c | 84 ++++++++++++++++++++++++++++++-------------------
2 files changed, 78 insertions(+), 54 deletions(-)
diffs (truncated from 304 to 300 lines):
diff -r 25e685c14148 -r acf619cdb521 sys/netinet/if_arp.c
--- a/sys/netinet/if_arp.c Thu Mar 08 06:47:30 2018 +0000
+++ b/sys/netinet/if_arp.c Thu Mar 08 06:48:23 2018 +0000
@@ -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 @@
}
static void
+arp_dad_stoptimer(struct dadq *dp)
+{
+
+ KASSERT(mutex_owned(&arp_dad_lock));
+
+ TAILQ_REMOVE(&dadq, dp, dad_list);
+ /* Tell the timer that dp is being destroyed. */
+ dp->dad_ifa = NULL;
+ callout_halt(&dp->dad_timer_ch, &arp_dad_lock);
+}
+
+static void
arp_dad_destroytimer(struct dadq *dp)
{
- TAILQ_REMOVE(&dadq, dp, dad_list);
- /* Request the timer to destroy dp. */
- dp->dad_ifa = NULL;
- callout_reset(&dp->dad_timer_ch, 0,
- (void (*)(void *))arp_dad_timer, dp);
+ callout_destroy(&dp->dad_timer_ch);
+ KASSERT(dp->dad_ifa == NULL);
+ kmem_intr_free(dp, sizeof(*dp));
}
static void
@@ -1652,11 +1662,11 @@
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 @@
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 @@
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 @@
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 -r 25e685c14148 -r acf619cdb521 sys/netinet6/nd6_nbr.c
--- a/sys/netinet6/nd6_nbr.c Thu Mar 08 06:47:30 2018 +0000
+++ b/sys/netinet6/nd6_nbr.c Thu Mar 08 06:48:23 2018 +0000
@@ -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 @@
}
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 @@
}
/* 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
break;
}
}
-
- /* We are done with DAD, with duplicated address found. (failure) */
- nd6_dad_destroytimer(dp);
}
static void
@@ -1499,8 +1502,10 @@
/* 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 @@
dp->dad_ns_icount++;
}
mutex_exit(&nd6_dad_lock);
+
+ if (duplicate && dp) {
+ nd6_dad_destroytimer(dp);
+ ifafree(ifa);
+ }
}
static void
@@ -1520,13 +1530,21 @@
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);
+ 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);
Home |
Main Index |
Thread Index |
Old Index