svn commit: r333967 - head/sys/netinet

[Matthew Macy]

On Mon, May 21, 2018 at 00:47 Marko Zec <zec at fer.hr> wrote:

> On Mon, 21 May 2018 07:12:06 +0000
> Matt Macy <mmacy at freebsd.org> wrote:
>
> > Author: mmacy
> > Date: Mon May 21 07:12:06 2018
> > New Revision: 333967
> > URL: https://svnweb.freebsd.org/changeset/base/333967
> >
> > Log:
> >   ensure that vnet is set when doing in_leavegroup
> >
> > Modified:
> >   head/sys/netinet/in_mcast.c
> >
> > Modified: head/sys/netinet/in_mcast.c
> >
> ==============================================================================
> > --- head/sys/netinet/in_mcast.c       Mon May 21 05:20:23
> > 2018  (r333966) +++ head/sys/netinet/in_mcast.c       Mon May
> > 21 07:12:06 2018      (r333967) @@ -1664,6 +1664,8 @@
> > inp_gcmoptions(epoch_context_t ctx) {
> >       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);
> > @@ -1673,7 +1675,13 @@ inp_gcmoptions(epoch_context_t ctx)
> >               imf = imo->imo_mfilters ? &imo->imo_mfilters[idx] :
> > NULL; if (imf)
> >                       imf_leave(imf);
> > -             (void)in_leavegroup(imo->imo_membership[idx], imf);
> > +             inm = imo->imo_membership[idx];
> > +             ifp = inm->inm_ifp;
> > +             if (ifp)
> > +                     CURVNET_SET(ifp->if_vnet);
>
> Unfortunately, this won't work because CURVNET_SET() expands to a
> sequence of declarations and assignments which are NOT enclosed in a
> single block.  Instead, only the first statement in CURVNET_SET()
> sequence, which is an assert, will be executed conditionally only if
> ifp != NULL, while the rest of the CURVNET_SET() body will fall out of
> the scope of the if (ifp) conditional.
>

That's pretty counter to the way macros are done _everywhere_ else in the
kernel. You should probably fix that.


> I'd recommend backing out this patch, and instead extending the struct
> ip_moptions with an struct vnet * entry, which would be populated
> before scheduling inp_gcmoptions().  Then CURVNET_SET(imo->imo_vnet)
> could be called only once (and unconditionally) in gcmoptions(),
> instead of (attempts at) doing this multiple times in a for loop.
>
> Marko
>
>
> > +             (void)in_leavegroup(inm, imf);
> > +             if (ifp)
> > +                     CURVNET_RESTORE();
> >               if (imf)
> >                       imf_purge(imf);
> >       }
> >
>
>