>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:
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "[email protected]"
