Author: mmacy
Date: Wed Aug 15 20:23:08 2018
New Revision: 337866
URL: https://svnweb.freebsd.org/changeset/base/337866
Log:
Fix in6_multi double free
This is actually several different bugs:
- The code is not designed to handle inpcb deletion after interface deletion
- add reference for inpcb membership
- The multicast address has to be removed from interface lists when the
refcount
goes to zero OR when the interface goes away
- decouple list disconnect from refcount (v6 only for now)
- ifmultiaddr can exist past being on interface lists
- add flag for tracking whether or not it's enqueued
- deferring freeing moptions makes the incpb cleanup code simpler but opens
the
door wider still to races
- call inp_gcmoptions synchronously after dropping the the inpcb lock
Fundamentally multicast needs a rewrite - but keep applying band-aids for now.
Tested by: kp
Reported by: novel, kp, lwhsu
Modified:
head/sys/net/if.c
head/sys/net/if_var.h
head/sys/netinet/in_mcast.c
head/sys/netinet/in_pcb.c
head/sys/netinet/ip_carp.c
head/sys/netinet6/in6_ifattach.c
head/sys/netinet6/in6_mcast.c
head/sys/netinet6/in6_var.h
head/sys/netinet6/mld6.c
Modified: head/sys/net/if.c
==============================================================================
--- head/sys/net/if.c Wed Aug 15 19:46:13 2018 (r337865)
+++ head/sys/net/if.c Wed Aug 15 20:23:08 2018 (r337866)
@@ -3545,6 +3545,7 @@ if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
error = ENOMEM;
goto free_llsa_out;
}
+ ll_ifma->ifma_flags |= IFMA_F_ENQUEUED;
CK_STAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ll_ifma,
ifma_link);
} else
@@ -3557,6 +3558,7 @@ if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
* referenced link layer address. Add the primary address to the
* ifnet address list.
*/
+ ifma->ifma_flags |= IFMA_F_ENQUEUED;
CK_STAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link);
if (retifma != NULL)
@@ -3757,9 +3759,10 @@ if_delmulti_locked(struct ifnet *ifp, struct ifmultiad
if (--ifma->ifma_refcount > 0)
return 0;
- if (ifp != NULL && detaching == 0)
+ if (ifp != NULL && detaching == 0 && (ifma->ifma_flags &
IFMA_F_ENQUEUED)) {
CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifmultiaddr,
ifma_link);
-
+ ifma->ifma_flags &= ~IFMA_F_ENQUEUED;
+ }
/*
* If this ifma is a network-layer ifma, a link-layer ifma may
* have been associated with it. Release it first if so.
@@ -3772,8 +3775,11 @@ if_delmulti_locked(struct ifnet *ifp, struct ifmultiad
ll_ifma->ifma_ifp = NULL; /* XXX */
if (--ll_ifma->ifma_refcount == 0) {
if (ifp != NULL) {
- CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ll_ifma,
ifmultiaddr,
- ifma_link);
+ if (ll_ifma->ifma_flags & IFMA_F_ENQUEUED) {
+ CK_STAILQ_REMOVE(&ifp->if_multiaddrs,
ll_ifma, ifmultiaddr,
+ ifma_link);
+ ifma->ifma_flags &= ~IFMA_F_ENQUEUED;
+ }
}
if_freemulti(ll_ifma);
}
Modified: head/sys/net/if_var.h
==============================================================================
--- head/sys/net/if_var.h Wed Aug 15 19:46:13 2018 (r337865)
+++ head/sys/net/if_var.h Wed Aug 15 20:23:08 2018 (r337866)
@@ -548,12 +548,14 @@ void ifa_ref(struct ifaddr *ifa);
* Multicast address structure. This is analogous to the ifaddr
* structure except that it keeps track of multicast addresses.
*/
+#define IFMA_F_ENQUEUED 0x1
struct ifmultiaddr {
CK_STAILQ_ENTRY(ifmultiaddr) ifma_link; /* queue macro glue */
struct sockaddr *ifma_addr; /* address this membership is for */
struct sockaddr *ifma_lladdr; /* link-layer translation, if any */
struct ifnet *ifma_ifp; /* back-pointer to interface */
u_int ifma_refcount; /* reference count */
+ int ifma_flags;
void *ifma_protospec; /* protocol-specific state, if any */
struct ifmultiaddr *ifma_llifma; /* pointer to ifma for ifma_lladdr */
struct epoch_context ifma_epoch_ctx;
Modified: head/sys/netinet/in_mcast.c
==============================================================================
--- head/sys/netinet/in_mcast.c Wed Aug 15 19:46:13 2018 (r337865)
+++ head/sys/netinet/in_mcast.c Wed Aug 15 20:23:08 2018 (r337866)
@@ -263,7 +263,10 @@ inm_disconnect(struct in_multi *inm)
ifma = inm->inm_ifma;
if_ref(ifp);
- CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifmultiaddr, ifma_link);
+ if (ifma->ifma_flags & IFMA_F_ENQUEUED) {
+ CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifmultiaddr,
ifma_link);
+ ifma->ifma_flags &= ~IFMA_F_ENQUEUED;
+ }
MCDPRINTF("removed ifma: %p from %s\n", ifma, ifp->if_xname);
if ((ll_ifma = ifma->ifma_llifma) != NULL) {
MPASS(ifma != ll_ifma);
@@ -271,7 +274,10 @@ inm_disconnect(struct in_multi *inm)
MPASS(ll_ifma->ifma_llifma == NULL);
MPASS(ll_ifma->ifma_ifp == ifp);
if (--ll_ifma->ifma_refcount == 0) {
- CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ll_ifma,
ifmultiaddr, ifma_link);
+ if (ll_ifma->ifma_flags & IFMA_F_ENQUEUED) {
+ CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ll_ifma,
ifmultiaddr, ifma_link);
+ ifma->ifma_flags &= ~IFMA_F_ENQUEUED;
+ }
MCDPRINTF("removed ll_ifma: %p from %s\n", ll_ifma,
ifp->if_xname);
if_freemulti(ll_ifma);
ifma_restart = true;
@@ -1668,16 +1674,13 @@ inp_findmoptions(struct inpcb *inp)
}
static void
-inp_gcmoptions(epoch_context_t ctx)
+inp_gcmoptions(struct ip_moptions *imo)
{
- struct ip_moptions *imo;
struct in_mfilter *imf;
struct in_multi *inm;
struct ifnet *ifp;
size_t idx, nmships;
- imo = __containerof(ctx, struct ip_moptions, imo_epoch_ctx);
-
nmships = imo->imo_num_memberships;
for (idx = 0; idx < nmships; ++idx) {
imf = imo->imo_mfilters ? &imo->imo_mfilters[idx] : NULL;
@@ -1713,7 +1716,7 @@ inp_freemoptions(struct ip_moptions *imo)
{
if (imo == NULL)
return;
- epoch_call(net_epoch_preempt, &imo->imo_epoch_ctx, inp_gcmoptions);
+ inp_gcmoptions(imo);
}
/*
@@ -2265,7 +2268,8 @@ inp_join_group(struct inpcb *inp, struct sockopt *sopt
__func__);
IN_MULTI_LIST_UNLOCK();
goto out_imo_free;
- }
+ }
+ inm_acquire(inm);
imo->imo_membership[idx] = inm;
} else {
CTR1(KTR_IGMPV3, "%s: merge inm state", __func__);
@@ -2305,6 +2309,12 @@ out_in_multi_locked:
out_imo_free:
if (error && is_new) {
+ inm = imo->imo_membership[idx];
+ if (inm != NULL) {
+ IN_MULTI_LIST_LOCK();
+ inm_release_deferred(inm);
+ IN_MULTI_LIST_UNLOCK();
+ }
imo->imo_membership[idx] = NULL;
--imo->imo_num_memberships;
}
Modified: head/sys/netinet/in_pcb.c
==============================================================================
--- head/sys/netinet/in_pcb.c Wed Aug 15 19:46:13 2018 (r337865)
+++ head/sys/netinet/in_pcb.c Wed Aug 15 20:23:08 2018 (r337866)
@@ -1578,7 +1578,7 @@ in_pcbfree_deferred(epoch_context_t ctx)
INP_WLOCK(inp);
#ifdef INET
- inp_freemoptions(inp->inp_moptions);
+ struct ip_moptions *imo = inp->inp_moptions;
inp->inp_moptions = NULL;
#endif
/* XXXRW: Do as much as possible here. */
@@ -1587,9 +1587,10 @@ in_pcbfree_deferred(epoch_context_t ctx)
ipsec_delete_pcbpolicy(inp);
#endif
#ifdef INET6
+ struct ip6_moptions *im6o = NULL;
if (inp->inp_vflag & INP_IPV6PROTO) {
ip6_freepcbopts(inp->in6p_outputopts);
- ip6_freemoptions(inp->in6p_moptions);
+ im6o = inp->in6p_moptions;
inp->in6p_moptions = NULL;
}
#endif
@@ -1602,6 +1603,12 @@ in_pcbfree_deferred(epoch_context_t ctx)
#endif
released = in_pcbrele_wlocked(inp);
MPASS(released);
+#ifdef INET6
+ ip6_freemoptions(im6o);
+#endif
+#ifdef INET
+ inp_freemoptions(imo);
+#endif
}
/*
Modified: head/sys/netinet/ip_carp.c
==============================================================================
--- head/sys/netinet/ip_carp.c Wed Aug 15 19:46:13 2018 (r337865)
+++ head/sys/netinet/ip_carp.c Wed Aug 15 20:23:08 2018 (r337866)
@@ -1423,6 +1423,7 @@ carp_multicast_setup(struct carp_if *cif, sa_family_t
free(im6o->im6o_membership, M_CARP);
break;
}
+ in6m_acquire(in6m);
im6o->im6o_membership[0] = in6m;
im6o->im6o_num_memberships++;
@@ -1444,6 +1445,7 @@ carp_multicast_setup(struct carp_if *cif, sa_family_t
free(im6o->im6o_membership, M_CARP);
break;
}
+ in6m_acquire(in6m);
im6o->im6o_membership[1] = in6m;
im6o->im6o_num_memberships++;
break;
Modified: head/sys/netinet6/in6_ifattach.c
==============================================================================
--- head/sys/netinet6/in6_ifattach.c Wed Aug 15 19:46:13 2018
(r337865)
+++ head/sys/netinet6/in6_ifattach.c Wed Aug 15 20:23:08 2018
(r337866)
@@ -863,6 +863,7 @@ in6_purgemaddrs(struct ifnet *ifp)
ifma->ifma_protospec == NULL)
continue;
inm = (struct in6_multi *)ifma->ifma_protospec;
+ in6m_disconnect(inm);
in6m_rele_locked(&purgeinms, inm);
if (__predict_false(ifma6_restart)) {
ifma6_restart = false;
Modified: head/sys/netinet6/in6_mcast.c
==============================================================================
--- head/sys/netinet6/in6_mcast.c Wed Aug 15 19:46:13 2018
(r337865)
+++ head/sys/netinet6/in6_mcast.c Wed Aug 15 20:23:08 2018
(r337866)
@@ -538,6 +538,8 @@ in6m_release(struct in6_multi *inm)
CTR2(KTR_MLD, "%s: purging ifma %p", __func__, ifma);
KASSERT(ifma->ifma_protospec == NULL,
("%s: ifma_protospec != NULL", __func__));
+ if (ifp == NULL)
+ ifp = ifma->ifma_ifp;
if (ifp != NULL) {
CURVNET_SET(ifp->if_vnet);
@@ -592,11 +594,20 @@ in6m_disconnect(struct in6_multi *inm)
struct ifmultiaddr *ifma, *ll_ifma;
ifp = inm->in6m_ifp;
+
+ if (ifp == NULL)
+ return;
+ inm->in6m_ifp = NULL;
IF_ADDR_WLOCK_ASSERT(ifp);
ifma = inm->in6m_ifma;
+ if (ifma == NULL)
+ return;
if_ref(ifp);
- CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifmultiaddr, ifma_link);
+ if (ifma->ifma_flags & IFMA_F_ENQUEUED) {
+ CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifmultiaddr,
ifma_link);
+ ifma->ifma_flags &= ~IFMA_F_ENQUEUED;
+ }
MCDPRINTF("removed ifma: %p from %s\n", ifma, ifp->if_xname);
if ((ll_ifma = ifma->ifma_llifma) != NULL) {
MPASS(ifma != ll_ifma);
@@ -605,7 +616,10 @@ in6m_disconnect(struct in6_multi *inm)
MPASS(ll_ifma->ifma_ifp == ifp);
if (--ll_ifma->ifma_refcount == 0) {
ifma6_restart = true;
- CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ll_ifma,
ifmultiaddr, ifma_link);
+ if (ll_ifma->ifma_flags & IFMA_F_ENQUEUED) {
+ CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ll_ifma,
ifmultiaddr, ifma_link);
+ ifma->ifma_flags &= ~IFMA_F_ENQUEUED;
+ }
MCDPRINTF("removed ll_ifma: %p from %s\n", ll_ifma,
ifp->if_xname);
if_freemulti(ll_ifma);
}
@@ -632,7 +646,7 @@ in6m_release_deferred(struct in6_multi *inm)
IN6_MULTI_LIST_LOCK_ASSERT();
KASSERT(inm->in6m_refcount > 0, ("refcount == %d inm: %p",
inm->in6m_refcount, inm));
if (--inm->in6m_refcount == 0) {
- in6m_disconnect(inm);
+ MPASS(inm->in6m_ifp == NULL);
SLIST_INIT(&tmp);
inm->in6m_ifma->ifma_protospec = NULL;
MPASS(inm->in6m_ifma->ifma_llifma == NULL);
@@ -1310,6 +1324,7 @@ out_in6m_release:
break;
}
}
+ in6m_disconnect(inm);
in6m_release_deferred(inm);
IF_ADDR_RUNLOCK(ifp);
} else {
@@ -1389,13 +1404,17 @@ in6_leavegroup_locked(struct in6_multi *inm, /*const*/
KASSERT(error == 0, ("%s: failed to merge inm state", __func__));
CTR1(KTR_MLD, "%s: doing mld downcall", __func__);
- error = mld_change_state(inm, 0);
+ error = 0;
+ if (ifp)
+ error = mld_change_state(inm, 0);
if (error)
CTR1(KTR_MLD, "%s: failed mld downcall", __func__);
CTR2(KTR_MLD, "%s: dropping ref on %p", __func__, inm);
if (ifp)
IF_ADDR_WLOCK(ifp);
+ if (inm->in6m_refcount == 1 && inm->in6m_ifp != NULL)
+ in6m_disconnect(inm);
in6m_release_deferred(inm);
if (ifp)
IF_ADDR_WUNLOCK(ifp);
@@ -1629,16 +1648,13 @@ in6p_findmoptions(struct inpcb *inp)
*/
static void
-inp_gcmoptions(epoch_context_t ctx)
+inp_gcmoptions(struct ip6_moptions *imo)
{
- struct ip6_moptions *imo;
struct in6_mfilter *imf;
struct in6_multi *inm;
struct ifnet *ifp;
size_t idx, nmships;
- imo = __containerof(ctx, struct ip6_moptions, imo6_epoch_ctx);
-
nmships = imo->im6o_num_memberships;
for (idx = 0; idx < nmships; ++idx) {
imf = imo->im6o_mfilters ? &imo->im6o_mfilters[idx] : NULL;
@@ -1668,7 +1684,7 @@ ip6_freemoptions(struct ip6_moptions *imo)
{
if (imo == NULL)
return;
- epoch_call(net_epoch_preempt, &imo->imo6_epoch_ctx, inp_gcmoptions);
+ inp_gcmoptions(imo);
}
/*
@@ -2162,6 +2178,7 @@ in6p_join_group(struct inpcb *inp, struct sockopt *sop
IN6_MULTI_UNLOCK();
goto out_im6o_free;
}
+ in6m_acquire(inm);
imo->im6o_membership[idx] = inm;
} else {
CTR1(KTR_MLD, "%s: merge inm state", __func__);
@@ -2196,6 +2213,12 @@ in6p_join_group(struct inpcb *inp, struct sockopt *sop
out_im6o_free:
if (error && is_new) {
+ inm = imo->im6o_membership[idx];
+ if (inm != NULL) {
+ IN6_MULTI_LIST_LOCK();
+ in6m_release_deferred(inm);
+ IN6_MULTI_LIST_UNLOCK();
+ }
imo->im6o_membership[idx] = NULL;
--imo->im6o_num_memberships;
}
Modified: head/sys/netinet6/in6_var.h
==============================================================================
--- head/sys/netinet6/in6_var.h Wed Aug 15 19:46:13 2018 (r337865)
+++ head/sys/netinet6/in6_var.h Wed Aug 15 20:23:08 2018 (r337866)
@@ -784,7 +784,7 @@ in6m_rele_locked(struct in6_multi_head *inmh, struct i
IN6_MULTI_LIST_LOCK_ASSERT();
if (--inm->in6m_refcount == 0) {
- in6m_disconnect(inm);
+ MPASS(inm->in6m_ifp == NULL);
inm->in6m_ifma->ifma_protospec = NULL;
MPASS(inm->in6m_ifma->ifma_llifma == NULL);
SLIST_INSERT_HEAD(inmh, inm, in6m_nrele);
Modified: head/sys/netinet6/mld6.c
==============================================================================
--- head/sys/netinet6/mld6.c Wed Aug 15 19:46:13 2018 (r337865)
+++ head/sys/netinet6/mld6.c Wed Aug 15 20:23:08 2018 (r337866)
@@ -557,6 +557,7 @@ mld_ifdetach(struct ifnet *ifp)
continue;
inm = (struct in6_multi *)ifma->ifma_protospec;
if (inm->in6m_state == MLD_LEAVING_MEMBER) {
+ in6m_disconnect(inm);
in6m_rele_locked(&inmh, inm);
ifma->ifma_protospec = NULL;
}
@@ -1483,6 +1484,7 @@ mld_v1_process_group_timer(struct in6_multi_head *inmh
case MLD_REPORTING_MEMBER:
if (report_timer_expired) {
inm->in6m_state = MLD_IDLE_MEMBER;
+ in6m_disconnect(inm);
in6m_rele_locked(inmh, inm);
}
break;
@@ -1607,6 +1609,7 @@ mld_v2_process_group_timers(struct in6_multi_head *inm
if (inm->in6m_state == MLD_LEAVING_MEMBER &&
inm->in6m_scrv == 0) {
inm->in6m_state = MLD_NOT_MEMBER;
+ in6m_disconnect(inm);
in6m_rele_locked(inmh, inm);
}
}
@@ -1697,6 +1700,7 @@ mld_v2_cancel_link_timers(struct mld_ifsoftc *mli)
* version, we need to release the final
* reference held for issuing the INCLUDE {}.
*/
+ in6m_disconnect(inm);
in6m_rele_locked(&inmh, inm);
ifma->ifma_protospec = NULL;
/* FALLTHROUGH */
@@ -1893,16 +1897,15 @@ mld_change_state(struct in6_multi *inm, const int dela
*/
KASSERT(inm->in6m_ifma != NULL, ("%s: no ifma", __func__));
ifp = inm->in6m_ifma->ifma_ifp;
- if (ifp != NULL) {
- /*
- * Sanity check that netinet6's notion of ifp is the
- * same as net's.
- */
- KASSERT(inm->in6m_ifp == ifp, ("%s: bad ifp", __func__));
- }
+ if (ifp == NULL)
+ return (0);
+ /*
+ * Sanity check that netinet6's notion of ifp is the
+ * same as net's.
+ */
+ KASSERT(inm->in6m_ifp == ifp, ("%s: bad ifp", __func__));
MLD_LOCK();
-
mli = MLD_IFINFO(ifp);
KASSERT(mli != NULL, ("%s: no mld_ifsoftc for ifp %p", __func__, ifp));
@@ -1996,9 +1999,9 @@ mld_initial_join(struct in6_multi *inm, struct mld_ifs
* group around for the final INCLUDE {} enqueue.
*/
if (mli->mli_version == MLD_VERSION_2 &&
- inm->in6m_state == MLD_LEAVING_MEMBER)
- in6m_release_deferred(inm);
-
+ inm->in6m_state == MLD_LEAVING_MEMBER) {
+ inm->in6m_refcount--;
+ }
inm->in6m_state = MLD_REPORTING_MEMBER;
switch (mli->mli_version) {
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
