Fix deadlock in vlan detach

[John Baldwin]

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