Date: Fri, 13 Jan 2012 19:50:52 +0000 (UTC) From: John Baldwin <jhb@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-9@freebsd.org Subject: svn commit: r230076 - in stable/9/sys: netinet netinet6 Message-ID: <201201131950.q0DJoqKE077548@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: jhb Date: Fri Jan 13 19:50:52 2012 New Revision: 230076 URL: http://svn.freebsd.org/changeset/base/230076 Log: MFC 229390,229420,229479: Fix some races in the multicast code by removing places where we would drop the IF_ADDR_LOCK while walking an interface's multicast address list: - Use TAILQ_FOREACH() instead of TAILQ_FOREACH_SAFE() for some loops that do not modify the queues they iterate over. - When cancelling multicast timers on an interface, don't release the reference on a group in the leaving state while iterating over the loop. Instead, use the same approach used in igmp_ifdetach() and mld_ifdetach() of placing the groups to free on a pending release list and then releasing the references after dropping the IF_ADDR_LOCK. - Use the mli_relinmhead list normally used to defer calls to in6m_release_locked() to defer calls to mld_v1_transmit_report() until after the IF_ADDR_LOCK is dropped. Modified: stable/9/sys/netinet/igmp.c stable/9/sys/netinet6/in6.c stable/9/sys/netinet6/mld6.c Directory Properties: stable/9/sys/ (props changed) stable/9/sys/amd64/include/xen/ (props changed) stable/9/sys/boot/ (props changed) stable/9/sys/boot/i386/efi/ (props changed) stable/9/sys/boot/ia64/efi/ (props changed) stable/9/sys/boot/ia64/ski/ (props changed) stable/9/sys/boot/powerpc/boot1.chrp/ (props changed) stable/9/sys/boot/powerpc/ofw/ (props changed) stable/9/sys/cddl/contrib/opensolaris/ (props changed) stable/9/sys/conf/ (props changed) stable/9/sys/contrib/dev/acpica/ (props changed) stable/9/sys/contrib/octeon-sdk/ (props changed) stable/9/sys/contrib/pf/ (props changed) stable/9/sys/contrib/x86emu/ (props changed) Modified: stable/9/sys/netinet/igmp.c ============================================================================== --- stable/9/sys/netinet/igmp.c Fri Jan 13 19:20:33 2012 (r230075) +++ stable/9/sys/netinet/igmp.c Fri Jan 13 19:50:52 2012 (r230076) @@ -1641,7 +1641,7 @@ igmp_fasttimo_vnet(void) struct ifqueue qrq; /* Query response packets */ struct ifnet *ifp; struct igmp_ifinfo *igi; - struct ifmultiaddr *ifma, *tifma; + struct ifmultiaddr *ifma; struct in_multi *inm; int loop, uri_fasthz; @@ -1708,8 +1708,7 @@ igmp_fasttimo_vnet(void) } IF_ADDR_LOCK(ifp); - TAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, - tifma) { + TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { if (ifma->ifma_addr->sa_family != AF_INET || ifma->ifma_protospec == NULL) continue; @@ -2003,7 +2002,7 @@ igmp_v3_cancel_link_timers(struct igmp_i { struct ifmultiaddr *ifma; struct ifnet *ifp; - struct in_multi *inm; + struct in_multi *inm, *tinm; CTR3(KTR_IGMPV3, "%s: cancel v3 timers on ifp %p(%s)", __func__, igi->igi_ifp, igi->igi_ifp->if_xname); @@ -2049,14 +2048,8 @@ igmp_v3_cancel_link_timers(struct igmp_i * 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 - * a recursive mutex. */ - IF_ADDR_UNLOCK(ifp); - inm_release_locked(inm); - IF_ADDR_LOCK(ifp); + SLIST_INSERT_HEAD(&igi->igi_relinmhead, inm, inm_nrele); /* FALLTHROUGH */ case IGMP_G_QUERY_PENDING_MEMBER: case IGMP_SG_QUERY_PENDING_MEMBER: @@ -2075,6 +2068,10 @@ igmp_v3_cancel_link_timers(struct igmp_i _IF_DRAIN(&inm->inm_scq); } IF_ADDR_UNLOCK(ifp); + SLIST_FOREACH_SAFE(inm, &igi->igi_relinmhead, inm_nrele, tinm) { + SLIST_REMOVE_HEAD(&igi->igi_relinmhead, inm_nrele); + inm_release_locked(inm); + } } /* @@ -3320,7 +3317,7 @@ igmp_v3_merge_state_changes(struct in_mu static void igmp_v3_dispatch_general_query(struct igmp_ifinfo *igi) { - struct ifmultiaddr *ifma, *tifma; + struct ifmultiaddr *ifma; struct ifnet *ifp; struct in_multi *inm; int retval, loop; @@ -3334,7 +3331,7 @@ igmp_v3_dispatch_general_query(struct ig ifp = igi->igi_ifp; IF_ADDR_LOCK(ifp); - TAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, tifma) { + TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { if (ifma->ifma_addr->sa_family != AF_INET || ifma->ifma_protospec == NULL) continue; Modified: stable/9/sys/netinet6/in6.c ============================================================================== --- stable/9/sys/netinet6/in6.c Fri Jan 13 19:20:33 2012 (r230075) +++ stable/9/sys/netinet6/in6.c Fri Jan 13 19:50:52 2012 (r230076) @@ -1299,7 +1299,7 @@ in6_purgeaddr(struct ifaddr *ifa) struct sockaddr_in6 mltaddr, mltmask; int plen, error; struct rtentry *rt; - struct ifaddr *ifa0, *nifa; + struct ifaddr *ifa0; /* * find another IPv6 address as the gateway for the @@ -1307,7 +1307,7 @@ in6_purgeaddr(struct ifaddr *ifa) * address routes */ IF_ADDR_LOCK(ifp); - TAILQ_FOREACH_SAFE(ifa0, &ifp->if_addrhead, ifa_link, nifa) { + TAILQ_FOREACH(ifa0, &ifp->if_addrhead, ifa_link) { if ((ifa0->ifa_addr->sa_family != AF_INET6) || memcmp(&satosin6(ifa0->ifa_addr)->sin6_addr, &ia->ia_addr.sin6_addr, Modified: stable/9/sys/netinet6/mld6.c ============================================================================== --- stable/9/sys/netinet6/mld6.c Fri Jan 13 19:20:33 2012 (r230075) +++ stable/9/sys/netinet6/mld6.c Fri Jan 13 19:50:52 2012 (r230076) @@ -121,7 +121,8 @@ static int mld_v1_input_query(struct ifn /*const*/ struct mld_hdr *); static int mld_v1_input_report(struct ifnet *, const struct ip6_hdr *, /*const*/ struct mld_hdr *); -static void mld_v1_process_group_timer(struct in6_multi *, const int); +static void mld_v1_process_group_timer(struct mld_ifinfo *, + struct in6_multi *); static void mld_v1_process_querier_timers(struct mld_ifinfo *); static int mld_v1_transmit_report(struct in6_multi *, const int); static void mld_v1_update_group(struct in6_multi *, const int); @@ -1335,8 +1336,8 @@ mld_fasttimo_vnet(void) struct ifqueue qrq; /* Query response packets */ struct ifnet *ifp; struct mld_ifinfo *mli; - struct ifmultiaddr *ifma, *tifma; - struct in6_multi *inm; + struct ifmultiaddr *ifma; + struct in6_multi *inm, *tinm; int uri_fasthz; uri_fasthz = 0; @@ -1400,24 +1401,14 @@ mld_fasttimo_vnet(void) } IF_ADDR_LOCK(ifp); - TAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, - tifma) { + TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { if (ifma->ifma_addr->sa_family != AF_INET6 || ifma->ifma_protospec == NULL) continue; inm = (struct in6_multi *)ifma->ifma_protospec; switch (mli->mli_version) { case MLD_VERSION_1: - /* - * XXX Drop IF_ADDR lock temporarily to - * avoid recursion caused by a potential - * call by in6ifa_ifpforlinklocal(). - * rwlock candidate? - */ - IF_ADDR_UNLOCK(ifp); - mld_v1_process_group_timer(inm, - mli->mli_version); - IF_ADDR_LOCK(ifp); + mld_v1_process_group_timer(mli, inm); break; case MLD_VERSION_2: mld_v2_process_group_timers(mli, &qrq, @@ -1427,9 +1418,25 @@ mld_fasttimo_vnet(void) } IF_ADDR_UNLOCK(ifp); - if (mli->mli_version == MLD_VERSION_2) { - struct in6_multi *tinm; - + switch (mli->mli_version) { + case MLD_VERSION_1: + /* + * Transmit reports for this lifecycle. This + * is done while not holding IF_ADDR_LOCK + * since this can call + * in6ifa_ifpforlinklocal() which locks + * IF_ADDR_LOCK internally as well as + * ip6_output() to transmit a packet. + */ + SLIST_FOREACH_SAFE(inm, &mli->mli_relinmhead, + in6m_nrele, tinm) { + SLIST_REMOVE_HEAD(&mli->mli_relinmhead, + in6m_nrele); + (void)mld_v1_transmit_report(inm, + MLD_LISTENER_REPORT); + } + break; + case MLD_VERSION_2: mld_dispatch_queue(&qrq, 0); mld_dispatch_queue(&scq, 0); @@ -1443,6 +1450,7 @@ mld_fasttimo_vnet(void) in6m_nrele); in6m_release_locked(inm); } + break; } } @@ -1456,7 +1464,7 @@ out_locked: * Will update the global pending timer flags. */ static void -mld_v1_process_group_timer(struct in6_multi *inm, const int version) +mld_v1_process_group_timer(struct mld_ifinfo *mli, struct in6_multi *inm) { int report_timer_expired; @@ -1483,8 +1491,8 @@ mld_v1_process_group_timer(struct in6_mu case MLD_REPORTING_MEMBER: if (report_timer_expired) { inm->in6m_state = MLD_IDLE_MEMBER; - (void)mld_v1_transmit_report(inm, - MLD_LISTENER_REPORT); + SLIST_INSERT_HEAD(&mli->mli_relinmhead, inm, + in6m_nrele); } break; case MLD_G_QUERY_PENDING_MEMBER: @@ -1655,7 +1663,7 @@ mld_v2_cancel_link_timers(struct mld_ifi { struct ifmultiaddr *ifma; struct ifnet *ifp; - struct in6_multi *inm; + struct in6_multi *inm, *tinm; CTR3(KTR_MLD, "%s: cancel v2 timers on ifp %p(%s)", __func__, mli->mli_ifp, mli->mli_ifp->if_xname); @@ -1694,14 +1702,9 @@ mld_v2_cancel_link_timers(struct mld_ifi * If we are leaving the group and switching * version, we need to release the final * reference held for issuing the INCLUDE {}. - * - * SMPNG: Must drop and re-acquire IF_ADDR_LOCK - * around in6m_release_locked(), as it is not - * a recursive mutex. */ - IF_ADDR_UNLOCK(ifp); - in6m_release_locked(inm); - IF_ADDR_LOCK(ifp); + SLIST_INSERT_HEAD(&mli->mli_relinmhead, inm, + in6m_nrele); /* FALLTHROUGH */ case MLD_G_QUERY_PENDING_MEMBER: case MLD_SG_QUERY_PENDING_MEMBER: @@ -1719,6 +1722,10 @@ mld_v2_cancel_link_timers(struct mld_ifi } } IF_ADDR_UNLOCK(ifp); + SLIST_FOREACH_SAFE(inm, &mli->mli_relinmhead, in6m_nrele, tinm) { + SLIST_REMOVE_HEAD(&mli->mli_relinmhead, in6m_nrele); + in6m_release_locked(inm); + } } /* @@ -2975,7 +2982,7 @@ mld_v2_merge_state_changes(struct in6_mu static void mld_v2_dispatch_general_query(struct mld_ifinfo *mli) { - struct ifmultiaddr *ifma, *tifma; + struct ifmultiaddr *ifma; struct ifnet *ifp; struct in6_multi *inm; int retval; @@ -2989,7 +2996,7 @@ mld_v2_dispatch_general_query(struct mld ifp = mli->mli_ifp; IF_ADDR_LOCK(ifp); - TAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, tifma) { + TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { if (ifma->ifma_addr->sa_family != AF_INET6 || ifma->ifma_protospec == NULL) continue;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201131950.q0DJoqKE077548>