Date: Thu, 2 Aug 2012 14:00:26 GMT From: dfilter@FreeBSD.ORG (dfilter service) To: freebsd-net@FreeBSD.org Subject: Re: kern/165863: commit references a PR Message-ID: <201208021400.q72E0Qu2057088@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/165863; it has been noted by GNATS. From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/165863: commit references a PR Date: Thu, 2 Aug 2012 13:58:02 +0000 (UTC) Author: glebius Date: Thu Aug 2 13:57:49 2012 New Revision: 238990 URL: http://svn.freebsd.org/changeset/base/238990 Log: Fix races between in_lltable_prefix_free(), lla_lookup(), llentry_free() and arptimer(): o Use callout_init_rw() for lle timeout, this allows us safely disestablish them. - This allows us to simplify the arptimer() and make it race safe. o Consistently use ifp->if_afdata_lock to lock access to linked lists in the lle hashes. o Introduce new lle flag LLE_LINKED, which marks an entry that is attached to the hash. - Use LLE_LINKED to avoid double unlinking via consequent calls to llentry_free(). - Mark lle with LLE_DELETED via |= operation istead of =, so that other flags won't be lost. o Make LLE_ADDREF(), LLE_REMREF() and LLE_FREE_LOCKED() more consistent and provide more informative KASSERTs. The patch is a collaborative work of all submitters and myself. PR: kern/165863 Submitted by: Andrey Zonov <andrey zonov.org> Submitted by: Ryan Stone <rysto32 gmail.com> Submitted by: Eric van Gyzen <eric_van_gyzen dell.com> Modified: head/sys/net/if_llatbl.c head/sys/net/if_llatbl.h head/sys/net/if_var.h head/sys/netinet/if_ether.c head/sys/netinet/in.c head/sys/netinet6/in6.c Modified: head/sys/net/if_llatbl.c ============================================================================== --- head/sys/net/if_llatbl.c Thu Aug 2 13:20:44 2012 (r238989) +++ head/sys/net/if_llatbl.c Thu Aug 2 13:57:49 2012 (r238990) @@ -106,10 +106,19 @@ llentry_free(struct llentry *lle) size_t pkts_dropped; struct mbuf *next; - pkts_dropped = 0; + IF_AFDATA_WLOCK_ASSERT(lle->lle_tbl->llt_ifp); LLE_WLOCK_ASSERT(lle); + + /* XXX: guard against race with other llentry_free(). */ + if (!(lle->la_flags & LLE_LINKED)) { + LLE_FREE_LOCKED(lle); + return (0); + } + LIST_REMOVE(lle, lle_next); + lle->la_flags &= ~(LLE_VALID | LLE_LINKED); + pkts_dropped = 0; while ((lle->la_numheld > 0) && (lle->la_hold != NULL)) { next = lle->la_hold->m_nextpkt; m_freem(lle->la_hold); @@ -122,7 +131,6 @@ llentry_free(struct llentry *lle) ("%s: la_numheld %d > 0, pkts_droped %zd", __func__, lle->la_numheld, pkts_dropped)); - lle->la_flags &= ~LLE_VALID; LLE_FREE_LOCKED(lle); return (pkts_dropped); @@ -173,17 +181,16 @@ lltable_free(struct lltable *llt) SLIST_REMOVE(&V_lltables, llt, lltable, llt_link); LLTABLE_WUNLOCK(); + IF_AFDATA_WLOCK(llt->llt_ifp); for (i = 0; i < LLTBL_HASHTBL_SIZE; i++) { LIST_FOREACH_SAFE(lle, &llt->lle_head[i], lle_next, next) { - int canceled; - - canceled = callout_drain(&lle->la_timer); LLE_WLOCK(lle); - if (canceled) + if (callout_stop(&lle->la_timer)) LLE_REMREF(lle); llentry_free(lle); } } + IF_AFDATA_WUNLOCK(llt->llt_ifp); free(llt, M_LLTABLE); } Modified: head/sys/net/if_llatbl.h ============================================================================== --- head/sys/net/if_llatbl.h Thu Aug 2 13:20:44 2012 (r238989) +++ head/sys/net/if_llatbl.h Thu Aug 2 13:57:49 2012 (r238990) @@ -103,26 +103,28 @@ struct llentry { #define LLE_ADDREF(lle) do { \ LLE_WLOCK_ASSERT(lle); \ KASSERT((lle)->lle_refcnt >= 0, \ - ("negative refcnt %d", (lle)->lle_refcnt)); \ + ("negative refcnt %d on lle %p", \ + (lle)->lle_refcnt, (lle))); \ (lle)->lle_refcnt++; \ } while (0) #define LLE_REMREF(lle) do { \ LLE_WLOCK_ASSERT(lle); \ - KASSERT((lle)->lle_refcnt > 1, \ - ("bogus refcnt %d", (lle)->lle_refcnt)); \ + KASSERT((lle)->lle_refcnt > 0, \ + ("bogus refcnt %d on lle %p", \ + (lle)->lle_refcnt, (lle))); \ (lle)->lle_refcnt--; \ } while (0) #define LLE_FREE_LOCKED(lle) do { \ - if ((lle)->lle_refcnt <= 1) \ - (lle)->lle_free((lle)->lle_tbl, (lle));\ + if ((lle)->lle_refcnt == 1) \ + (lle)->lle_free((lle)->lle_tbl, (lle)); \ else { \ - (lle)->lle_refcnt--; \ + LLE_REMREF(lle); \ LLE_WUNLOCK(lle); \ } \ /* guard against invalid refs */ \ - lle = NULL; \ + (lle) = NULL; \ } while (0) #define LLE_FREE(lle) do { \ @@ -172,6 +174,7 @@ MALLOC_DECLARE(M_LLTABLE); #define LLE_VALID 0x0008 /* ll_addr is valid */ #define LLE_PROXY 0x0010 /* proxy entry ??? */ #define LLE_PUB 0x0020 /* publish entry ??? */ +#define LLE_LINKED 0x0040 /* linked to lookup structure */ #define LLE_EXCLUSIVE 0x2000 /* return lle xlocked */ #define LLE_DELETE 0x4000 /* delete on a lookup - match LLE_IFADDR */ #define LLE_CREATE 0x8000 /* create on a lookup miss */ Modified: head/sys/net/if_var.h ============================================================================== --- head/sys/net/if_var.h Thu Aug 2 13:20:44 2012 (r238989) +++ head/sys/net/if_var.h Thu Aug 2 13:57:49 2012 (r238990) @@ -415,6 +415,8 @@ EVENTHANDLER_DECLARE(group_change_event, #define IF_AFDATA_DESTROY(ifp) rw_destroy(&(ifp)->if_afdata_lock) #define IF_AFDATA_LOCK_ASSERT(ifp) rw_assert(&(ifp)->if_afdata_lock, RA_LOCKED) +#define IF_AFDATA_RLOCK_ASSERT(ifp) rw_assert(&(ifp)->if_afdata_lock, RA_RLOCKED) +#define IF_AFDATA_WLOCK_ASSERT(ifp) rw_assert(&(ifp)->if_afdata_lock, RA_WLOCKED) #define IF_AFDATA_UNLOCK_ASSERT(ifp) rw_assert(&(ifp)->if_afdata_lock, RA_UNLOCKED) int if_handoff(struct ifqueue *ifq, struct mbuf *m, struct ifnet *ifp, Modified: head/sys/netinet/if_ether.c ============================================================================== --- head/sys/netinet/if_ether.c Thu Aug 2 13:20:44 2012 (r238989) +++ head/sys/netinet/if_ether.c Thu Aug 2 13:57:49 2012 (r238990) @@ -163,49 +163,40 @@ arp_ifscrub(struct ifnet *ifp, uint32_t static void arptimer(void *arg) { + struct llentry *lle = (struct llentry *)arg; struct ifnet *ifp; - struct llentry *lle; - int pkts_dropped; + size_t pkts_dropped; + + if (lle->la_flags & LLE_STATIC) { + LLE_WUNLOCK(lle); + return; + } - KASSERT(arg != NULL, ("%s: arg NULL", __func__)); - lle = (struct llentry *)arg; ifp = lle->lle_tbl->llt_ifp; CURVNET_SET(ifp->if_vnet); + + if (lle->la_flags != LLE_DELETED) { + int evt; + + if (lle->la_flags & LLE_VALID) + evt = LLENTRY_EXPIRED; + else + evt = LLENTRY_TIMEDOUT; + EVENTHANDLER_INVOKE(lle_event, lle, evt); + } + + callout_stop(&lle->la_timer); + + /* XXX: LOR avoidance. We still have ref on lle. */ + LLE_WUNLOCK(lle); IF_AFDATA_LOCK(ifp); LLE_WLOCK(lle); - if (lle->la_flags & LLE_STATIC) - LLE_WUNLOCK(lle); - else { - if (!callout_pending(&lle->la_timer) && - callout_active(&lle->la_timer)) { - callout_stop(&lle->la_timer); - LLE_REMREF(lle); - - if (lle->la_flags != LLE_DELETED) { - int evt; - - if (lle->la_flags & LLE_VALID) - evt = LLENTRY_EXPIRED; - else - evt = LLENTRY_TIMEDOUT; - EVENTHANDLER_INVOKE(lle_event, lle, evt); - } - pkts_dropped = llentry_free(lle); - ARPSTAT_ADD(dropped, pkts_dropped); - ARPSTAT_INC(timeouts); - } else { -#ifdef DIAGNOSTIC - struct sockaddr *l3addr = L3_ADDR(lle); - log(LOG_INFO, - "arptimer issue: %p, IPv4 address: \"%s\"\n", lle, - inet_ntoa( - ((const struct sockaddr_in *)l3addr)->sin_addr)); -#endif - LLE_WUNLOCK(lle); - } - } + LLE_REMREF(lle); + pkts_dropped = llentry_free(lle); IF_AFDATA_UNLOCK(ifp); + ARPSTAT_ADD(dropped, pkts_dropped); + ARPSTAT_INC(timeouts); CURVNET_RESTORE(); } Modified: head/sys/netinet/in.c ============================================================================== --- head/sys/netinet/in.c Thu Aug 2 13:20:44 2012 (r238989) +++ head/sys/netinet/in.c Thu Aug 2 13:57:49 2012 (r238990) @@ -1280,7 +1280,6 @@ in_lltable_new(const struct sockaddr *l3 if (lle == NULL) /* NB: caller generates msg */ return NULL; - callout_init(&lle->base.la_timer, CALLOUT_MPSAFE); /* * For IPv4 this will trigger "arpresolve" to generate * an ARP request. @@ -1290,7 +1289,10 @@ in_lltable_new(const struct sockaddr *l3 lle->base.lle_refcnt = 1; lle->base.lle_free = in_lltable_free; LLE_LOCK_INIT(&lle->base); - return &lle->base; + callout_init_rw(&lle->base.la_timer, &lle->base.lle_lock, + CALLOUT_RETURNUNLOCKED); + + return (&lle->base); } #define IN_ARE_MASKED_ADDR_EQUAL(d, a, m) ( \ @@ -1306,6 +1308,7 @@ in_lltable_prefix_free(struct lltable *l int i; size_t pkts_dropped; + IF_AFDATA_WLOCK(llt->llt_ifp); for (i = 0; i < LLTBL_HASHTBL_SIZE; i++) { LIST_FOREACH_SAFE(lle, &llt->lle_head[i], lle_next, next) { /* @@ -1315,17 +1318,15 @@ in_lltable_prefix_free(struct lltable *l if (IN_ARE_MASKED_ADDR_EQUAL(satosin(L3_ADDR(lle)), pfx, msk) && ((flags & LLE_STATIC) || !(lle->la_flags & LLE_STATIC))) { - int canceled; - - canceled = callout_drain(&lle->la_timer); LLE_WLOCK(lle); - if (canceled) + if (callout_stop(&lle->la_timer)) LLE_REMREF(lle); pkts_dropped = llentry_free(lle); ARPSTAT_ADD(dropped, pkts_dropped); } } } + IF_AFDATA_WUNLOCK(llt->llt_ifp); } @@ -1457,11 +1458,12 @@ in_lltable_lookup(struct lltable *llt, u lle->lle_tbl = llt; lle->lle_head = lleh; + lle->la_flags |= LLE_LINKED; LIST_INSERT_HEAD(lleh, lle, lle_next); } else if (flags & LLE_DELETE) { if (!(lle->la_flags & LLE_IFADDR) || (flags & LLE_IFADDR)) { LLE_WLOCK(lle); - lle->la_flags = LLE_DELETED; + lle->la_flags |= LLE_DELETED; EVENTHANDLER_INVOKE(lle_event, lle, LLENTRY_DELETED); LLE_WUNLOCK(lle); #ifdef DIAGNOSTIC Modified: head/sys/netinet6/in6.c ============================================================================== --- head/sys/netinet6/in6.c Thu Aug 2 13:20:44 2012 (r238989) +++ head/sys/netinet6/in6.c Thu Aug 2 13:57:49 2012 (r238990) @@ -2497,23 +2497,22 @@ in6_lltable_prefix_free(struct lltable * * (flags & LLE_STATIC) means deleting all entries * including static ND6 entries. */ + IF_AFDATA_WLOCK(llt->llt_ifp); for (i = 0; i < LLTBL_HASHTBL_SIZE; i++) { LIST_FOREACH_SAFE(lle, &llt->lle_head[i], lle_next, next) { if (IN6_ARE_MASKED_ADDR_EQUAL( - &((struct sockaddr_in6 *)L3_ADDR(lle))->sin6_addr, - &pfx->sin6_addr, - &msk->sin6_addr) && - ((flags & LLE_STATIC) || !(lle->la_flags & LLE_STATIC))) { - int canceled; - - canceled = callout_drain(&lle->la_timer); + &satosin6(L3_ADDR(lle))->sin6_addr, + &pfx->sin6_addr, &msk->sin6_addr) && + ((flags & LLE_STATIC) || + !(lle->la_flags & LLE_STATIC))) { LLE_WLOCK(lle); - if (canceled) + if (callout_stop(&lle->la_timer)) LLE_REMREF(lle); llentry_free(lle); } } } + IF_AFDATA_WUNLOCK(llt->llt_ifp); } static int @@ -2605,11 +2604,12 @@ in6_lltable_lookup(struct lltable *llt, lle->lle_tbl = llt; lle->lle_head = lleh; + lle->la_flags |= LLE_LINKED; LIST_INSERT_HEAD(lleh, lle, lle_next); } else if (flags & LLE_DELETE) { if (!(lle->la_flags & LLE_IFADDR) || (flags & LLE_IFADDR)) { LLE_WLOCK(lle); - lle->la_flags = LLE_DELETED; + lle->la_flags |= LLE_DELETED; LLE_WUNLOCK(lle); #ifdef DIAGNOSTIC log(LOG_INFO, "ifaddr cache = %p is deleted\n", lle); _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201208021400.q72E0Qu2057088>