From owner-freebsd-current@FreeBSD.ORG Wed Mar 28 14:30:08 2007 Return-Path: X-Original-To: freebsd-current@freebsd.org Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4F9F016A408 for ; Wed, 28 Mar 2007 14:30:08 +0000 (UTC) (envelope-from andre@freebsd.org) Received: from c00l3r.networx.ch (c00l3r.networx.ch [62.48.2.2]) by mx1.freebsd.org (Postfix) with ESMTP id C15E013C4C9 for ; Wed, 28 Mar 2007 14:30:07 +0000 (UTC) (envelope-from andre@freebsd.org) Received: (qmail 18630 invoked from network); 28 Mar 2007 13:57:50 -0000 Received: from dotat.atdotat.at (HELO [62.48.0.47]) ([62.48.0.47]) (envelope-sender ) by c00l3r.networx.ch (qmail-ldap-1.03) with SMTP for ; 28 Mar 2007 13:57:50 -0000 Message-ID: <460A7BEE.9010508@freebsd.org> Date: Wed, 28 Mar 2007 16:30:06 +0200 From: Andre Oppermann User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050217 MIME-Version: 1.0 To: "Bruce M. Simpson" References: <46094415.3050704@FreeBSD.org> In-Reply-To: <46094415.3050704@FreeBSD.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: FreeBSD Current Subject: Re: [Fwd: Panic: if_freemulti: protospec not NULL] X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Mar 2007 14:30:08 -0000 Bruce M. Simpson wrote: > Patch for this condition is attached. This patch fixes the panic for me. -- Andre > This particular bug is irritating: the IPv4 stack joins 224.0.0.1 once > for every IPv4 unicast address configured in the stack. This is > incorrect behaviour. The implementation of refcounting has exposed this > bug. The fix is not particularly elegant. > > It is most likely a left-over from the FreeBSD 2.x/3.x era when IPv4 > 'aliases' were first introduced. At that point in time the IGMP code in > FreeBSD would allow groups to be joined on a per-IPv4-address basis, > which is inconsistent with the IGMPv2/v3 specified behaviour (and indeed > that addressed in future multicast RFCs). > > It seems that there are other situations where the stack is not > adequately equipped to deal with interfaces going away unexpectedly. We > can't afford to be complacent about multicast code on the basis of 'it's > not the critical path', because it is an integral component of IPv6, and > many ideas which people are trying to implement in IPv4 also require > that the multicast code is fit for purpose. > > We would do well to have more people available to help on reviewing and > possibly rewriting parts of the network stack from a perspective of > correctness, not just performance. If this interests you please consider > signing up to the Wiki and updating the page at > http://wiki.freebsd.org/NetworkRFCCompliance. > > Regards, > BMS > > > ------------------------------------------------------------------------ > > ==== //depot/user/bms/netdev/sys/netinet/in.c#11 - /home/bms/p4/netdev/sys/netinet/in.c ==== > --- /tmp/tmp.1127.0 Tue Mar 27 16:43:56 2007 > +++ /home/bms/p4/netdev/sys/netinet/in.c Tue Mar 27 16:40:04 2007 > @@ -212,6 +212,11 @@ > /* > * Generic internet control operations (ioctl's). > * Ifp is 0 if not an interface-specific ioctl. > + * > + * XXX: This code has some nice big juicy bugs related to interface > + * attach and detach. Specifically, the 224.0.0.1 group is now only > + * joined on the first IPv4 address configured on an interface, and > + * only left when all IPv4 state is torn down for an interface. > */ > /* ARGSUSED */ > int > @@ -230,7 +235,9 @@ > struct in_aliasreq *ifra = (struct in_aliasreq *)data; > struct sockaddr_in oldaddr; > int error, hostIsNew, iaIsNew, maskIsNew, s; > + int iaIsFirst; > > + iaIsFirst = 0; > iaIsNew = 0; > > switch (cmd) { > @@ -282,6 +289,8 @@ > break; > } > } > + if (ia == NULL) > + iaIsFirst = 1; > } > > switch (cmd) { > @@ -423,8 +432,20 @@ > (struct sockaddr_in *) &ifr->ifr_addr, 1); > if (error != 0 && iaIsNew) > break; > - if (error == 0) > + if (error == 0) { > + /* > + * Only join 224.0.0.1 if this is the very first > + * IPv4 unicast address which has been configured > + * on this ifnet. > + */ > + if (iaIsFirst && (ifp->if_flags & IFF_MULTICAST) != 0) { > + struct in_addr addr; > + > + addr.s_addr = htonl(INADDR_ALLHOSTS_GROUP); > + in_addmulti(&addr, ifp); > + } > EVENTHANDLER_INVOKE(ifaddr_event, ifp); > + } > return (0); > > case SIOCSIFNETMASK: > @@ -467,8 +488,20 @@ > if ((ifp->if_flags & IFF_BROADCAST) && > (ifra->ifra_broadaddr.sin_family == AF_INET)) > ia->ia_broadaddr = ifra->ifra_broadaddr; > - if (error == 0) > + if (error == 0) { > + /* > + * Only join 224.0.0.1 if this is the very first > + * IPv4 unicast address which has been configured > + * on this ifnet. > + */ > + if (iaIsFirst && (ifp->if_flags & IFF_MULTICAST) != 0) { > + struct in_addr addr; > + > + addr.s_addr = htonl(INADDR_ALLHOSTS_GROUP); > + in_addmulti(&addr, ifp); > + } > EVENTHANDLER_INVOKE(ifaddr_event, ifp); > + } > return (error); > > case SIOCDIFADDR: > @@ -503,8 +536,33 @@ > s = splnet(); > TAILQ_REMOVE(&ifp->if_addrhead, &ia->ia_ifa, ifa_link); > TAILQ_REMOVE(&in_ifaddrhead, ia, ia_link); > - if (ia->ia_addr.sin_family == AF_INET) > + if (ia->ia_addr.sin_family == AF_INET) { > + struct in_ifaddr *pia; > + > LIST_REMOVE(ia, ia_hash); > + /* > + * Only purge the 224.0.0.1 membership if the last IPv4 > + * unicast address configured on this ifnet is removed. > + * > + * XXX: This currently involves trawling through the > + * in_ifaddrhead list looking for a match, which is > + * inefficient, though this is only done when an interface > + * address goes away. This could be done much more elegantly. > + */ > + pia = NULL; > + IFP_TO_IA(ifp, pia); > + if (pia == NULL) { > + struct in_multi *inm; > + struct in_addr addr; > + > + addr.s_addr = htonl(INADDR_ALLHOSTS_GROUP); > + IN_MULTI_LOCK(); > + IN_LOOKUP_MULTI(addr, ifp, inm); > + if (inm != NULL) > + in_delmulti(inm); > + IN_MULTI_UNLOCK(); > + } > + } > IFAFREE(&ia->ia_ifa); > splx(s); > > @@ -793,16 +851,6 @@ > if ((error = in_addprefix(ia, flags)) != 0) > return (error); > > - /* > - * If the interface supports multicast, join the "all hosts" > - * multicast group on that interface. > - */ > - if (ifp->if_flags & IFF_MULTICAST) { > - struct in_addr addr; > - > - addr.s_addr = htonl(INADDR_ALLHOSTS_GROUP); > - in_addmulti(&addr, ifp); > - } > return (error); > } > > @@ -1114,6 +1162,9 @@ > igmp_leavegroup(inm); > > ifma = inm->inm_ifma; > +#ifdef DIAGNOSTIC > + printf("%s: purging ifma %p\n", __func__, ifma); > +#endif > KASSERT(ifma->ifma_protospec == inm, > ("%s: ifma_protospec != inm", __func__)); > ifma->ifma_protospec = NULL; > @@ -1135,6 +1186,9 @@ > struct in_multi *inm; > struct in_multi *oinm; > > +#ifdef DIAGNOSTIC > + printf("%s: purging ifp %p\n", __func__, ifp); > +#endif > IFF_LOCKGIANT(ifp); > IN_MULTI_LOCK(); > LIST_FOREACH_SAFE(inm, &in_multihead, inm_link, oinm) {