From owner-freebsd-bugs@FreeBSD.ORG Fri Jul 23 07:50:03 2010 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id F30B91065675 for ; Fri, 23 Jul 2010 07:50:03 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id B62ED8FC12 for ; Fri, 23 Jul 2010 07:50:03 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id o6N7o3l7078380 for ; Fri, 23 Jul 2010 07:50:03 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.4/8.14.4/Submit) id o6N7o3CY078379; Fri, 23 Jul 2010 07:50:03 GMT (envelope-from gnats) Resent-Date: Fri, 23 Jul 2010 07:50:03 GMT Resent-Message-Id: <201007230750.o6N7o3CY078379@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Dmitrij Tejblum Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1FBE1106564A for ; Fri, 23 Jul 2010 07:49:27 +0000 (UTC) (envelope-from nobody@FreeBSD.org) Received: from www.freebsd.org (www.freebsd.org [IPv6:2001:4f8:fff6::21]) by mx1.freebsd.org (Postfix) with ESMTP id 0DFAB8FC0A for ; Fri, 23 Jul 2010 07:49:27 +0000 (UTC) Received: from www.freebsd.org (localhost [127.0.0.1]) by www.freebsd.org (8.14.3/8.14.3) with ESMTP id o6N7nQ5w077510 for ; Fri, 23 Jul 2010 07:49:26 GMT (envelope-from nobody@www.freebsd.org) Received: (from nobody@localhost) by www.freebsd.org (8.14.3/8.14.3/Submit) id o6N7nQOx077509; Fri, 23 Jul 2010 07:49:26 GMT (envelope-from nobody) Message-Id: <201007230749.o6N7nQOx077509@www.freebsd.org> Date: Fri, 23 Jul 2010 07:49:26 GMT From: Dmitrij Tejblum To: freebsd-gnats-submit@FreeBSD.org X-Send-Pr-Version: www-3.1 Cc: Subject: kern/148857: [patch] [ip6] Panics due to insufficient locking in netinet6/nd6.c X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Jul 2010 07:50:04 -0000 >Number: 148857 >Category: kern >Synopsis: [patch] [ip6] Panics due to insufficient locking in netinet6/nd6.c >Confidential: no >Severity: serious >Priority: medium >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Fri Jul 23 07:50:03 UTC 2010 >Closed-Date: >Last-Modified: >Originator: Dmitrij Tejblum >Release: 8.1-PRERELEASE >Organization: Yandex >Environment: >Description: nd6_llinfo_timer() heavily use and modify an `struct llentry' called `ln'. It should lock it, to protect against someone else work with the llentry. However, it does not. >How-To-Repeat: It is better to run kernel with INVARIANTS. Run ping6 to an on-link IPv6 address that is not assigned to any node. The system will panic after a few seconds. The panic is caused by immediate generation of DST_UNREACH icmp response (since the address is unreachable) and ping6 sending the next ping. Both of these actions work with ln->la_hold queue of packets. >Fix: I've tested the attached patch with INVARIANTS and WITNESS Patch attached with submission follows: --- sys/netinet6/nd6.c 2010-07-19 17:46:38.000000000 +0400 +++ sys/netinet6/nd6.c 2010-07-23 00:05:57.000000000 +0400 @@ -400,12 +400,13 @@ skip1: */ void nd6_llinfo_settimer_locked(struct llentry *ln, long tick) { int canceled; + LLE_WLOCK_ASSERT(ln); if (tick < 0) { ln->la_expire = 0; ln->ln_ntick = 0; canceled = callout_stop(&ln->ln_timer_ch); } else { ln->la_expire = time_second + tick / hz; @@ -437,31 +438,33 @@ static void nd6_llinfo_timer(void *arg) { struct llentry *ln; struct in6_addr *dst; struct ifnet *ifp; struct nd_ifinfo *ndi = NULL; + struct mbuf *m = NULL; ln = (struct llentry *)arg; if (ln == NULL) { panic("%s: NULL entry!\n", __func__); return; } + LLE_WLOCK_ASSERT(ln); if ((ifp = ((ln->lle_tbl != NULL) ? ln->lle_tbl->llt_ifp : NULL)) == NULL) panic("ln ifp == NULL"); CURVNET_SET(ifp->if_vnet); if (ln->ln_ntick > 0) { if (ln->ln_ntick > INT_MAX) { ln->ln_ntick -= INT_MAX; - nd6_llinfo_settimer(ln, INT_MAX); + nd6_llinfo_settimer_locked(ln, INT_MAX); } else { ln->ln_ntick = 0; - nd6_llinfo_settimer(ln, ln->ln_ntick); + nd6_llinfo_settimer_locked(ln, ln->ln_ntick); } goto done; } ndi = ND_IFINFO(ifp); dst = &L3_ADDR_SIN6(ln)->sin6_addr; @@ -476,39 +479,38 @@ nd6_llinfo_timer(void *arg) } switch (ln->ln_state) { case ND6_LLINFO_INCOMPLETE: if (ln->la_asked < V_nd6_mmaxtries) { ln->la_asked++; - nd6_llinfo_settimer(ln, (long)ndi->retrans * hz / 1000); + nd6_llinfo_settimer_locked(ln, (long)ndi->retrans * hz / 1000); nd6_ns_output(ifp, NULL, dst, ln, 0); } else { - struct mbuf *m = ln->la_hold; + m = ln->la_hold; if (m) { struct mbuf *m0; /* * assuming every packet in la_hold has the * same IP header */ m0 = m->m_nextpkt; m->m_nextpkt = NULL; - icmp6_error2(m, ICMP6_DST_UNREACH, - ICMP6_DST_UNREACH_ADDR, 0, ifp); + /* send error after unlock, to avoid reversal */ ln->la_hold = m0; clear_llinfo_pqueue(ln); } (void)nd6_free(ln, 0); ln = NULL; } break; case ND6_LLINFO_REACHABLE: if (!ND6_LLINFO_PERMANENT(ln)) { ln->ln_state = ND6_LLINFO_STALE; - nd6_llinfo_settimer(ln, (long)V_nd6_gctimer * hz); + nd6_llinfo_settimer_locked(ln, (long)V_nd6_gctimer * hz); } break; case ND6_LLINFO_STALE: /* Garbage Collection(RFC 2461 5.3) */ if (!ND6_LLINFO_PERMANENT(ln)) { @@ -519,33 +521,36 @@ nd6_llinfo_timer(void *arg) case ND6_LLINFO_DELAY: if (ndi && (ndi->flags & ND6_IFF_PERFORMNUD) != 0) { /* We need NUD */ ln->la_asked = 1; ln->ln_state = ND6_LLINFO_PROBE; - nd6_llinfo_settimer(ln, (long)ndi->retrans * hz / 1000); + nd6_llinfo_settimer_locked(ln, (long)ndi->retrans * hz / 1000); nd6_ns_output(ifp, dst, dst, ln, 0); } else { ln->ln_state = ND6_LLINFO_STALE; /* XXX */ - nd6_llinfo_settimer(ln, (long)V_nd6_gctimer * hz); + nd6_llinfo_settimer_locked(ln, (long)V_nd6_gctimer * hz); } break; case ND6_LLINFO_PROBE: if (ln->la_asked < V_nd6_umaxtries) { ln->la_asked++; - nd6_llinfo_settimer(ln, (long)ndi->retrans * hz / 1000); + nd6_llinfo_settimer_locked(ln, (long)ndi->retrans * hz / 1000); nd6_ns_output(ifp, dst, dst, ln, 0); } else { (void)nd6_free(ln, 0); ln = NULL; } break; } done: if (ln != NULL) - LLE_FREE(ln); + LLE_FREE_LOCKED(ln); + if (m != NULL) + icmp6_error2(m, ICMP6_DST_UNREACH, ICMP6_DST_UNREACH_ADDR, + 0, ifp); CURVNET_RESTORE(); } /* * ND6 timer routine to expire default route list and prefix list @@ -851,13 +856,13 @@ nd6_lookup(struct in6_addr *addr6, int f if (flags & ND6_EXCLUSIVE) llflags |= LLE_EXCLUSIVE; ln = lla_lookup(LLTABLE6(ifp), llflags, (struct sockaddr *)&sin6); if ((ln != NULL) && (flags & LLE_CREATE)) { ln->ln_state = ND6_LLINFO_NOSTATE; - callout_init(&ln->ln_timer_ch, 0); + callout_init_rw(&ln->ln_timer_ch, &ln->lle_lock, CALLOUT_RETURNUNLOCKED); } return (ln); } /* @@ -997,19 +1002,20 @@ static struct llentry * nd6_free(struct llentry *ln, int gc) { struct llentry *next; struct nd_defrouter *dr; struct ifnet *ifp=NULL; + LLE_WLOCK_ASSERT(ln); /* * we used to have pfctlinput(PRC_HOSTDEAD) here. * even though it is not harmful, it was not really necessary. */ /* cancel timer */ - nd6_llinfo_settimer(ln, -1); + nd6_llinfo_settimer_locked(ln, -1); if (!V_ip6_forwarding) { int s; s = splnet(); dr = defrouter_lookup(&L3_ADDR_SIN6(ln)->sin6_addr, ln->lle_tbl->llt_ifp); @@ -1025,18 +1031,17 @@ nd6_free(struct llentry *ln, int gc) * thing, especially when we're using router preference * values. * XXX: the check for ln_state would be redundant, * but we intentionally keep it just in case. */ if (dr->expire > time_second) - nd6_llinfo_settimer(ln, + nd6_llinfo_settimer_locked(ln, (dr->expire - time_second) * hz); else - nd6_llinfo_settimer(ln, (long)V_nd6_gctimer * hz); + nd6_llinfo_settimer_locked(ln, (long)V_nd6_gctimer * hz); splx(s); - LLE_WLOCK(ln); LLE_REMREF(ln); LLE_WUNLOCK(ln); return (LIST_NEXT(ln, lle_next)); } if (ln->ln_router || dr) { @@ -1086,12 +1091,13 @@ nd6_free(struct llentry *ln, int gc) * might have freed other entries (particularly the old next entry) as * a side effect (XXX). */ next = LIST_NEXT(ln, lle_next); ifp = ln->lle_tbl->llt_ifp; + LLE_WUNLOCK(ln); IF_AFDATA_LOCK(ifp); LLE_WLOCK(ln); LLE_REMREF(ln); llentry_free(ln); IF_AFDATA_UNLOCK(ifp); @@ -1829,33 +1835,33 @@ nd6_output_lle(struct ifnet *ifp, struct i--; } } else { ln->la_hold = m; } /* + * If there has been no NS for the neighbor after entering the + * INCOMPLETE state, send the first solicitation. + */ + if (!ND6_LLINFO_PERMANENT(ln) && ln->la_asked == 0) { + ln->la_asked++; + + nd6_llinfo_settimer_locked(ln, + (long)ND_IFINFO(ifp)->retrans * hz / 1000); + nd6_ns_output(ifp, NULL, &dst->sin6_addr, ln, 0); + } + /* * We did the lookup (no lle arg) so we * need to do the unlock here */ if (lle == NULL) { if (flags & LLE_EXCLUSIVE) LLE_WUNLOCK(ln); else LLE_RUNLOCK(ln); } - /* - * If there has been no NS for the neighbor after entering the - * INCOMPLETE state, send the first solicitation. - */ - if (!ND6_LLINFO_PERMANENT(ln) && ln->la_asked == 0) { - ln->la_asked++; - - nd6_llinfo_settimer(ln, - (long)ND_IFINFO(ifp)->retrans * hz / 1000); - nd6_ns_output(ifp, NULL, &dst->sin6_addr, ln, 0); - } return (0); sendpkt: /* discard the packet if IPv6 operation is disabled on the interface */ if ((ND_IFINFO(ifp)->flags & ND6_IFF_IFDISABLED)) { error = ENETDOWN; /* better error? */ --- sys/netinet6/nd6_nbr.c 2010-07-19 17:46:38.000000000 +0400 +++ sys/netinet6/nd6_nbr.c 2010-07-22 17:39:39.000000000 +0400 @@ -421,12 +421,17 @@ nd6_ns_output(struct ifnet *ifp, const s } } if (m == NULL) return; m->m_pkthdr.rcvif = NULL; + if (ln != NULL) { + LLE_WLOCK_ASSERT(ln); + LLE_WUNLOCK(ln); + } + if (daddr6 == NULL || IN6_IS_ADDR_MULTICAST(daddr6)) { m->m_flags |= M_MCAST; im6o.im6o_multicast_ifp = ifp; im6o.im6o_multicast_hlim = 255; im6o.im6o_multicast_loop = 0; } @@ -473,23 +478,27 @@ nd6_ns_output(struct ifnet *ifp, const s * - saddr6 belongs to the outgoing interface. * Otherwise, we perform the source address selection as usual. */ struct ip6_hdr *hip6; /* hold ip6 */ struct in6_addr *hsrc = NULL; - if ((ln != NULL) && ln->la_hold) { - /* - * assuming every packet in la_hold has the same IP - * header - */ - hip6 = mtod(ln->la_hold, struct ip6_hdr *); - /* XXX pullup? */ - if (sizeof(*hip6) < ln->la_hold->m_len) - hsrc = &hip6->ip6_src; - else - hsrc = NULL; + if (ln != NULL) { + LLE_RLOCK(ln); + if (ln->la_hold) { + /* + * assuming every packet in la_hold has the same IP + * header + */ + hip6 = mtod(ln->la_hold, struct ip6_hdr *); + /* XXX pullup? */ + if (sizeof(*hip6) < ln->la_hold->m_len) + hsrc = &hip6->ip6_src; + else + hsrc = NULL; + } + LLE_RUNLOCK(ln); } if (hsrc && (ifa = (struct ifaddr *)in6ifa_ifpwithaddr(ifp, hsrc)) != NULL) { src = hsrc; ifa_free(ifa); } else { @@ -570,19 +579,23 @@ nd6_ns_output(struct ifnet *ifp, const s icmp6_ifstat_inc(ifp, ifs6_out_neighborsolicit); ICMP6STAT_INC(icp6s_outhist[ND_NEIGHBOR_SOLICIT]); if (ro.ro_rt) { /* we don't cache this route. */ RTFREE(ro.ro_rt); } + if (ln) + LLE_WLOCK(ln); return; bad: if (ro.ro_rt) { RTFREE(ro.ro_rt); } m_freem(m); + if (ln) + LLE_WLOCK(ln); return; } /* * Neighbor advertisement input handling. * >Release-Note: >Audit-Trail: >Unformatted: