From 2d222cb761fa2934bc4945af286c52265f0c52e6 Mon Sep 17 00:00:00 2001 From: Alexander Motin Date: Mon, 4 Aug 2014 00:58:12 +0000 Subject: [PATCH] Improve locking of multicast addresses in VLAN and LAGG interfaces. This fixes several scenarios of reproducible panics, cause by races between multicast address changes and interface destruction. MFC after: 2 weeks --- sys/net/if_lagg.c | 34 +++++++++++++++++++--------------- sys/net/if_lagg.h | 1 + sys/net/if_vlan.c | 31 ++++++++++++++++++------------- 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/sys/net/if_lagg.c b/sys/net/if_lagg.c index ec868e127160..cc92acfcaf24 100644 --- a/sys/net/if_lagg.c +++ b/sys/net/if_lagg.c @@ -1217,35 +1217,39 @@ lagg_ether_cmdmulti(struct lagg_port *lp, int set) struct ifnet *ifp = lp->lp_ifp; struct ifnet *scifp = sc->sc_ifp; struct lagg_mc *mc; - struct ifmultiaddr *ifma, *rifma = NULL; - struct sockaddr_dl sdl; + struct ifmultiaddr *ifma; int error; LAGG_WLOCK_ASSERT(sc); - link_init_sdl(ifp, (struct sockaddr *)&sdl, IFT_ETHER); - sdl.sdl_alen = ETHER_ADDR_LEN; - if (set) { + IF_ADDR_WLOCK(scifp); TAILQ_FOREACH(ifma, &scifp->if_multiaddrs, ifma_link) { if (ifma->ifma_addr->sa_family != AF_LINK) continue; - bcopy(LLADDR((struct sockaddr_dl *)ifma->ifma_addr), - LLADDR(&sdl), ETHER_ADDR_LEN); - - error = if_addmulti(ifp, (struct sockaddr *)&sdl, &rifma); + mc = malloc(sizeof(struct lagg_mc), M_DEVBUF, M_NOWAIT); + if (mc == NULL) { + IF_ADDR_WUNLOCK(scifp); + return (ENOMEM); + } + bcopy(ifma->ifma_addr, &mc->mc_addr, + ifma->ifma_addr->sa_len); + mc->mc_addr.sdl_index = ifp->if_index; + mc->mc_ifma = NULL; + SLIST_INSERT_HEAD(&lp->lp_mc_head, mc, mc_entries); + } + IF_ADDR_WUNLOCK(scifp); + SLIST_FOREACH (mc, &lp->lp_mc_head, mc_entries) { + error = if_addmulti(ifp, + (struct sockaddr *)&mc->mc_addr, &mc->mc_ifma); if (error) return (error); - mc = malloc(sizeof(struct lagg_mc), M_DEVBUF, M_NOWAIT); - if (mc == NULL) - return (ENOMEM); - mc->mc_ifma = rifma; - SLIST_INSERT_HEAD(&lp->lp_mc_head, mc, mc_entries); } } else { while ((mc = SLIST_FIRST(&lp->lp_mc_head)) != NULL) { SLIST_REMOVE(&lp->lp_mc_head, mc, lagg_mc, mc_entries); - if_delmulti_ifma(mc->mc_ifma); + if (mc->mc_ifma && !lp->lp_detaching) + if_delmulti_ifma(mc->mc_ifma); free(mc, M_DEVBUF); } } diff --git a/sys/net/if_lagg.h b/sys/net/if_lagg.h index e52f9014ca1e..4a1d9a0a77cb 100644 --- a/sys/net/if_lagg.h +++ b/sys/net/if_lagg.h @@ -174,6 +174,7 @@ struct lagg_lb { }; struct lagg_mc { + struct sockaddr_dl mc_addr; struct ifmultiaddr *mc_ifma; SLIST_ENTRY(lagg_mc) mc_entries; }; diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c index 00d99ed777bf..763020b2b050 100644 --- a/sys/net/if_vlan.c +++ b/sys/net/if_vlan.c @@ -460,48 +460,48 @@ trunk_destroy(struct ifvlantrunk *trunk) * traffic that it doesn't really want, which ends up being discarded * later by the upper protocol layers. Unfortunately, there's no way * to avoid this: there really is only one physical interface. - * - * XXX: There is a possible race here if more than one thread is - * modifying the multicast state of the vlan interface at the same time. */ static int vlan_setmulti(struct ifnet *ifp) { struct ifnet *ifp_p; - struct ifmultiaddr *ifma, *rifma = NULL; + struct ifmultiaddr *ifma; struct ifvlan *sc; struct vlan_mc_entry *mc; int error; - /*VLAN_LOCK_ASSERT();*/ - /* Find the parent. */ sc = ifp->if_softc; + TRUNK_LOCK_ASSERT(TRUNK(sc)); ifp_p = PARENT(sc); CURVNET_SET_QUIET(ifp_p->if_vnet); /* First, remove any existing filter entries. */ while ((mc = SLIST_FIRST(&sc->vlan_mc_listhead)) != NULL) { - error = if_delmulti(ifp_p, (struct sockaddr *)&mc->mc_addr); - if (error) - return (error); SLIST_REMOVE_HEAD(&sc->vlan_mc_listhead, mc_entries); + (void)if_delmulti(ifp_p, (struct sockaddr *)&mc->mc_addr); free(mc, M_VLAN); } /* Now program new ones. */ + IF_ADDR_WLOCK(ifp); TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { if (ifma->ifma_addr->sa_family != AF_LINK) continue; mc = malloc(sizeof(struct vlan_mc_entry), M_VLAN, M_NOWAIT); - if (mc == NULL) + if (mc == NULL) { + IF_ADDR_WUNLOCK(ifp); return (ENOMEM); + } bcopy(ifma->ifma_addr, &mc->mc_addr, ifma->ifma_addr->sa_len); mc->mc_addr.sdl_index = ifp_p->if_index; SLIST_INSERT_HEAD(&sc->vlan_mc_listhead, mc, mc_entries); + } + IF_ADDR_WUNLOCK(ifp); + SLIST_FOREACH (mc, &sc->vlan_mc_listhead, mc_entries) { error = if_addmulti(ifp_p, (struct sockaddr *)&mc->mc_addr, - &rifma); + NULL); if (error) return (error); } @@ -1374,7 +1374,7 @@ vlan_unconfig_locked(struct ifnet *ifp, int departing) * Check if we were the last. */ if (trunk->refcnt == 0) { - trunk->parent->if_vlantrunk = NULL; + parent->if_vlantrunk = NULL; /* * XXXGL: If some ithread has already entered * vlan_input() and is now blocked on the trunk @@ -1568,6 +1568,7 @@ vlan_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) struct ifreq *ifr; struct ifaddr *ifa; struct ifvlan *ifv; + struct ifvlantrunk *trunk; struct vlanreq vlr; int error = 0; @@ -1712,8 +1713,12 @@ vlan_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) * If we don't have a parent, just remember the membership for * when we do. */ - if (TRUNK(ifv) != NULL) + trunk = TRUNK(ifv); + if (trunk != NULL) { + TRUNK_LOCK(trunk); error = vlan_setmulti(ifp); + TRUNK_UNLOCK(trunk); + } break; default: