When a vlan interface is torn down in vlan_unconfig(), it removes references
to any multicast addresses from the parent device. If any of those attempts
fail, then vlan_unconfig() fails and doesn't tear down the device. However,
when a "normal" ifnet is detached, it destroys all its multicast addresses in
if_detach() before it invokes the ifnet_departure eventhandler. This means
that when the vlan eventhandler tries to call vlan_unconfig(), it will fail
if multicast has ever been used on the vlan interface as the attempts to
release a reference on the multicast address on the parent interface will
fail with ENOENT. However, the code does not expect vlan_unconfig() to ever
fail, and in fact it will loop forever here:
restart:
for (i = 0; i < (1 << ifp->if_vlantrunk->hwidth); i++)
if ((ifv = LIST_FIRST(&ifp->if_vlantrunk->hash[i]))) {
vlan_unconfig_locked(ifv->ifv_ifp);
if (ifp->if_vlantrunk)
goto restart; /* trunk->hwidth can change */
else
break;
}
due to the 'goto restart'.
The fix I came up with was to make vlan_unconfig() simply ignore errors from
removing a multicast reference from the parent device and always succeed. I
think this is probably more robust anyway. None of the callers of
vlan_unconfig() ever check the return value to handle failure anyway. You
can trigger this hang by kldunload'ing a network driver where at least one
instance has a sub-interface with a multicast address registered. Thoughts?
Index: if_vlan.c
===================================================================
--- if_vlan.c (revision 207329)
+++ if_vlan.c (working copy)
@@ -187,8 +187,8 @@
int (*func)(struct ifnet *, int));
static int vlan_setflags(struct ifnet *ifp, int status);
static int vlan_setmulti(struct ifnet *ifp);
-static int vlan_unconfig(struct ifnet *ifp);
-static int vlan_unconfig_locked(struct ifnet *ifp);
+static void vlan_unconfig(struct ifnet *ifp);
+static void vlan_unconfig_locked(struct ifnet *ifp);
static int vlan_config(struct ifvlan *ifv, struct ifnet *p, uint16_t tag);
static void vlan_link_state(struct ifnet *ifp);
static void vlan_capabilities(struct ifvlan *ifv);
@@ -1128,25 +1128,22 @@
return (error);
}
-static int
+static void
vlan_unconfig(struct ifnet *ifp)
{
- int ret;
VLAN_LOCK();
- ret = vlan_unconfig_locked(ifp);
+ vlan_unconfig_locked(ifp);
VLAN_UNLOCK();
- return (ret);
}
-static int
+static void
vlan_unconfig_locked(struct ifnet *ifp)
{
struct ifvlantrunk *trunk;
struct vlan_mc_entry *mc;
struct ifvlan *ifv;
struct ifnet *parent;
- int error;
VLAN_LOCK_ASSERT();
@@ -1175,9 +1172,15 @@
while ((mc = SLIST_FIRST(&ifv->vlan_mc_listhead)) != NULL) {
bcopy((char *)&mc->mc_addr, LLADDR(&sdl),
ETHER_ADDR_LEN);
- error = if_delmulti(parent, (struct sockaddr *)&sdl);
- if (error)
- return (error);
+
+ /*
+ * This may fail if the parent interface is
+ * being detached. Regardless, we should do a
+ * best effort to free this interface as much
+ * as possible as all callers expect vlan
+ * destruction to succeed.
+ */
+ (void)if_delmulti(parent, (struct sockaddr *)&sdl);
SLIST_REMOVE_HEAD(&ifv->vlan_mc_listhead, mc_entries);
free(mc, M_VLAN);
}
@@ -1223,8 +1226,6 @@
*/
if (parent != NULL)
EVENTHANDLER_INVOKE(vlan_unconfig, parent, ifv->ifv_tag);
-
- return (0);
}
/* Handle a reference counted flag that should be set on the parent as well */
--
John Baldwin
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
