Re: panic: igmp_v3_dispatch_general_query: called when version 2

From: Bruce Simpson <bms_at_incunabulum.net>
Date: Sun, 31 May 2009 21:05:05 +0100
deeptech71_at_gmail.com wrote:
> Bruce Simpson wrote:
>> I may not get free time to fix this right away, are you able to test a
>> patch when I can make one available?
>
> Sure.

Thanks. Can you please test this patch and let me know if it works for you?

You ran into a race where the v3 link timer could be set but not 
cancelled when the version changes; that function is trying to be too 
smart about the work it has to do.

There are some more things in there than 'just' the change you need, I 
want to bring in some of the MLDv2 changes which tighten query 
processing. I've checked it into Perforce for now and can merge it in 
the week.

thanks,
BMS

==== //depot/user/bms/netdev/sys/netinet/igmp.c#232 - /home/bms/p4/netdev/sys/netinet/igmp.c ====
--- /tmp/tmp.94699.26	2009-05-31 20:58:26.000000000 +0100
+++ /home/bms/p4/netdev/sys/netinet/igmp.c	2009-05-31 20:52:56.000000000 +0100
_at__at_ -98,7 +98,8 _at__at_
 static int	igmp_handle_state_change(struct in_multi *,
 		    struct igmp_ifinfo *);
 static int	igmp_initial_join(struct in_multi *, struct igmp_ifinfo *);
-static int	igmp_input_v1_query(struct ifnet *, const struct ip *);
+static int	igmp_input_v1_query(struct ifnet *, const struct ip *,
+		    const struct igmp *);
 static int	igmp_input_v2_query(struct ifnet *, const struct ip *,
 		    const struct igmp *);
 static int	igmp_input_v3_query(struct ifnet *, const struct ip *,
_at__at_ -700,7 +701,8 _at__at_
  * VIMAGE: The curvnet pointer is derived from the input ifp.
  */
 static int
-igmp_input_v1_query(struct ifnet *ifp, const struct ip *ip)
+igmp_input_v1_query(struct ifnet *ifp, const struct ip *ip,
+    const struct igmp *igmp)
 {
 	INIT_VNET_INET(ifp->if_vnet);
 	struct ifmultiaddr	*ifma;
_at__at_ -708,20 +710,18 _at__at_
 	struct in_multi		*inm;
 
 	/*
-	 * IGMPv1 General Queries SHOULD always addressed to 224.0.0.1.
+	 * IGMPv1 Host Mmembership Queries SHOULD always be addressed to
+	 * 224.0.0.1. They are always treated as General Queries.
 	 * igmp_group is always ignored. Do not drop it as a userland
 	 * daemon may wish to see it.
+	 * XXX SMPng: unlocked increments in igmpstat assumed atomic.
 	 */
-	if (!in_allhosts(ip->ip_dst)) {
+	if (!in_allhosts(ip->ip_dst) || !in_nullhost(igmp->igmp_group)) {
 		IGMPSTAT_INC(igps_rcv_badqueries);
 		return (0);
 	}
-
 	IGMPSTAT_INC(igps_rcv_gen_queries);
 
-	/*
-	 * Switch to IGMPv1 host compatibility mode.
-	 */
 	IN_MULTI_LOCK();
 	IGMP_LOCK();
 
_at__at_ -734,6 +734,9 _at__at_
 		goto out_locked;
 	}
 
+	/*
+	 * Switch to IGMPv1 host compatibility mode.
+	 */
 	igmp_set_version(igi, IGMP_VERSION_1);
 
 	CTR2(KTR_IGMPV3, "process v1 query on ifp %p(%s)", ifp, ifp->if_xname);
_at__at_ -791,12 +794,29 _at__at_
 	struct ifmultiaddr	*ifma;
 	struct igmp_ifinfo	*igi;
 	struct in_multi		*inm;
+	int			 is_general_query;
 	uint16_t		 timer;
 
+	is_general_query = 0;
+
 	/*
-	 * Perform lazy allocation of IGMP link info if required,
-	 * and switch to IGMPv2 host compatibility mode.
+	 * Validate address fields upfront.
+	 * XXX SMPng: unlocked increments in igmpstat assumed atomic.
 	 */
+	if (in_nullhost(igmp->igmp_group)) {
+		/*
+		 * IGMPv2 General Query.
+		 * If this was not sent to the all-hosts group, ignore it.
+		 */
+		if (!in_allhosts(ip->ip_dst))
+			return (0);
+		IGMPSTAT_INC(igps_rcv_gen_queries);
+		is_general_query = 1;
+	} else {
+		/* IGMPv2 Group-Specific Query. */
+		IGMPSTAT_INC(igps_rcv_group_queries);
+	}
+
 	IN_MULTI_LOCK();
 	IGMP_LOCK();
 
_at__at_ -809,16 +829,37 _at__at_
 		goto out_locked;
 	}
 
+	/*
+	 * Ignore v2 query if in v1 Compatibility Mode.
+	 */
+	if (igi->igi_version == IGMP_VERSION_1)
+		goto out_locked;
+
 	igmp_set_version(igi, IGMP_VERSION_2);
 
 	timer = igmp->igmp_code * PR_FASTHZ / IGMP_TIMER_SCALE;
 	if (timer == 0)
 		timer = 1;
 
-	if (!in_nullhost(igmp->igmp_group)) {
+	if (is_general_query) {
 		/*
-		 * IGMPv2 Group-Specific Query.
-		 * If this is a group-specific IGMPv2 query, we need only
+		 * For each reporting group joined on this
+		 * interface, kick the report timer.
+		 */
+		CTR2(KTR_IGMPV3, "process v2 general query on ifp %p(%s)",
+		    ifp, ifp->if_xname);
+		IF_ADDR_LOCK(ifp);
+		TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
+			if (ifma->ifma_addr->sa_family != AF_INET ||
+			    ifma->ifma_protospec == NULL)
+				continue;
+			inm = (struct in_multi *)ifma->ifma_protospec;
+			igmp_v2_update_group(inm, timer);
+		}
+		IF_ADDR_UNLOCK(ifp);
+	} else {
+		/*
+		 * Group-specific IGMPv2 query, we need only
 		 * look up the single group to process it.
 		 */
 		inm = inm_lookup(ifp, igmp->igmp_group);
_at__at_ -827,32 +868,6 _at__at_
 			    inet_ntoa(igmp->igmp_group), ifp, ifp->if_xname);
 			igmp_v2_update_group(inm, timer);
 		}
