Source-Changes-HG archive

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

[src/trunk]: src/sys Fix reference leaks of llentry



details:   https://anonhg.NetBSD.org/src/rev/9e6ed57b2258
branches:  trunk
changeset: 360306:9e6ed57b2258
user:      ozaki-r <ozaki-r%NetBSD.org@localhost>
date:      Tue Mar 06 07:24:01 2018 +0000

description:
Fix reference leaks of llentry

callout_reset and callout_halt can cancel a pending callout without telling us.
Detect a cancel and remove a reference by using callout_pending and
callout_stop (it's a bit tricy though, we can detect it).

While here, we can remove remaining abuses of mutex_owned for softnet_lock.

diffstat:

 sys/net/if_llatbl.c  |  38 +++++++++++++++-----------------------
 sys/netinet/if_arp.c |  44 +++++++++++++++++++++++---------------------
 sys/netinet/in.c     |  35 ++---------------------------------
 sys/netinet6/in6.c   |  42 +++---------------------------------------
 sys/netinet6/nd6.c   |  38 ++++++++++++++++----------------------
 5 files changed, 59 insertions(+), 138 deletions(-)

diffs (truncated from 338 to 300 lines):

diff -r 95cae087c50d -r 9e6ed57b2258 sys/net/if_llatbl.c
--- a/sys/net/if_llatbl.c       Tue Mar 06 07:20:41 2018 +0000
+++ b/sys/net/if_llatbl.c       Tue Mar 06 07:24:01 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if_llatbl.c,v 1.23 2018/02/14 14:15:53 maxv Exp $      */
+/*     $NetBSD: if_llatbl.c,v 1.24 2018/03/06 07:24:01 ozaki-r Exp $   */
 /*
  * Copyright (c) 2004 Luigi Rizzo, Alessandro Cerri. All rights reserved.
  * Copyright (c) 2004-2008 Qing Li. All rights reserved.
@@ -358,6 +358,18 @@
                llt->llt_unlink_entry(lle);
        }
 
+       /*
+        * Stop a pending callout if one exists.  If we cancel one, we have to
+        * remove a reference to avoid a leak.  callout_pending is required to
+        * to exclude the case that the callout has never been scheduled.
+        */
+       /* XXX once softnet_lock goes away, we should use callout_halt */
+       if (callout_pending(&lle->la_timer)) {
+               bool expired = callout_stop(&lle->la_timer);
+               if (!expired)
+                       LLE_REMREF(lle);
+       }
+
        pkts_dropped = lltable_drop_entry_queue(lle);
 
        LLE_FREE_LOCKED(lle);
@@ -428,28 +440,8 @@
        llentries_unlink(llt, &dchain);
        IF_AFDATA_WUNLOCK(llt->llt_ifp);
 
-       LIST_FOREACH_SAFE(lle, &dchain, lle_chain, next) {
-               /*
-                * 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);
-       }
-
+       LIST_FOREACH_SAFE(lle, &dchain, lle_chain, next)
+               (void)llentry_free(lle);
 }
 
 /*
diff -r 95cae087c50d -r 9e6ed57b2258 sys/netinet/if_arp.c
--- a/sys/netinet/if_arp.c      Tue Mar 06 07:20:41 2018 +0000
+++ b/sys/netinet/if_arp.c      Tue Mar 06 07:24:01 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if_arp.c,v 1.269 2018/03/06 07:19:03 ozaki-r Exp $     */
+/*     $NetBSD: if_arp.c,v 1.270 2018/03/06 07:24:01 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.269 2018/03/06 07:19:03 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_arp.c,v 1.270 2018/03/06 07:24:01 ozaki-r Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_ddb.h"
@@ -318,29 +318,18 @@
        KASSERT((lle->la_flags & LLE_STATIC) == 0);
 
        LLE_WLOCK(lle);
-       if (callout_pending(&lle->la_timer)) {
-               /*
-                * Here we are a bit odd in the treatment of
-                * active/pending. If the pending bit is set, it got
-                * rescheduled before I ran. The active
-                * bit we ignore, since if it was stopped
-                * in ll_tablefree() and was currently running
-                * it would have return 0 so the code would
-                * not have deleted it since the callout could
-                * not be stopped so we want to go through
-                * with the delete here now. If the callout
-                * was restarted, the pending bit will be back on and
-                * we just want to bail since the callout_reset would
-                * return 1 and our reference would have been removed
-                * by arpresolve() below.
-                */
-               LLE_WUNLOCK(lle);
+
+       /*
+        * This shortcut is required to avoid trying to touch ifp that may be
+        * being destroyed.
+        */
+       if ((lle->la_flags & LLE_LINKED) == 0) {
+               LLE_FREE_LOCKED(lle);
                return;
        }
+
        ifp = lle->lle_tbl->llt_ifp;
 
-       callout_stop(&lle->la_timer);
-
        /* XXX: LOR avoidance. We still have ref on lle. */
        LLE_WUNLOCK(lle);
 
@@ -369,6 +358,19 @@
        LLE_WLOCK_ASSERT(la);
        KASSERT((la->la_flags & LLE_STATIC) == 0);
 
+       /*
+        * We have to take care of a reference leak which occurs if
+        * callout_reset overwrites a pending callout schedule.  Unfortunately
+        * we don't have a mean to know the overwrite, so we need to know it
+        * using callout_stop.  We need to call callout_pending first to exclude
+        * the case that the callout has never been scheduled.
+        */
+       if (callout_pending(&la->la_timer)) {
+               bool expired = callout_stop(&la->la_timer);
+               if (!expired)
+                       /* A pending callout schedule is canceled. */
+                       LLE_REMREF(la);
+       }
        LLE_ADDREF(la);
        callout_reset(&la->la_timer, hz * sec, arptimer, la);
 }
diff -r 95cae087c50d -r 9e6ed57b2258 sys/netinet/in.c
--- a/sys/netinet/in.c  Tue Mar 06 07:20:41 2018 +0000
+++ b/sys/netinet/in.c  Tue Mar 06 07:24:01 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: in.c,v 1.220 2018/03/06 07:20:41 ozaki-r Exp $ */
+/*     $NetBSD: in.c,v 1.221 2018/03/06 07:24:01 ozaki-r Exp $ */
 
 /*
  * Copyright (C) 1995, 1996, 1997, and 1998 WIDE Project.
@@ -91,7 +91,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: in.c,v 1.220 2018/03/06 07:20:41 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: in.c,v 1.221 2018/03/06 07:24:01 ozaki-r Exp $");
 
 #include "arp.h"
 
@@ -1992,44 +1992,13 @@
 static void
 in_lltable_free_entry(struct lltable *llt, struct llentry *lle)
 {
-       struct ifnet *ifp __diagused;
        size_t pkts_dropped;
-       bool locked = false;
 
        LLE_WLOCK_ASSERT(lle);
        KASSERT(llt != NULL);
 
-       /* Unlink entry from table if not already */
-       if ((lle->la_flags & LLE_LINKED) != 0) {
-               ifp = llt->llt_ifp;
-               IF_AFDATA_WLOCK_ASSERT(ifp);
-               lltable_unlink_entry(llt, lle);
-               locked = true;
-       }
-
-       /*
-        * 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 (locked)
-               IF_AFDATA_WUNLOCK(ifp);
-
-       /* cancel timer */
-       callout_halt(&lle->lle_timer, NULL);
-
-       LLE_WLOCK(lle);
-       LLE_REMREF(lle);
-
-       /* Drop hold queue */
        pkts_dropped = llentry_free(lle);
        arp_stat_add(ARP_STAT_DFRDROPPED, (uint64_t)pkts_dropped);
-
-       if (locked)
-               IF_AFDATA_WLOCK(ifp);
 }
 
 static int
diff -r 95cae087c50d -r 9e6ed57b2258 sys/netinet6/in6.c
--- a/sys/netinet6/in6.c        Tue Mar 06 07:20:41 2018 +0000
+++ b/sys/netinet6/in6.c        Tue Mar 06 07:24:01 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: in6.c,v 1.261 2018/03/06 07:20:41 ozaki-r Exp $        */
+/*     $NetBSD: in6.c,v 1.262 2018/03/06 07:24:01 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.261 2018/03/06 07:20:41 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: in6.c,v 1.262 2018/03/06 07:24:01 ozaki-r Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_inet.h"
@@ -2446,45 +2446,9 @@
 static void
 in6_lltable_free_entry(struct lltable *llt, struct llentry *lle)
 {
-       struct ifnet *ifp = llt->llt_ifp;
-       bool locked = false;
 
        LLE_WLOCK_ASSERT(lle);
-
-       /* Unlink entry from table */
-       if ((lle->la_flags & LLE_LINKED) != 0) {
-               IF_AFDATA_WLOCK_ASSERT(ifp);
-               lltable_unlink_entry(llt, lle);
-               KASSERT((lle->la_flags & LLE_LINKED) == 0);
-               locked = true;
-       }
-       /*
-        * 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 (locked)
-               IF_AFDATA_WUNLOCK(ifp);
-
-#ifdef NET_MPSAFE
-       callout_halt(&lle->lle_timer, NULL);
-#else
-       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);
-
-       lltable_drop_entry_queue(lle);
-       LLE_FREE_LOCKED(lle);
-
-       if (locked)
-               IF_AFDATA_WLOCK(ifp);
+       (void) llentry_free(lle);
 }
 
 static int
diff -r 95cae087c50d -r 9e6ed57b2258 sys/netinet6/nd6.c
--- a/sys/netinet6/nd6.c        Tue Mar 06 07:20:41 2018 +0000
+++ b/sys/netinet6/nd6.c        Tue Mar 06 07:24:01 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: nd6.c,v 1.245 2018/01/29 19:51:15 christos Exp $       */
+/*     $NetBSD: nd6.c,v 1.246 2018/03/06 07:24:01 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.245 2018/01/29 19:51:15 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: nd6.c,v 1.246 2018/03/06 07:24:01 ozaki-r Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_net_mpsafe.h"
@@ -402,6 +402,19 @@
 
        KASSERT(xtick >= 0);
 
+       /*
+        * We have to take care of a reference leak which occurs if
+        * callout_reset overwrites a pending callout schedule.  Unfortunately
+        * we don't have a mean to know the overwrite, so we need to know it
+        * using callout_stop.  We need to call callout_pending first to exclude
+        * the case that the callout has never been scheduled.
+        */
+       if (callout_pending(&ln->la_timer)) {
+               bool expired = callout_stop(&ln->la_timer);
+               if (!expired)
+                       LLE_REMREF(ln);
+       }
+
        ln->ln_expire = time_uptime + xtick / hz;
        LLE_ADDREF(ln);



Home | Main Index | Thread Index | Old Index