On 30/11/16(Wed) 23:39, Vincent Gross wrote:
> [...] 
> New diff with nits fixed :

Some comments inline

> Index: sys/net/if_vxlan.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vxlan.c,v
> retrieving revision 1.52
> diff -u -p -r1.52 if_vxlan.c
> --- sys/net/if_vxlan.c        29 Nov 2016 10:09:57 -0000      1.52
> +++ sys/net/if_vxlan.c        30 Nov 2016 22:23:01 -0000
> @@ -199,11 +205,35 @@ vxlan_clone_destroy(struct ifnet *ifp)
>  void
>  vxlan_multicast_cleanup(struct ifnet *ifp)
>  {
> -     struct vxlan_softc      *sc = (struct vxlan_softc *)ifp->if_softc;
> -     struct ip_moptions      *imo = &sc->sc_imo;
> -     struct ifnet            *mifp;
> +     struct vxlan_softc       *sc = (struct vxlan_softc *)ifp->if_softc;
> +     struct ip_moptions       *imo;
> +     struct in_multi         **imm;
> +     struct ip6_moptions      *im6o;
                                  ^^^^
This won't compile without INET6, you should try.

> +     struct in6_multi_mship   *im6m, *im6m_next;
> +     struct ifnet             *mifp = NULL;
> +
> +     switch (sc->sc_dst.ss_family) {

It doesn't make sense to bake yet-another-different-but-copy of
carp_multicast_cleanup().

dlg@ already rewrote the copy that was in vlan(4), so please either
stick to the one ine carp(4) or go for the one vlan(4) which is mp-safe.

> +     case AF_INET:
> +             imo = &sc->sc_imo;
> +             mifp = if_get(imo->imo_ifidx);
> +             imm = imo->imo_membership;
> +             while (imo->imo_num_memberships > 0)
> +                     in_delmulti(imm[--imo->imo_num_memberships]);
> +             free(imm, M_IPMOPTS,
> +                 sizeof(struct in_multi *) * IP_MIN_MEMBERSHIPS);
> +             break;
> +#ifdef INET6
> +     case AF_INET6:
> +             im6o = &sc->sc_im6o;
> +             mifp = if_get(im6o->im6o_ifidx);
> +             LIST_FOREACH_SAFE(im6m, &im6o->im6o_memberships, i6mm_chain,
> +                 im6m_next)
> +                     in6_leavegroup(im6m);
> +             break;
> +#endif /* INET6 */
> +     }
> +     bzero(&sc->sc_imu, sizeof(sc->sc_imu));

Prefer memset()/memcpy() to bzero()/bcopy() in new code :)

>  
> -     mifp = if_get(imo->imo_ifidx);

Is is possible to have imo_ifidx and im6o_ifidx?  Can they be different?
It is quite confusing, I'd recomend to store the index of the related
interface in the softc just like we do in vlan(4).  Abusing multicast 
groups fields is not the way to go.

I'd also suggest to rename "mifp" into "ifp0" to be coherent with other
pseudo-drivers.