-		IGMPSTAT_INC(igps_rcv_group_queries);
-	} else {
-		/*
-		 * IGMPv2 General Query.
-		 * If this was not sent to the all-hosts group, ignore it.
-		 */
-		if (in_allhosts(ip->ip_dst)) {
-			/*
-			 * For each reporting group joined on this
-			 * interface, kick the report timer.
-			 */
-			CTR2(KTR_IGMPV3,
-			    "process v2 general query on ifp %p(%s)",
-			    ifp, ifp->if_xname);
-
-			IF_ADDR_LOCK(ifp);
-			TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
-				if (ifma->ifma_addr->sa_family != AF_INET ||
-				    ifma->ifma_protospec == NULL)
-					continue;
-				inm = (struct in_multi *)ifma->ifma_protospec;
-				igmp_v2_update_group(inm, timer);
-			}
-			IF_ADDR_UNLOCK(ifp);
-		}
-		IGMPSTAT_INC(igps_rcv_gen_queries);
 	}
 
 out_locked:
_at__at_ -931,10 +946,13 _at__at_
 	INIT_VNET_INET(ifp->if_vnet);
 	struct igmp_ifinfo	*igi;
 	struct in_multi		*inm;
+	int			 is_general_query;
 	uint32_t		 maxresp, nsrc, qqi;
 	uint16_t		 timer;
 	uint8_t			 qrv;
 
+	is_general_query = 0;
+
 	CTR2(KTR_IGMPV3, "process v3 query on ifp %p(%s)", ifp, ifp->if_xname);
 
 	maxresp = igmpv3->igmp_code;	/* in 1/10ths of a second */
_at__at_ -945,7 +963,7 _at__at_
 
 	/*
 	 * Robustness must never be less than 2 for on-wire IGMPv3.
-	 * FIXME: Check if ifp has IGIF_LOOPBACK set, as we make
+	 * FUTURE: Check if ifp has IGIF_LOOPBACK set, as we will make
 	 * an exception for interfaces whose IGMPv3 state changes
 	 * are redirected to loopback (e.g. MANET).
 	 */
_at__at_ -968,6 +986,33 _at__at_
 
 	nsrc = ntohs(igmpv3->igmp_numsrc);
 
