Module Name: src
Committed By: ozaki-r
Date: Tue Mar 6 07:24:01 UTC 2018
Modified Files:
src/sys/net: if_llatbl.c
src/sys/netinet: if_arp.c in.c
src/sys/netinet6: in6.c nd6.c
Log Message:
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.
To generate a diff of this commit:
cvs rdiff -u -r1.23 -r1.24 src/sys/net/if_llatbl.c
cvs rdiff -u -r1.269 -r1.270 src/sys/netinet/if_arp.c
cvs rdiff -u -r1.220 -r1.221 src/sys/netinet/in.c
cvs rdiff -u -r1.261 -r1.262 src/sys/netinet6/in6.c
cvs rdiff -u -r1.245 -r1.246 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.23 src/sys/net/if_llatbl.c:1.24
--- src/sys/net/if_llatbl.c:1.23 Wed Feb 14 14:15:53 2018
+++ src/sys/net/if_llatbl.c Tue Mar 6 07:24:01 2018
@@ -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 @@ llentry_free(struct llentry *lle)
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 @@ lltable_purge_entries(struct lltable *ll
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);
}
/*
Index: src/sys/netinet/if_arp.c
diff -u src/sys/netinet/if_arp.c:1.269 src/sys/netinet/if_arp.c:1.270
--- src/sys/netinet/if_arp.c:1.269 Tue Mar 6 07:19:03 2018
+++ src/sys/netinet/if_arp.c Tue Mar 6 07:24:01 2018
@@ -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,28 +318,17 @@ arptimer(void *arg)
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);
+ ifp = lle->lle_tbl->llt_ifp;
/* XXX: LOR avoidance. We still have ref on lle. */
LLE_WUNLOCK(lle);
@@ -369,6 +358,19 @@ arp_settimer(struct llentry *la, int sec
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);
}
Index: src/sys/netinet/in.c
diff -u src/sys/netinet/in.c:1.220 src/sys/netinet/in.c:1.221
--- src/sys/netinet/in.c:1.220 Tue Mar 6 07:20:41 2018
+++ src/sys/netinet/in.c Tue Mar 6 07:24:01 2018
@@ -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 @@ in_lltable_match_prefix(const struct soc
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
Index: src/sys/netinet6/in6.c
diff -u src/sys/netinet6/in6.c:1.261 src/sys/netinet6/in6.c:1.262
--- src/sys/netinet6/in6.c:1.261 Tue Mar 6 07:20:41 2018
+++ src/sys/netinet6/in6.c Tue Mar 6 07:24:01 2018
@@ -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 @@ in6_lltable_match_prefix(const struct so
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
Index: src/sys/netinet6/nd6.c
diff -u src/sys/netinet6/nd6.c:1.245 src/sys/netinet6/nd6.c:1.246
--- src/sys/netinet6/nd6.c:1.245 Mon Jan 29 19:51:15 2018
+++ src/sys/netinet6/nd6.c Tue Mar 6 07:24:01 2018
@@ -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 @@ nd6_llinfo_settimer(struct llentry *ln,
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);
if (xtick > INT_MAX) {
@@ -451,6 +464,7 @@ nd6_llinfo_timer(void *arg)
const struct in6_addr *daddr6 = NULL;
SOFTNET_KERNEL_LOCK_UNLESS_NET_MPSAFE();
+
LLE_WLOCK(ln);
if ((ln->la_flags & LLE_LINKED) == 0)
goto out;
@@ -459,28 +473,8 @@ nd6_llinfo_timer(void *arg)
goto out;
}
- if (callout_pending(&ln->la_timer)) {
- /*
- * Here we are a bit odd here 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 nd6_llinfo_settimer above since canceled
- * would have been 1.
- */
- goto out;
- }
ifp = ln->lle_tbl->llt_ifp;
-
KASSERT(ifp != NULL);
ndi = ND_IFINFO(ifp);
