Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys Fix deadlock between llentry timers and destruction of l...



details:   https://anonhg.NetBSD.org/src/rev/02eb447e7d5c
branches:  trunk
changeset: 349736:02eb447e7d5c
user:      ozaki-r <ozaki-r%NetBSD.org@localhost>
date:      Wed Dec 21 08:47:02 2016 +0000

description:
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...

diffstat:

 sys/net/if_llatbl.c |  29 ++++++++++++++++++++++++++---
 sys/net/if_llatbl.h |   3 ++-
 sys/netinet6/in6.c  |  32 +++++++++++++++++++++++---------
 sys/netinet6/nd6.c  |  42 ++++++++++++++++--------------------------
 4 files changed, 67 insertions(+), 39 deletions(-)

diffs (220 lines):

diff -r 053667e1a781 -r 02eb447e7d5c sys/net/if_llatbl.c
--- a/sys/net/if_llatbl.c       Wed Dec 21 07:02:16 2016 +0000
+++ b/sys/net/if_llatbl.c       Wed Dec 21 08:47:02 2016 +0000
@@ -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 @@
        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 @@
 }
 
 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;
diff -r 053667e1a781 -r 02eb447e7d5c sys/net/if_llatbl.h
--- a/sys/net/if_llatbl.h       Wed Dec 21 07:02:16 2016 +0000
+++ b/sys/net/if_llatbl.h       Wed Dec 21 08:47:02 2016 +0000
@@ -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 @@
     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);
diff -r 053667e1a781 -r 02eb447e7d5c sys/netinet6/in6.c
--- a/sys/netinet6/in6.c        Wed Dec 21 07:02:16 2016 +0000
+++ b/sys/netinet6/in6.c        Wed Dec 21 08:47:02 2016 +0000
@@ -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 @@
 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
diff -r 053667e1a781 -r 02eb447e7d5c sys/netinet6/nd6.c
--- a/sys/netinet6/nd6.c        Wed Dec 21 07:02:16 2016 +0000
+++ b/sys/netinet6/nd6.c        Wed Dec 21 08:47:02 2016 +0000
@@ -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 @@
        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 @@
        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 @@
         * 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 @@
        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);
 }



Home | Main Index | Thread Index | Old Index