Date: Mon, 21 May 2018 09:05:09 -0700 From: Matthew Macy <mmacy@freebsd.org> To: Marko Zec <zec@fer.hr> Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r333968 - in head/sys: netinet netinet6 Message-ID: <CAPrugNq37vM828KmzXiOn_e3hRvkKzGyzQG_98Ke2L-Jb-tvuA@mail.gmail.com> In-Reply-To: <20180521105512.375c8a36@x23.koncar-institut.local> References: <201805210834.w4L8YAcD022948@repo.freebsd.org> <20180521105512.375c8a36@x23.koncar-institut.local>
next in thread | previous in thread | raw e-mail | index | archive | help
Looking closer I think the ifp should always be set. I'll just add an assert to that effect and make it non-conditional. -M On Mon, May 21, 2018 at 1:55 AM, Marko Zec <zec@fer.hr> wrote: > On Mon, 21 May 2018 08:34:10 +0000 > Matt Macy <mmacy@freebsd.org> wrote: > >> Author: mmacy >> Date: Mon May 21 08:34:10 2018 >> New Revision: 333968 >> URL: https://svnweb.freebsd.org/changeset/base/333968 >> >> Log: >> in(6)_mcast: Expand out vnet set / restore macro so that they work >> in a conditional block >> Reported by: zec at fer.hr >> >> Modified: >> head/sys/netinet/in_mcast.c >> head/sys/netinet6/in6_mcast.c >> >> Modified: head/sys/netinet/in_mcast.c >> ============================================================================== >> --- head/sys/netinet/in_mcast.c Mon May 21 07:12:06 >> 2018 (r333967) +++ head/sys/netinet/in_mcast.c Mon May >> 21 08:34:10 2018 (r333968) @@ -653,6 +653,7 @@ >> inm_release(struct in_multi *inm) { >> struct ifmultiaddr *ifma; >> struct ifnet *ifp; >> + struct vnet *saved_vnet; >> >> CTR2(KTR_IGMPV3, "%s: refcount is %d", __func__, >> inm->inm_refcount); MPASS(inm->inm_refcount == 0); >> @@ -663,14 +664,16 @@ inm_release(struct in_multi *inm) >> >> /* XXX this access is not covered by IF_ADDR_LOCK */ >> CTR2(KTR_IGMPV3, "%s: purging ifma %p", __func__, ifma); >> - if (ifp) >> - CURVNET_SET(ifp->if_vnet); >> + if (ifp) { >> + saved_vnet = curvnet; >> + curvnet = ifp->if_vnet; >> + } > > Uhmm... please don't do this, for at least two reasons: > > 1) so far we could identify all VNET context switches by tracking > CURVNET_SET / _RESTORE macros. With this change a non-standard hack is > introduced, opening the door for more alternative / creative variations > to come, thus increasing the entropy > > 2) CURVNET_* macros provide elementary capability of tracking vnet > recursions, and much more importantly, forgotten or missed context > restores. Your change breaks this tracking chain. > > Is there a reason one could not extend struct in_multi with a struct > vnet *inm_vnet entry, populate it on struct in_multi creation, and be > done with all curvnet woes in a clean way? > > Marko > > >> inm_purge(inm); >> free(inm, M_IPMADDR); >> >> if_delmulti_ifma_flags(ifma, 1); >> if (ifp) { >> - CURVNET_RESTORE(); >> + curvnet = saved_vnet; >> if_rele(ifp); >> } >> } >> @@ -1666,6 +1669,7 @@ inp_gcmoptions(epoch_context_t ctx) >> struct in_mfilter *imf; >> struct in_multi *inm; >> struct ifnet *ifp; >> + struct vnet *saved_vnet; >> size_t idx, nmships; >> >> imo = __containerof(ctx, struct ip_moptions, imo_epoch_ctx); >> @@ -1677,11 +1681,13 @@ inp_gcmoptions(epoch_context_t ctx) >> imf_leave(imf); >> inm = imo->imo_membership[idx]; >> ifp = inm->inm_ifp; >> - if (ifp) >> - CURVNET_SET(ifp->if_vnet); >> + if (ifp) { >> + saved_vnet = curvnet; >> + curvnet = ifp->if_vnet; >> + } >> (void)in_leavegroup(inm, imf); >> if (ifp) >> - CURVNET_RESTORE(); >> + curvnet = saved_vnet; >> if (imf) >> imf_purge(imf); >> } >> >> Modified: head/sys/netinet6/in6_mcast.c >> ============================================================================== >> --- head/sys/netinet6/in6_mcast.c Mon May 21 07:12:06 >> 2018 (r333967) +++ head/sys/netinet6/in6_mcast.c Mon >> May 21 08:34:10 2018 (r333968) @@ -524,6 +524,7 @@ >> in6m_release(struct in6_multi *inm) { >> struct ifmultiaddr *ifma; >> struct ifnet *ifp; >> + struct vnet *saved_vnet; >> >> CTR2(KTR_MLD, "%s: refcount is %d", __func__, >> inm->in6m_refcount); >> @@ -539,14 +540,16 @@ in6m_release(struct in6_multi *inm) >> KASSERT(ifma->ifma_protospec == NULL, >> ("%s: ifma_protospec != NULL", __func__)); >> >> - if (ifp) >> - CURVNET_SET(ifp->if_vnet); >> + if (ifp) { >> + saved_vnet = curvnet; >> + curvnet = ifp->if_vnet; >> + } >> in6m_purge(inm); >> free(inm, M_IP6MADDR); >> >> if_delmulti_ifma_flags(ifma, 1); >> if (ifp) { >> - CURVNET_RESTORE(); >> + curvnet = saved_vnet; >> if_rele(ifp); >> } >> } >> @@ -1628,6 +1631,7 @@ inp_gcmoptions(epoch_context_t ctx) >> struct in6_mfilter *imf; >> struct in6_multi *inm; >> struct ifnet *ifp; >> + struct vnet *saved_vnet; >> size_t idx, nmships; >> >> imo = __containerof(ctx, struct ip6_moptions, >> imo6_epoch_ctx); @@ -1639,11 +1643,13 @@ >> inp_gcmoptions(epoch_context_t ctx) im6f_leave(imf); >> inm = imo->im6o_membership[idx]; >> ifp = inm->in6m_ifp; >> - if (ifp) >> - CURVNET_SET(ifp->if_vnet); >> + if (ifp) { >> + saved_vnet = curvnet; >> + curvnet = ifp->if_vnet; >> + } >> (void)in6_leavegroup(inm, imf); >> if (ifp) >> - CURVNET_RESTORE(); >> + curvnet = saved_vnet; >> if (imf) >> im6f_purge(imf); >> } >> >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPrugNq37vM828KmzXiOn_e3hRvkKzGyzQG_98Ke2L-Jb-tvuA>