+	/*
+	 * Validate address fields and versions upfront before
+	 * accepting v3 query.
+	 * XXX SMPng: Unlocked access to igmpstat counters here.
+	 */
+	if (in_nullhost(igmpv3->igmp_group)) {
+		/*
+		 * IGMPv3 General Query.
+		 *
+		 * General Queries SHOULD be directed to 224.0.0.1.
+		 * A general query with a source list has undefined
+		 * behaviour; discard it.
+		 */
+		IGMPSTAT_INC(igps_rcv_gen_queries);
+		if (!in_allhosts(ip->ip_dst) || nsrc > 0) {
+			IGMPSTAT_INC(igps_rcv_badqueries);
+			return (0);
+		}
+		is_general_query = 1;
+	} else {
+		/* Group or group-source specific query. */
+		if (nsrc == 0)
+			IGMPSTAT_INC(igps_rcv_group_queries);
+		else
+			IGMPSTAT_INC(igps_rcv_gsr_queries);
+	}
+
 	IN_MULTI_LOCK();
 	IGMP_LOCK();
 
_at__at_ -980,8 +1025,19 _at__at_
 		goto out_locked;
 	}
 
-	igmp_set_version(igi, IGMP_VERSION_3);
+	/*
+	 * Discard the v3 query if we're in Compatibility Mode.
+	 * The RFC is not obviously worded that hosts need to stay in
+	 * compatibility mode until the Old Version Querier Present
+	 * timer expires.
+	 */
+	if (igi->igi_version != IGMP_VERSION_3) {
+		CTR3(KTR_IGMPV3, "ignore v3 query in v%d mode on ifp %p(%s)",
+		    igi->igi_version, ifp, ifp->if_xname);
+		goto out_locked;
+	}
 
+	igmp_set_version(igi, IGMP_VERSION_3);
 	igi->igi_rv = qrv;
 	igi->igi_qi = qqi;
 	igi->igi_qri = maxresp;
_at__at_ -989,41 +1045,23 _at__at_
 	CTR4(KTR_IGMPV3, "%s: qrv %d qi %d qri %d", __func__, qrv, qqi,
 	    maxresp);
 
