Re: [Fwd: Panic: if_freemulti: protospec not NULL]

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
> _at__at_ -212,6 +212,11 _at__at_
>  /*
>   * 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
> _at__at_ -230,7 +235,9 _at__at_
>  	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) {
> _at__at_ -282,6 +289,8 _at__at_
>  					break;
>  				}
>  			}
> +		if (ia == NULL)
> +			iaIsFirst = 1;
>  	}
>  
>  	switch (cmd) {
> _at__at_ -423,8 +432,20 _at__at_
>  		    (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:
> _at__at_ -467,8 +488,20 _at__at_
>  		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:
> _at__at_ -503,8 +536,33 _at__at_
>  	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);
>  
> _at__at_ -793,16 +851,6 _at__at_
>  	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);
>  }
>  
> _at__at_ -1114,6 +1162,9 _at__at_
>  		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;
> _at__at_ -1135,6 +1186,9 _at__at_
>  	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) {