From owner-freebsd-current@FreeBSD.ORG Sun May 31 20:05:08 2009 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 64ABC106566C for ; Sun, 31 May 2009 20:05:08 +0000 (UTC) (envelope-from bms@incunabulum.net) Received: from out4.smtp.messagingengine.com (out4.smtp.messagingengine.com [66.111.4.28]) by mx1.freebsd.org (Postfix) with ESMTP id 2510B8FC1D for ; Sun, 31 May 2009 20:05:08 +0000 (UTC) (envelope-from bms@incunabulum.net) Received: from compute2.internal (compute2.internal [10.202.2.42]) by out1.messagingengine.com (Postfix) with ESMTP id A111B348F3D; Sun, 31 May 2009 16:05:07 -0400 (EDT) Received: from heartbeat2.messagingengine.com ([10.202.2.161]) by compute2.internal (MEProxy); Sun, 31 May 2009 16:05:07 -0400 X-Sasl-enc: Nm8l2sVOtyJFy3Vzx3P5VpNA6rjRXo5w6BIgrKUI2pt/ 1243800306 Received: from anglepoise.lon.incunabulum.net (82-35-112-254.cable.ubr07.dals.blueyonder.co.uk [82.35.112.254]) by mail.messagingengine.com (Postfix) with ESMTPSA id BAB8239400; Sun, 31 May 2009 16:05:06 -0400 (EDT) Message-ID: <4A22E2F1.6070404@incunabulum.net> Date: Sun, 31 May 2009 21:05:05 +0100 From: Bruce Simpson User-Agent: Thunderbird 2.0.0.21 (X11/20090406) MIME-Version: 1.0 To: deeptech71@gmail.com References: <4A1DD57A.7010704@gmail.com> <4A1E9831.4010606@incunabulum.net> <4A1EC8FB.6090206@gmail.com> <4A1FBFF5.6090103@incunabulum.net> <4A20791D.5070209@gmail.com> In-Reply-To: <4A20791D.5070209@gmail.com> Content-Type: multipart/mixed; boundary="------------050205070009090208030305" Cc: freebsd-current@freebsd.org Subject: Re: panic: igmp_v3_dispatch_general_query: called when version 2 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: Sun, 31 May 2009 20:05:08 -0000 This is a multi-part message in MIME format. --------------050205070009090208030305 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit deeptech71@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 --------------050205070009090208030305 Content-Type: text/plain; name="igmpv3.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="igmpv3.diff" ==== //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 @@ -98,7 +98,8 @@ 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 *, @@ -700,7 +701,8 @@ * 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; @@ -708,20 +710,18 @@ 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(); @@ -734,6 +734,9 @@ 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); @@ -791,12 +794,29 @@ 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(); @@ -809,16 +829,37 @@ 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); @@ -827,32 +868,6 @@ 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: @@ -931,10 +946,13 @@ 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 */ @@ -945,7 +963,7 @@ /* * 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). */ @@ -968,6 +986,33 @@ 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(); @@ -980,8 +1025,19 @@ 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; @@ -989,41 +1045,23 @@ 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 @@ -1033,7 +1071,6 @@ 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.", @@ -1041,8 +1078,6 @@ 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); @@ -1465,7 +1500,7 @@ 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; } @@ -1907,6 +1942,7 @@ static void igmp_set_version(struct igmp_ifinfo *igi, const int version) { + int old_version_timer; IGMP_LOCK_ASSERT(); @@ -1914,7 +1950,6 @@ 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. @@ -1947,6 +1982,11 @@ /* * 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) @@ -1963,21 +2003,21 @@ 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) { @@ -1987,12 +2027,19 @@ 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 @@ -2007,15 +2054,16 @@ 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); } --------------050205070009090208030305--