From c6f4ea8062f3cc09fe522edaacc3f632738bca71 Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Tue, 3 Jan 2012 20:34:52 +0000 Subject: [PATCH] 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 pending release list and then releasing the references after dropping the IF_ADDR_LOCK. This closes an ugly race where the code was dropping the lock in the middle of iterating over the list. It also fixes some additional potential use-after-free bugs since the cancellation routine also applied other changes to the group after dropping the reference. Now those changes are performed before the reference is dropped and the group is potentially freed. Prodded to fix by: glebius Reviewed by: bz MFC after: 1 week --- sys/netinet/igmp.c | 14 ++++++-------- sys/netinet6/mld6.c | 15 +++++++-------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/sys/netinet/igmp.c b/sys/netinet/igmp.c index 79f6db5fb9f..3561fb8ae87 100644 --- a/sys/netinet/igmp.c +++ b/sys/netinet/igmp.c @@ -2003,7 +2003,7 @@ igmp_v3_cancel_link_timers(struct igmp_ifinfo *igi) { 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 +2049,8 @@ igmp_v3_cancel_link_timers(struct igmp_ifinfo *igi) * 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 +2069,10 @@ igmp_v3_cancel_link_timers(struct igmp_ifinfo *igi) _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); + } } /* diff --git a/sys/netinet6/mld6.c b/sys/netinet6/mld6.c index 5ebf4a1d6ab..583ce108179 100644 --- a/sys/netinet6/mld6.c +++ b/sys/netinet6/mld6.c @@ -1656,7 +1656,7 @@ mld_v2_cancel_link_timers(struct mld_ifinfo *mli) { 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); @@ -1695,14 +1695,9 @@ mld_v2_cancel_link_timers(struct mld_ifinfo *mli) * 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: @@ -1720,6 +1715,10 @@ mld_v2_cancel_link_timers(struct mld_ifinfo *mli) } } 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); + } } /*