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