From owner-svn-src-all@freebsd.org Mon May 21 16:05:10 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B64B9EF5DAB; Mon, 21 May 2018 16:05:10 +0000 (UTC) (envelope-from mmacy@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 66394842CE; Mon, 21 May 2018 16:05:10 +0000 (UTC) (envelope-from mmacy@freebsd.org) Received: from mail-io0-f172.google.com (mail-io0-f172.google.com [209.85.223.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) (Authenticated sender: mmacy) by smtp.freebsd.org (Postfix) with ESMTPSA id 2CF7217043; Mon, 21 May 2018 16:05:10 +0000 (UTC) (envelope-from mmacy@freebsd.org) Received: by mail-io0-f172.google.com with SMTP id g14-v6so14892826ioc.7; Mon, 21 May 2018 09:05:10 -0700 (PDT) X-Gm-Message-State: ALKqPwc5hoFpW0bNE6E0e3u2apVmoQ3dNDxYZhL+btM+97U5EYa86rZN wxwLVzgp1HjAx/QewD1ZYmCTf4y0fd2I/ytsQMU= X-Google-Smtp-Source: AB8JxZrS7/CDfUk2D3N0FolNQxeIr3s93UAYH98FyjovIM/PyPAjgpPXE9FRwbDZ1zruEj3YWD1m3CsbEsOKyy8RYF4= X-Received: by 2002:a6b:a712:: with SMTP id q18-v6mr21759241ioe.237.1526918709700; Mon, 21 May 2018 09:05:09 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a02:85ae:0:0:0:0:0 with HTTP; Mon, 21 May 2018 09:05:09 -0700 (PDT) In-Reply-To: <20180521105512.375c8a36@x23.koncar-institut.local> References: <201805210834.w4L8YAcD022948@repo.freebsd.org> <20180521105512.375c8a36@x23.koncar-institut.local> From: Matthew Macy Date: Mon, 21 May 2018 09:05:09 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r333968 - in head/sys: netinet netinet6 To: Marko Zec Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 May 2018 16:05:11 -0000 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 wrote: > On Mon, 21 May 2018 08:34:10 +0000 > Matt Macy 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); >> } >> >