>       if (mifp != NULL) {
>               if (sc->sc_ahcookie != NULL) {
>                       hook_disestablish(mifp->if_addrhooks, sc->sc_ahcookie);
> @@ -219,14 +249,9 @@ vxlan_multicast_cleanup(struct ifnet *if
>                           sc->sc_dhcookie);
>                       sc->sc_dhcookie = NULL;
>               }
> -
> -             if_put(mifp);
>       }
>  
> -     if (imo->imo_num_memberships > 0) {
> -             in_delmulti(imo->imo_membership[--imo->imo_num_memberships]);
> -             imo->imo_ifidx = 0;
> -     }
> +     if_put(mifp);
>  }
>  
>  int
> @@ -234,55 +259,141 @@ vxlan_multicast_join(struct ifnet *ifp, 
>      struct sockaddr *dst)
>  {
>       struct vxlan_softc      *sc = ifp->if_softc;
> -     struct ip_moptions      *imo = &sc->sc_imo;
> +     struct ip_moptions      *imo;
> +     struct ip6_moptions     *im6o;
> +     struct in6_multi_mship  *im6m;
                                 ^^^^
That should certainly be under INET6.

>       struct sockaddr_in      *src4, *dst4;
>  #ifdef INET6
> -     struct sockaddr_in6     *dst6;
> +     struct sockaddr_in6     *src6, *dst6;
>  #endif /* INET6 */
>       struct ifaddr           *ifa;
> -     struct ifnet            *mifp;
> +     struct ifnet            *mifp, *m6ifp = NULL;
> +     struct rtentry          *rt;
> +     int                      error;
>  

Like previously I'd suggest adding vxlan_join_multicast6() that mirrors
what's in carp(4).

>       switch (dst->sa_family) {
>       case AF_INET:
>               dst4 = satosin(dst);
> +             src4 = satosin(src);
>               if (!IN_MULTICAST(dst4->sin_addr.s_addr))
>                       return (0);
> +             if (src4->sin_addr.s_addr == INADDR_ANY ||
> +                 IN_MULTICAST(src4->sin_addr.s_addr))
> +                     return (EINVAL);
> +             if ((ifa = ifa_ifwithaddr(src, sc->sc_rdomain)) == NULL ||
> +                 (mifp = ifa->ifa_ifp) == NULL ||
> +                 (mifp->if_flags & IFF_MULTICAST) == 0)
> +                     return (EADDRNOTAVAIL);
> +
> +             imo = &sc->sc_imo;
> +             imo->imo_membership = mallocarray(sizeof(struct in_multi *),
> +                 IP_MIN_MEMBERSHIPS, M_IPMOPTS, M_NOWAIT|M_ZERO);

I'd malloc() that in vxlan_clone(), like for carp(4) then you can use
M_WAITOK and don't clutter your error path with the free(9).

> +             if (imo->imo_membership == NULL ||
> +                 (imo->imo_membership[0] =
> +                 in_addmulti(&dst4->sin_addr, mifp)) == NULL) {
> +                     free(imo->imo_membership, M_IPMOPTS,
> +                         sizeof(struct in_multi *) * IP_MIN_MEMBERSHIPS);
> +                     return (ENOMEM);
> +             }
> +             imo->imo_max_memberships = IP_MIN_MEMBERSHIPS;
> +             imo->imo_num_memberships++;
> +             imo->imo_ifidx = mifp->if_index;
> +             if (sc->sc_ttl > 0)
> +                     imo->imo_ttl = sc->sc_ttl;
> +             else
> +                     imo->imo_ttl = IP_DEFAULT_MULTICAST_TTL;
> +             imo->imo_loop = 0;
>               break;
>  #ifdef INET6
>       case AF_INET6:
>               dst6 = satosin6(dst);
> +             src6 = satosin6(src);
>               if (!IN6_IS_ADDR_MULTICAST(&dst6->sin6_addr))
>                       return (0);
> +             if (IN6_IS_ADDR_UNSPECIFIED(&src6->sin6_addr) ||
> +                 IN6_IS_ADDR_MULTICAST(&src6->sin6_addr))
> +                     return (EINVAL);
> +             if (IN6_IS_ADDR_MC_LINKLOCAL(&dst6->sin6_addr) ||
> +                 IN6_IS_ADDR_MC_INTFACELOCAL(&dst6->sin6_addr)) {
> +                     /* Scoped mcast destination : get index from dst6 */
> +                     if (dst6->sin6_scope_id)
> +                             m6ifp = if_get(dst6->sin6_scope_id);

Do I understand correctly that you're dealing with scoped mcast
addresses given by ifconfig(8) as argument from SIOCSLIFPHYADDR?  Is
that intended?

> +             } else {
> +                     /*
> +                      * unscoped multicast destination : get ifindex
> +                      * from src6, or routing table as fallack
> +                      */
> +                     if (src6->sin6_scope_id)
> +                             m6ifp = if_get(src6->sin6_scope_id);
> +                     if (m6ifp == NULL) {
> +                             rt = rtalloc(dst, 0, sc->sc_rdomain);
> +                             if (rt)
> +                                     m6ifp = if_get(rt->rt_ifidx);
> +                             rtfree(rt);
> +                     }

Why do we need all theses paths?  Can't we keep the kernel dumb and
enforce a policy or fix userland tools?

> +             }
> +             if (!m6ifp || !ISSET(m6ifp->if_flags, IFF_MULTICAST)) {
> +                     if_put(m6ifp);
> +                     return (EADDRNOTAVAIL);
> +             }
> +
> +             if ((error = in6_embedscope(&src6->sin6_addr, src6, NULL)) ||
> +                 (error = in6_embedscope(&dst6->sin6_addr, dst6, NULL))) {
> +                     if_put(m6ifp);
> +                     return (error);
> +             }

Funny that this functions never fails 8)


> +             /* do we own this address ? */
> +             rt = rtalloc(src, 0, sc->sc_rdomain);
> +             if (!rtisvalid(rt) || !ISSET(rt->rt_flags, RTF_LOCAL)) {
> +                     if_put(m6ifp);
> +                     rtfree(rt);
> +                     return (EADDRNOTAVAIL);
> +             }
> +             rtfree(rt);

Note that you are doing a routing lookup here, these could hopefully
also be done in the v4 case and reduce one more ifa_ifwith*() function
call.


>  
> -             /* Multicast mode is currently not supported for IPv6 */
> -             return (EAFNOSUPPORT);
> +             /*
> +              * If src6 has a global scope, then we do no further checks
> +              * because whichever interface we reach on the remote
> +              * endpoint, the EP will have all it needs to reach us back on
> +              * unicast should it decide to do so (think dynamic and
> +              * multipoint).
> +              *
> +              * However, if the source address has a scope smaller than
> +              * global, we must ensure that the source address is inside
> +              * the destination address' zone.
> +              */
> +             if (IN6_IS_ADDR_LINKLOCAL(&src6->sin6_addr) ||
> +                 IN6_IS_ADDR_SITELOCAL(&src6->sin6_addr)) {
> +                     /*
> +                      * As a first shot, let's say that sin6_scope_id
> +                      * must be equal.
> +                      */
> +                     if (src6->sin6_scope_id != dst6->sin6_scope_id) {
> +                             if_put(m6ifp);
> +                             return (EADDRNOTAVAIL);
> +                     }
> +             }

I'd do this check before doing the lookup, so you don't have to keep a
complicated error path.

> +
> +             im6o = &sc->sc_im6o;
> +             im6m = in6_joingroup(m6ifp, &dst6->sin6_addr, &error);
> +             if (!im6m) {
> +                     if_put(m6ifp);
> +                     return (error);
> +             }
> +             im6o->im6o_ifidx = m6ifp->if_index;
> +             LIST_INSERT_HEAD(&im6o->im6o_memberships, im6m, i6mm_chain);
> +             if (sc->sc_ttl > 0)
> +                     im6o->im6o_hlim = sc->sc_ttl;
> +             else
> +                     im6o->im6o_hlim = ip6_defmcasthlim;
> +             im6o->im6o_loop = 0;
> +             mifp = m6ifp;
> +             break;
>  #endif /* INET6 */
>       default:
>               return (EAFNOSUPPORT);
>       }
>  
> -     src4 = satosin(src);
> -     dst4 = satosin(dst);
> -
> -     if (src4->sin_addr.s_addr == INADDR_ANY ||
> -         IN_MULTICAST(src4->sin_addr.s_addr))
> -             return (EINVAL);
> -     if ((ifa = ifa_ifwithaddr(src, sc->sc_rdomain)) == NULL ||
> -         (mifp = ifa->ifa_ifp) == NULL ||
> -         (mifp->if_flags & IFF_MULTICAST) == 0)
> -             return (EADDRNOTAVAIL);
> -
> -     if ((imo->imo_membership[0] =
> -         in_addmulti(&dst4->sin_addr, mifp)) == NULL)
> -             return (ENOBUFS);
> -
> -     imo->imo_num_memberships++;
> -     imo->imo_ifidx = mifp->if_index;
> -     if (sc->sc_ttl > 0)
> -             imo->imo_ttl = sc->sc_ttl;
> -     else
> -             imo->imo_ttl = IP_DEFAULT_MULTICAST_TTL;
> -     imo->imo_loop = 0;
>  
>       /*
>        * Use interface hooks to track any changes on the interface
> @@ -297,6 +408,8 @@ vxlan_multicast_join(struct ifnet *ifp, 
>               panic("%s: cannot allocate interface hook",
>                   mifp->if_xname);
>  
> +     if_put(m6ifp);
> +
>       return (0);
>  }
>  
> @@ -902,7 +1015,8 @@ vxlan_output(struct ifnet *ifp, struct m
>               break;
>  #ifdef INET6
>       case AF_INET6:
> -             error = ip6_output(m, 0, NULL, IPV6_MINMTU, 0, NULL);
> +             error = ip6_output(m, 0, NULL, IPV6_MINMTU,
> +                 &sc->sc_im6o, NULL);
>               break;
>  #endif /* INET6 */
>       default:
> 

Reply via email to