Module Name: src
Committed By: ozaki-r
Date: Wed Dec 21 08:47:02 UTC 2016
Modified Files:
src/sys/net: if_llatbl.c if_llatbl.h
src/sys/netinet6: in6.c nd6.c
Log Message:
Fix deadlock between llentry timers and destruction of llentry
llentry timer (of nd6) holds both llentry's lock and softnet_lock.
A caller also holds them and calls callout_halt to wait for the
timer to quit. However we can pass only one lock to callout_halt,
so passing either of them can cause a deadlock. Fix it by avoid
calling callout_halt without holding llentry's lock.
BTW in the first place we cannot pass llentry's lock to callout_halt
because it's a rwlock...
To generate a diff of this commit:
cvs rdiff -u -r1.15 -r1.16 src/sys/net/if_llatbl.c
cvs rdiff -u -r1.9 -r1.10 src/sys/net/if_llatbl.h
cvs rdiff -u -r1.225 -r1.226 src/sys/netinet6/in6.c
cvs rdiff -u -r1.221 -r1.222 src/sys/netinet6/nd6.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/net/if_llatbl.c
diff -u src/sys/net/if_llatbl.c:1.15 src/sys/net/if_llatbl.c:1.16
--- src/sys/net/if_llatbl.c:1.15 Tue Oct 11 12:32:30 2016
+++ src/sys/net/if_llatbl.c Wed Dec 21 08:47:02 2016
@@ -1,4 +1,4 @@
-/* $NetBSD: if_llatbl.c,v 1.15 2016/10/11 12:32:30 roy Exp $ */
+/* $NetBSD: if_llatbl.c,v 1.16 2016/12/21 08:47:02 ozaki-r Exp $ */
/*
* Copyright (c) 2004 Luigi Rizzo, Alessandro Cerri. All rights reserved.
* Copyright (c) 2004-2008 Qing Li. All rights reserved.
@@ -424,8 +424,24 @@ lltable_purge_entries(struct lltable *ll
IF_AFDATA_WUNLOCK(llt->llt_ifp);
LIST_FOREACH_SAFE(lle, &dchain, lle_chain, next) {
- if (callout_halt(&lle->la_timer, &lle->lle_lock))
- LLE_REMREF(lle);
+ /*
+ * We need to release the lock here to lle_timer proceeds;
+ * lle_timer should stop immediately if LLE_LINKED isn't set.
+ * Note that we cannot pass lle->lle_lock to callout_halt
+ * because it's a rwlock.
+ */
+ LLE_ADDREF(lle);
+ LLE_WUNLOCK(lle);
+#ifdef NET_MPSAFE
+ callout_halt(&lle->la_timer, NULL);
+#else
+ if (mutex_owned(softnet_lock))
+ callout_halt(&lle->la_timer, softnet_lock);
+ else
+ callout_halt(&lle->la_timer, NULL);
+#endif
+ LLE_WLOCK(lle);
+ LLE_REMREF(lle);
llentry_free(lle);
}
@@ -557,6 +573,13 @@ lltable_unlink_entry(struct lltable *llt
}
void
+lltable_free_entry(struct lltable *llt, struct llentry *lle)
+{
+
+ llt->llt_free_entry(llt, lle);
+}
+
+void
lltable_fill_sa_entry(const struct llentry *lle, struct sockaddr *sa)
{
struct lltable *llt;
Index: src/sys/net/if_llatbl.h
diff -u src/sys/net/if_llatbl.h:1.9 src/sys/net/if_llatbl.h:1.10
--- src/sys/net/if_llatbl.h:1.9 Mon Apr 4 07:37:07 2016
+++ src/sys/net/if_llatbl.h Wed Dec 21 08:47:02 2016
@@ -1,4 +1,4 @@
-/* $NetBSD: if_llatbl.h,v 1.9 2016/04/04 07:37:07 ozaki-r Exp $ */
+/* $NetBSD: if_llatbl.h,v 1.10 2016/12/21 08:47:02 ozaki-r Exp $ */
/*
* Copyright (c) 2004 Luigi Rizzo, Alessandro Cerri. All rights reserved.
* Copyright (c) 2004-2008 Qing Li. All rights reserved.
@@ -273,6 +273,7 @@ struct llentry *lltable_create_lle(struc
const void *paddr);
void lltable_link_entry(struct lltable *llt, struct llentry *lle);
void lltable_unlink_entry(struct lltable *llt, struct llentry *lle);
+void lltable_free_entry(struct lltable *llt, struct llentry *lle);
void lltable_fill_sa_entry(const struct llentry *lle, struct sockaddr *sa);
struct ifnet *lltable_get_ifp(const struct lltable *llt);
int lltable_get_af(const struct lltable *llt);
Index: src/sys/netinet6/in6.c
diff -u src/sys/netinet6/in6.c:1.225 src/sys/netinet6/in6.c:1.226
--- src/sys/netinet6/in6.c:1.225 Mon Dec 19 07:51:34 2016
+++ src/sys/netinet6/in6.c Wed Dec 21 08:47:02 2016
@@ -1,4 +1,4 @@
-/* $NetBSD: in6.c,v 1.225 2016/12/19 07:51:34 ozaki-r Exp $ */
+/* $NetBSD: in6.c,v 1.226 2016/12/21 08:47:02 ozaki-r Exp $ */
/* $KAME: in6.c,v 1.198 2001/07/18 09:12:38 itojun Exp $ */
/*
@@ -62,7 +62,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: in6.c,v 1.225 2016/12/19 07:51:34 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: in6.c,v 1.226 2016/12/21 08:47:02 ozaki-r Exp $");
#ifdef _KERNEL_OPT
#include "opt_inet.h"
@@ -2362,28 +2362,42 @@ in6_lltable_match_prefix(const struct so
static void
in6_lltable_free_entry(struct lltable *llt, struct llentry *lle)
{
- struct ifnet *ifp __diagused;
+ struct ifnet *ifp = llt->llt_ifp;
+ IF_AFDATA_WLOCK_ASSERT(ifp);
LLE_WLOCK_ASSERT(lle);
- KASSERT(llt != NULL);
/* Unlink entry from table */
if ((lle->la_flags & LLE_LINKED) != 0) {
- ifp = llt->llt_ifp;
- IF_AFDATA_WLOCK_ASSERT(ifp);
lltable_unlink_entry(llt, lle);
+ KASSERT((lle->la_flags & LLE_LINKED) == 0);
}
+ /*
+ * We need to release the lock here to lle_timer proceeds;
+ * lle_timer should stop immediately if LLE_LINKED isn't set.
+ * Note that we cannot pass lle->lle_lock to callout_halt
+ * because it's a rwlock.
+ */
+ LLE_ADDREF(lle);
+ LLE_WUNLOCK(lle);
+ IF_AFDATA_WUNLOCK(ifp);
#ifdef NET_MPSAFE
callout_halt(&lle->lle_timer, NULL);
#else
- KASSERT(mutex_owned(softnet_lock));
- callout_halt(&lle->lle_timer, softnet_lock);
+ if (mutex_owned(softnet_lock))
+ callout_halt(&lle->lle_timer, softnet_lock);
+ else
+ callout_halt(&lle->lle_timer, NULL);
#endif
+ LLE_WLOCK(lle);
LLE_REMREF(lle);
- llentry_free(lle);
+ lltable_drop_entry_queue(lle);
+ LLE_FREE_LOCKED(lle);
+
+ IF_AFDATA_WLOCK(ifp);
}
static int
Index: src/sys/netinet6/nd6.c
diff -u src/sys/netinet6/nd6.c:1.221 src/sys/netinet6/nd6.c:1.222
--- src/sys/netinet6/nd6.c:1.221 Wed Dec 21 04:08:47 2016
+++ src/sys/netinet6/nd6.c Wed Dec 21 08:47:02 2016
@@ -1,4 +1,4 @@
-/* $NetBSD: nd6.c,v 1.221 2016/12/21 04:08:47 ozaki-r Exp $ */
+/* $NetBSD: nd6.c,v 1.222 2016/12/21 08:47:02 ozaki-r Exp $ */
/* $KAME: nd6.c,v 1.279 2002/06/08 11:16:51 itojun Exp $ */
/*
@@ -31,7 +31,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: nd6.c,v 1.221 2016/12/21 04:08:47 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: nd6.c,v 1.222 2016/12/21 08:47:02 ozaki-r Exp $");
#ifdef _KERNEL_OPT
#include "opt_net_mpsafe.h"
@@ -401,22 +401,18 @@ nd6_llinfo_settimer(struct llentry *ln,
CTASSERT(sizeof(time_t) > sizeof(int));
LLE_WLOCK_ASSERT(ln);
- if (xtick < 0) {
- ln->ln_expire = 0;
- ln->ln_ntick = 0;
- callout_halt(&ln->ln_timer_ch, &ln->lle_lock);
+ KASSERT(xtick >= 0);
+
+ ln->ln_expire = time_uptime + xtick / hz;
+ LLE_ADDREF(ln);
+ if (xtick > INT_MAX) {
+ ln->ln_ntick = xtick - INT_MAX;
+ callout_reset(&ln->ln_timer_ch, INT_MAX,
+ nd6_llinfo_timer, ln);
} else {
- ln->ln_expire = time_uptime + xtick / hz;
- LLE_ADDREF(ln);
- if (xtick > INT_MAX) {
- ln->ln_ntick = xtick - INT_MAX;
- callout_reset(&ln->ln_timer_ch, INT_MAX,
- nd6_llinfo_timer, ln);
- } else {
- ln->ln_ntick = 0;
- callout_reset(&ln->ln_timer_ch, xtick,
- nd6_llinfo_timer, ln);
- }
+ ln->ln_ntick = 0;
+ callout_reset(&ln->ln_timer_ch, xtick,
+ nd6_llinfo_timer, ln);
}
}
@@ -456,6 +452,8 @@ nd6_llinfo_timer(void *arg)
const struct in6_addr *daddr6 = NULL;
LLE_WLOCK(ln);
+ if ((ln->la_flags & LLE_LINKED) == 0)
+ goto out;
if (ln->ln_ntick > 0) {
nd6_llinfo_settimer(ln, ln->ln_ntick);
goto out;
@@ -1208,9 +1206,6 @@ nd6_free(struct llentry *ln, int gc)
* even though it is not harmful, it was not really necessary.
*/
- /* cancel timer */
- nd6_llinfo_settimer(ln, -1);
-
if (!ip6_forwarding) {
ND6_WLOCK();
dr = nd6_defrouter_lookup(in6, ifp);
@@ -1309,12 +1304,7 @@ nd6_free(struct llentry *ln, int gc)
IF_AFDATA_LOCK(ifp);
LLE_WLOCK(ln);
- /* Guard against race with other llentry_free(). */
- if (ln->la_flags & LLE_LINKED) {
- LLE_REMREF(ln);
- llentry_free(ln);
- } else
- LLE_FREE_LOCKED(ln);
+ lltable_free_entry(LLTABLE6(ifp), ln);
IF_AFDATA_UNLOCK(ifp);
}
