svn commit: r333968 - in head/sys: netinet netinet6

[Matthew Macy]

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 at fer.hr> wrote:
> On Mon, 21 May 2018 08:34:10 +0000
> Matt Macy <mmacy at 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);
>>       }
>>
>