-	if (in_nullhost(igmpv3->igmp_group)) {
+	if (is_general_query) {
 		/*
-		 * IGMPv3 General Query.
 		 * Schedule a current-state report on this ifp for
 		 * all groups, possibly containing source lists.
-		 */
-		IGMPSTAT_INC(igps_rcv_gen_queries);
-
-		if (!in_allhosts(ip->ip_dst) || nsrc > 0) {
-			/*
-			 * General Queries SHOULD be directed to 224.0.0.1.
-			 * A general query with a source list has undefined
-			 * behaviour; discard it.
-			 */
-			IGMPSTAT_INC(igps_rcv_badqueries);
-			goto out_locked;
-		}
-
-		CTR2(KTR_IGMPV3, "process v3 general query on ifp %p(%s)",
-		    ifp, ifp->if_xname);
-
-		/*
 		 * If there is a pending General Query response
 		 * scheduled earlier than the selected delay, do
 		 * not schedule any other reports.
 		 * Otherwise, reset the interface timer.
 		 */
+		CTR2(KTR_IGMPV3, "process v3 general query on ifp %p(%s)",
+		    ifp, ifp->if_xname);
 		if (igi->igi_v3_timer == 0 || igi->igi_v3_timer >= timer) {
 			igi->igi_v3_timer = IGMP_RANDOM_DELAY(timer);
 			V_interface_timers_running = 1;
 		}
 	} else {
 		/*
-		 * IGMPv3 Group-specific or Group-and-source-specific Query.
-		 *
 		 * Group-source-specific queries are throttled on
 		 * a per-group basis to defeat denial-of-service attempts.
 		 * Queries for groups we are not a member of on this
_at__at_ -1033,7 +1071,6 _at__at_
 		if (inm == NULL)
 			goto out_locked;
 		if (nsrc > 0) {
-			IGMPSTAT_INC(igps_rcv_gsr_queries);
 			if (!ratecheck(&inm->inm_lastgsrtv,
 			    &V_igmp_gsrdelay)) {
 				CTR1(KTR_IGMPV3, "%s: GS query throttled.",
_at__at_ -1041,8 +1078,6 _at__at_
 				IGMPSTAT_INC(igps_drop_gsr_queries);
 				goto out_locked;
 			}
-		} else {
-			IGMPSTAT_INC(igps_rcv_group_queries);
 		}
 		CTR3(KTR_IGMPV3, "process v3 %s query on ifp %p(%s)",
 		     inet_ntoa(igmpv3->igmp_group), ifp, ifp->if_xname);
_at__at_ -1465,7 +1500,7 _at__at_
 			IGMPSTAT_INC(igps_rcv_v1v2_queries);
 			if (!V_igmp_v1enable)
 				break;
-			if (igmp_input_v1_query(ifp, ip) != 0) {
+			if (igmp_input_v1_query(ifp, ip, igmp) != 0) {
 				m_freem(m);
 				return;
 			}
_at__at_ -1907,6 +1942,7 _at__at_
 static void
 igmp_set_version(struct igmp_ifinfo *igi, const int version)
 {
+	int old_version_timer;
 
 	IGMP_LOCK_ASSERT();
 
_at__at_ -1914,7 +1950,6 _at__at_
 	    version, igi->igi_ifp, igi->igi_ifp->if_xname);
 
 	if (version == IGMP_VERSION_1 || version == IGMP_VERSION_2) {
-		int old_version_timer;
 		/*
 		 * Compute the "Older Version Querier Present" timer as per
 		 * Section 8.12.
_at__at_ -1947,6 +1982,11 _at__at_
 /*
  * Cancel pending IGMPv3 timers for the given link and all groups
  * joined on it; state-change, general-query, and group-query timers.
+ *
+ * Only ever called on a transition from v3 to Compatibility mode. Kill
+ * the timers stone dead (this may be expensive for large N groups), they
+ * will be restarted if Compatibility Mode deems that they must be due to
+ * query processing.
  */
 static void
 igmp_v3_cancel_link_timers(struct igmp_ifinfo *igi)
_at__at_ -1963,21 +2003,21 _at__at_
 	IGMP_LOCK_ASSERT();
 
 	/*
-	 * Fast-track this potentially expensive operation
-	 * by checking all the global 'timer pending' flags.
+	 * Stop the v3 General Query Response on this link stone dead.
+	 * If fasttimo is woken up due to V_interface_timers_running,
+	 * the flag will be cleared if there are no pending link timers.
 	 */
-	if (!V_interface_timers_running &&
-	    !V_state_change_timers_running &&
-	    !V_current_state_timers_running)
-		return;
-
 	igi->igi_v3_timer = 0;
 
+	/*
+	 * Now clear the current-state and state-change report timers
+	 * for all memberships scoped to this link.
+	 */
 	ifp = igi->igi_ifp;
-
 	IF_ADDR_LOCK(ifp);
 	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
-		if (ifma->ifma_addr->sa_family != AF_INET)
+		if (ifma->ifma_addr->sa_family != AF_INET ||
+		    ifma->ifma_protospec == NULL)
 			continue;
 		inm = (struct in_multi *)ifma->ifma_protospec;
 		switch (inm->inm_state) {
_at__at_ -1987,12 +2027,19 _at__at_
 		case IGMP_LAZY_MEMBER:
 		case IGMP_SLEEPING_MEMBER:
 		case IGMP_AWAKENING_MEMBER:
+			/*
+			 * These states are either not relevant in v3 mode,
+			 * or are unreported. Do nothing.
+			 */
 			break;
 		case IGMP_LEAVING_MEMBER:
 			/*
-			 * If we are leaving the group and switching
-			 * IGMP version, we need to release the final
-			 * reference held for issuing the INCLUDE {}.
+			 * If we are leaving the group and switching to
+			 * compatibility mode, we need to release the final
+			 * reference held for issuing the INCLUDE {}, and
+			 * transition to REPORTING to ensure the host leave
+			 * message is sent upstream to the old querier --
+			 * transition to NOT would lose the leave and race.
 			 *
 			 * SMPNG: Must drop and re-acquire IF_ADDR_LOCK
 			 * around inm_release_locked(), as it is not
_at__at_ -2007,15 +2054,16 _at__at_
 			inm_clear_recorded(inm);
 			/* FALLTHROUGH */
 		case IGMP_REPORTING_MEMBER:
-			inm->inm_sctimer = 0;
-			inm->inm_timer = 0;
 			inm->inm_state = IGMP_REPORTING_MEMBER;
-			/*
-			 * Free any pending IGMPv3 state-change records.
-			 */
-			_IF_DRAIN(&inm->inm_scq);
 			break;
 		}
+		/*
+		 * Always clear state-change and group report timers.
+		 * Free any pending IGMPv3 state-change records.
+		 */
+		inm->inm_sctimer = 0;
+		inm->inm_timer = 0;
+		_IF_DRAIN(&inm->inm_scq);
 	}
 	IF_ADDR_UNLOCK(ifp);
 }
Received on Sun May 31 2009 - 18:05:08 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:39:48 UTC