Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/netbsd-8]: src/sys Pull up following revision(s) (requested by ozaki-r i...
details: https://anonhg.NetBSD.org/src/rev/f88cb5590acc
branches: netbsd-8
changeset: 434815:f88cb5590acc
user: martin <martin%NetBSD.org@localhost>
date: Mon Apr 02 08:54:35 2018 +0000
description:
Pull up following revision(s) (requested by ozaki-r in ticket #686):
sys/netinet/if_arp.c: revision 1.271
sys/netinet6/nd6_nbr.c: revision 1.151,1.152
Avoid passing NULL to nd6_dad_duplicated
Fix PR kern/53075
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 305 to 300 lines):
diff -r c482a7b5f2ed -r f88cb5590acc sys/netinet/if_arp.c
--- a/sys/netinet/if_arp.c Mon Apr 02 08:50:33 2018 +0000
+++ b/sys/netinet/if_arp.c Mon Apr 02 08:54:35 2018 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: if_arp.c,v 1.250.2.7 2018/03/13 13:27:10 martin Exp $ */
+/* $NetBSD: if_arp.c,v 1.250.2.8 2018/04/02 08:54:35 martin 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.250.2.7 2018/03/13 13:27:10 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_arp.c,v 1.250.2.8 2018/04/02 08:54:35 martin Exp $");
#ifdef _KERNEL_OPT
#include "opt_ddb.h"
@@ -1539,14 +1539,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
@@ -1665,11 +1675,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);
}
@@ -1681,15 +1691,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;
}
@@ -1713,7 +1720,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;
}
@@ -1762,19 +1769,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 c482a7b5f2ed -r f88cb5590acc sys/netinet6/nd6_nbr.c
--- a/sys/netinet6/nd6_nbr.c Mon Apr 02 08:50:33 2018 +0000
+++ b/sys/netinet6/nd6_nbr.c Mon Apr 02 08:54:35 2018 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: nd6_nbr.c,v 1.138.6.5 2018/03/20 09:13:15 bouyer Exp $ */
+/* $NetBSD: nd6_nbr.c,v 1.138.6.6 2018/04/02 08:54:35 martin 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.138.6.5 2018/03/20 09:13:15 bouyer Exp $");
+__KERNEL_RCSID(0, "$NetBSD: nd6_nbr.c,v 1.138.6.6 2018/04/02 08:54:35 martin Exp $");
#ifdef _KERNEL_OPT
#include "opt_inet.h"
@@ -1092,17 +1092,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));
}
/*
@@ -1214,9 +1221,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
@@ -1228,15 +1238,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;
}
@@ -1261,7 +1268,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;
}
@@ -1293,8 +1300,9 @@
}
if (duplicate) {
- /* (*dp) will be freed in nd6_dad_duplicated() */
- dp = NULL;
+ nd6_dad_duplicated(dp);
+ nd6_dad_stoptimer(dp);
+ need_free = true;
} else {
/*
* We are done with DAD. No NA came, no NS came.
@@ -1308,24 +1316,20 @@
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;
}
}
done:
- if (duplicate)
- nd6_dad_duplicated(dp);
-
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
@@ -1390,9 +1394,6 @@
break;
}
}
-
- /* We are done with DAD, with duplicated address found. (failure) */
- nd6_dad_destroytimer(dp);
}
static void
@@ -1457,7 +1458,10 @@
/* XXX more checks for loopback situation - see nd6_dad_timer too */
if (duplicate) {
- nd6_dad_duplicated(dp);
+ if (dp) {
+ nd6_dad_duplicated(dp);
+ nd6_dad_stoptimer(dp);
+ }
} else {
/*
* not sure if I got a duplicate.
@@ -1467,6 +1471,11 @@
dp->dad_ns_icount++;
}
mutex_exit(&nd6_dad_lock);
+
+ if (duplicate && dp) {
+ nd6_dad_destroytimer(dp);
+ ifafree(ifa);
+ }
}
static void
@@ -1477,12 +1486,21 @@
KASSERT(ifa != NULL);
mutex_enter(&nd6_dad_lock);
+
dp = nd6_dad_find(ifa);
- if (dp)
- dp->dad_na_icount++;
+ if (dp == NULL) {
+ mutex_exit(&nd6_dad_lock);
+ return;
+ }
+
+ dp->dad_na_icount++;
/* remove the address. */
nd6_dad_duplicated(dp);
+ nd6_dad_stoptimer(dp);
Home |
Main Index |
Thread Index |
Old Index