Date: Wed, 15 Aug 2018 20:23:09 +0000 (UTC) From: Matt Macy <mmacy@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r337866 - in head/sys: net netinet netinet6 Message-ID: <201808152023.w7FKN9LL055254@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201808152023.w7FKN9LL055254>