Module Name: src
Committed By: martin
Date: Mon Apr 2 08:54:35 UTC 2018
Modified Files:
src/sys/netinet [netbsd-8]: if_arp.c
src/sys/netinet6 [netbsd-8]: nd6_nbr.c
Log Message:
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.
To generate a diff of this commit:
cvs rdiff -u -r1.250.2.7 -r1.250.2.8 src/sys/netinet/if_arp.c
cvs rdiff -u -r1.138.6.5 -r1.138.6.6 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.250.2.7 src/sys/netinet/if_arp.c:1.250.2.8
--- src/sys/netinet/if_arp.c:1.250.2.7 Tue Mar 13 13:27:10 2018
+++ src/sys/netinet/if_arp.c Mon Apr 2 08:54:35 2018
@@ -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 @@ 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
@@ -1665,11 +1675,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);
}
@@ -1681,15 +1691,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;
}
@@ -1713,7 +1720,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;
}
@@ -1762,19 +1769,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.138.6.5 src/sys/netinet6/nd6_nbr.c:1.138.6.6
--- src/sys/netinet6/nd6_nbr.c:1.138.6.5 Tue Mar 20 09:13:15 2018
+++ src/sys/netinet6/nd6_nbr.c Mon Apr 2 08:54:35 2018
@@ -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 @@ 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));
}
/*
@@ -1214,9 +1221,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
@@ -1228,15 +1238,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;
}
@@ -1261,7 +1268,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;
}
@@ -1293,8 +1300,9 @@ nd6_dad_timer(struct dadq *dp)
}
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 @@ 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;
}
}
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 @@ nd6_dad_duplicated(struct dadq *dp)
break;
}
}
-
- /* We are done with DAD, with duplicated address found. (failure) */
- nd6_dad_destroytimer(dp);
}
static void
@@ -1457,7 +1458,10 @@ nd6_dad_ns_input(struct ifaddr *ifa)
/* 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 @@ nd6_dad_ns_input(struct ifaddr *ifa)
dp->dad_ns_icount++;
}
mutex_exit(&nd6_dad_lock);
+
+ if (duplicate && dp) {
+ nd6_dad_destroytimer(dp);
+ ifafree(ifa);
+ }
}
static void
@@ -1477,12 +1486,21 @@ nd6_dad_na_input(struct ifaddr *ifa)
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);
mutex_exit(&nd6_dad_lock);
+
+ nd6_dad_destroytimer(dp);
+ ifafree(ifa);
}
