On Sat, Nov 05, 2016 at 12:41:39PM +0100, Vincent Gross wrote:
> Updated diff, I reworked the logic to handle the if_get/if_put dance in
> vxlan_multicast_join(), and fixed an uninitialized variable.
>
> Ok ?
Some nits inline.
>
> Index: net/if_vxlan.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vxlan.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 if_vxlan.c
> --- net/if_vxlan.c 25 Oct 2016 16:31:08 -0000 1.51
> +++ net/if_vxlan.c 5 Nov 2016 11:36:02 -0000
> @@ -47,6 +47,8 @@
> #include <netinet/udp_var.h>
> #include <netinet/in_pcb.h>
>
#ifdef INET6
> +#include <netinet6/in6_var.h>
> +
> #if NPF > 0
> #include <net/pfvar.h>
> #endif
> @@ -61,7 +63,12 @@ struct vxlan_softc {
> struct arpcom sc_ac;
> struct ifmedia sc_media;
>
> - struct ip_moptions sc_imo;
> + union {
> + struct ip_moptions u_imo;
#ifdef INET6
> + struct ip6_moptions u_imo6;
> + } sc_imu;
> +#define sc_imo sc_imu.u_imo
> +#define sc_im6o sc_imu.u_imo6
Please write 6o or o6 consistently. I think u_im6o is better.
> void *sc_ahcookie;
> void *sc_lhcookie;
> void *sc_dhcookie;
> @@ -129,10 +136,6 @@ vxlan_clone_create(struct if_clone *ifc,
> M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL)
> return (ENOMEM);
>
> - sc->sc_imo.imo_membership = malloc(
> - (sizeof(struct in_multi *) * IP_MIN_MEMBERSHIPS), M_IPMOPTS,
> - M_WAITOK|M_ZERO);
> - sc->sc_imo.imo_max_memberships = IP_MIN_MEMBERSHIPS;
> sc->sc_dstport = htons(VXLAN_PORT);
> sc->sc_vnetid = VXLAN_VNI_UNSET;
>
> @@ -190,7 +193,6 @@ vxlan_clone_destroy(struct ifnet *ifp)
> ifmedia_delete_instance(&sc->sc_media, IFM_INST_ANY);
> ether_ifdetach(ifp);
> if_detach(ifp);
> - free(sc->sc_imo.imo_membership, M_IPMOPTS, 0);
> free(sc, M_DEVBUF, sizeof(*sc));
>
> return (0);
> @@ -199,11 +201,33 @@ 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;
> + struct in6_multi_mship *im6m, *im6m_next;
> + struct ifnet *mifp = NULL;
> +
> + switch (sc->sc_dst.ss_family) {
> + 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 *) * imo->imo_num_memberships);
imo->imo_num_memberships is always 0 here, use 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;
> + }
> + bzero(&sc->sc_imu, sizeof(sc->sc_imu));
>
> - mifp = if_get(imo->imo_ifidx);
> if (mifp != NULL) {
> if (sc->sc_ahcookie != NULL) {
> hook_disestablish(mifp->if_addrhooks, sc->sc_ahcookie);
> @@ -219,14 +243,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 +253,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;
> 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 = NULL, *m6ifp = NULL;
Initialisation mifp = NULL is not necessary.
> + struct rtentry *rt;
> + int error;
>
> 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_WAITOK|M_ZERO);
May we sleep here? As we have added a lot of tasks to if.c it may
be fine.
> + if (imo->imo_membership == NULL ||
This cannot happen with M_WAITOK. And I would just return a ENOMEM
after malloc(9) failed.
> + (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 (ENOBUFS);
> + }
> + 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);
> + } 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, RT_RESOLVE, sc->sc_rdomain);
Do we have to resolve the route here? Is a simple lookup for the
cloning route not sufficient?
> + if (rt)
> + m6ifp = if_get(rt->rt_ifidx);
> + rtfree(rt);
> + }
> + }
> + 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);
> + }
> + /* 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);
>
> - /* 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);
> + }
> + }
> +
> + im6o = &sc->sc_im6o;
> + im6o->im6o_ifidx = m6ifp->if_index;
> + im6m = in6_joingroup(m6ifp, &dst6->sin6_addr, &error);
> + if (!im6m) {
> + if_put(m6ifp);
> + return (error);
> + }
Please set im6o->im6o_ifidx only after in6_joingroup() was successful.
> + 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 +402,8 @@ vxlan_multicast_join(struct ifnet *ifp,
> panic("%s: cannot allocate interface hook",
> mifp->if_xname);
>
> + if_put(m6ifp);
> +
> return (0);
> }
>
> @@ -902,7 +1009,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:
