⬢ DragonFlyBSD Kernel Audit
DF-3486 / fix.diff
← back to finding ↓ download raw
--- a/sys/netgraph/eiface/ng_eiface.c
+++ b/sys/netgraph/eiface/ng_eiface.c
@@ -41,6 +41,8 @@
 #include <sys/syslog.h>
 #include <sys/serialize.h>
 #include <sys/thread2.h>
+#include <sys/spinlock2.h>
+#include <sys/kthread.h>
 
 #include <net/if.h>
 #include <net/if_types.h>
@@ -84,6 +86,9 @@
 	node_p	node;			/* Our netgraph node */
 	hook_p	ether;			/* Hook for ethernet stream */
 	struct	private *next;		/* When hung on the free list */
+	int	attach_state;		/* async ether_ifattach state */
+	int	down_pending;		/* shutdown seen while attaching */
+	struct	spinlock attach_spin;	/* protects attach_state/down_pending */
 };
 typedef struct ng_eiface_private *priv_p;
 
@@ -307,6 +312,55 @@
  ************************************************************************/
 
 /*
+ * Deferred interface attachment.
+ *
+ * Netgraph node constructors run in the control-message path, i.e. on a
+ * TD_TYPE_NETISR thread (the socket protocol thread).  if_attach() may
+ * not run in netisr context: it takes the ifnet lock, which asserts
+ * against netisr, and it calls netmsg_service_sync().  So the
+ * ether_ifattach() is executed from a short-lived kernel thread.
+ */
+#define EIFACE_ATTACH_PENDING	0	/* thread not started yet */
+#define EIFACE_ATTACH_RUNNING	1	/* thread owns the ifnet */
+#define EIFACE_ATTACH_DONE	2	/* ifnet attached */
+#define EIFACE_ATTACH_CANCELLED	3	/* node died before attach */
+
+static void
+ng_eiface_attach_thread(void *xpriv)
+{
+	priv_p priv = xpriv;
+	int attach, down;
+
+	spin_lock(&priv->attach_spin);
+	attach = (priv->attach_state == EIFACE_ATTACH_PENDING);
+	if (attach)
+		priv->attach_state = EIFACE_ATTACH_RUNNING;
+	spin_unlock(&priv->attach_spin);
+
+	if (attach)
+		ether_ifattach(priv->ifp, priv->arpcom.ac_enaddr, NULL);
+
+	spin_lock(&priv->attach_spin);
+	down = attach && priv->down_pending;
+	if (attach)
+		priv->attach_state = EIFACE_ATTACH_DONE;
+	spin_unlock(&priv->attach_spin);
+
+	/*
+	 * If the node was shut down while we were attaching, bring the
+	 * interface down here; ng_eiface_rmnode() could not do it safely
+	 * at that point.
+	 */
+	if (down) {
+		ifnet_serialize_all(priv->ifp);
+		priv->ifp->if_flags &= ~(IFF_UP | IFF_RUNNING);
+		ifq_clr_oactive(&priv->ifp->if_snd);
+		ifnet_deserialize_all(priv->ifp);
+	}
+	lwkt_exit();
+}
+
+/*
  * Constructor for a node
  */
 static int
@@ -352,8 +406,16 @@
 	ng_name_node(node, ifname);
 	*/
 
-	/* Attach the interface */
-	ether_ifattach(ifp, priv->arpcom.ac_enaddr, NULL);
+	/* Attach the interface (deferred; see ng_eiface_attach_thread) */
+	spin_init(&priv->attach_spin, "ng_eiface_attach");
+	priv->attach_state = EIFACE_ATTACH_PENDING;
+	if (kthread_create(ng_eiface_attach_thread, priv, NULL,
+			   "ng_eiface_attach") != 0) {
+		node->private = NULL;
+		ng_unref(node);
+		kfree(priv, M_NETGRAPH);
+		return (ENOMEM);
+	}
 
 	/* Done */
 	return (0);
@@ -539,13 +601,31 @@
 {
 	const priv_p priv = node->private;
 	struct ifnet *const ifp = priv->ifp;
+	int down_now;
 
 	ng_cutlinks(node);
 	node->flags &= ~NG_INVALID;
-	ifnet_serialize_all(ifp);
-	ifp->if_flags &= ~(IFF_UP | IFF_RUNNING);
-	ifq_clr_oactive(&ifp->if_snd);
-	ifnet_deserialize_all(ifp);
+
+	spin_lock(&priv->attach_spin);
+	if (priv->attach_state == EIFACE_ATTACH_PENDING) {
+		/* attach thread has not touched the ifnet yet */
+		priv->attach_state = EIFACE_ATTACH_CANCELLED;
+		down_now = 0;
+	} else if (priv->attach_state == EIFACE_ATTACH_RUNNING) {
+		/* defer the interface shutdown to the attach thread */
+		priv->down_pending = 1;
+		down_now = 0;
+	} else {
+		down_now = 1;
+	}
+	spin_unlock(&priv->attach_spin);
+
+	if (down_now) {
+		ifnet_serialize_all(ifp);
+		ifp->if_flags &= ~(IFF_UP | IFF_RUNNING);
+		ifq_clr_oactive(&ifp->if_snd);
+		ifnet_deserialize_all(ifp);
+	}
 	return (0);
 }
 
--- a/sys/netgraph/iface/ng_iface.c
+++ b/sys/netgraph/iface/ng_iface.c
@@ -65,6 +65,8 @@
 #include <sys/syslog.h>
 #include <sys/libkern.h>
 #include <sys/thread2.h>
+#include <sys/spinlock2.h>
+#include <sys/kthread.h>
 
 #include <net/if.h>
 #include <net/if_types.h>
@@ -102,6 +104,8 @@
 	int	unit;			/* Interface unit number */
 	node_p	node;			/* Our netgraph node */
 	hook_p	hooks[NUM_FAMILIES];	/* Hook for each address family */
+	int	attach_state;		/* async if_attach state */
+	struct	spinlock attach_spin;	/* protects attach_state */
 };
 typedef struct ng_iface_private *priv_p;
 
@@ -536,6 +540,77 @@
  ************************************************************************/
 
 /*
+ * Deferred interface attachment/detachment.
+ *
+ * Netgraph node constructors and shutdown methods run in the
+ * control-message path, i.e. on a TD_TYPE_NETISR thread (the socket
+ * protocol thread).  if_attach()/if_detach() may not run in netisr
+ * context: they take the ifnet lock, which asserts against netisr,
+ * and if_attach() calls netmsg_service_sync().  So the ifnet side of
+ * the node lifecycle is executed from short-lived kernel threads.
+ *
+ * The attach_state machine transfers ownership of the ifnet (and, on
+ * teardown, of priv itself) atomically between the control path and
+ * the helper threads.
+ */
+#define IFACE_ATTACH_PENDING	0	/* thread not started yet */
+#define IFACE_ATTACH_RUNNING	1	/* attach thread owns the ifnet */
+#define IFACE_ATTACH_DONE	2	/* ifnet attached; rmnode owns it */
+#define IFACE_ATTACH_CANCELLED	3	/* rmnode owns it; never attached */
+#define IFACE_ATTACH_DYING	4	/* attach thread owns it + teardown */
+#define IFACE_ATTACH_DETACHING	5	/* detach thread owns it + teardown */
+
+static void	ng_iface_attach_thread(void *);
+static void	ng_iface_detach_thread(void *);
+static void	ng_iface_teardown(priv_p priv);
+
+static void
+ng_iface_attach_thread(void *xpriv)
+{
+	priv_p priv = xpriv;
+	int attach;
+
+	spin_lock(&priv->attach_spin);
+	attach = (priv->attach_state == IFACE_ATTACH_PENDING);
+	if (attach)
+		priv->attach_state = IFACE_ATTACH_RUNNING;
+	spin_unlock(&priv->attach_spin);
+
+	if (attach) {
+		if_attach(priv->ifp, NULL);
+		bpfattach(priv->ifp, DLT_NULL, sizeof(uint32_t));
+
+		spin_lock(&priv->attach_spin);
+		if (priv->attach_state != IFACE_ATTACH_DYING)
+			priv->attach_state = IFACE_ATTACH_DONE;
+		attach = (priv->attach_state == IFACE_ATTACH_DYING);
+		spin_unlock(&priv->attach_spin);
+
+		if (attach)
+			ng_iface_teardown(priv);
+	}
+	lwkt_exit();
+}
+
+static void
+ng_iface_detach_thread(void *xpriv)
+{
+	ng_iface_teardown((priv_p)xpriv);
+	lwkt_exit();
+}
+
+static void
+ng_iface_teardown(priv_p priv)
+{
+	bpfdetach(priv->ifp);
+	if_detach(priv->ifp);
+	kfree(priv->ifp, M_NETGRAPH);
+	priv->ifp = NULL;
+	ng_iface_free_unit(priv->unit);
+	kfree(priv, M_NETGRAPH);
+}
+
+/*
  * Constructor for a node
  */
 static int
@@ -601,9 +676,20 @@
 	if (ng_name_node(node, ifname) != 0)
 		log(LOG_WARNING, "%s: can't acquire netgraph name\n", ifname);
 
-	/* Attach the interface */
-	if_attach(ifp, NULL);
-	bpfattach(ifp, DLT_NULL, sizeof(uint32_t));
+	/* Attach the interface (deferred; see ng_iface_attach_thread) */
+	spin_init(&priv->attach_spin, "ng_iface_attach");
+	priv->attach_state = IFACE_ATTACH_PENDING;
+	error = kthread_create(ng_iface_attach_thread, priv, NULL,
+			       "ng_iface_attach");
+	if (error) {
+		ng_unname(node);
+		node->private = NULL;
+		ng_unref(node);
+		ng_iface_free_unit(priv->unit);
+		kfree(ifp, M_NETGRAPH);
+		kfree(priv, M_NETGRAPH);
+		return (error);
+	}
 
 	/* Done */
 	return (0);
@@ -794,15 +880,47 @@
 ng_iface_rmnode(node_p node)
 {
 	const priv_p priv = node->private;
+	int own_ifp;
 
 	ng_cutlinks(node);
 	ng_unname(node);
-	bpfdetach(priv->ifp);
-	if_detach(priv->ifp);
-	kfree(priv->ifp, M_NETGRAPH);
-	priv->ifp = NULL;
-	ng_iface_free_unit(priv->unit);
-	kfree(priv, M_NETGRAPH);
+
+	spin_lock(&priv->attach_spin);
+	switch (priv->attach_state) {
+	case IFACE_ATTACH_PENDING:
+		/* never attached: inline cleanup is safe (no ifnet ops) */
+		priv->attach_state = IFACE_ATTACH_CANCELLED;
+		own_ifp = 1;
+		break;
+	case IFACE_ATTACH_RUNNING:
+		/* attach thread owns it and will do the teardown */
+		priv->attach_state = IFACE_ATTACH_DYING;
+		own_ifp = 0;
+		break;
+	case IFACE_ATTACH_DONE:
+		/*
+		 * Attached: if_detach() must not run from the netisr
+		 * control path; hand the teardown to a kernel thread.
+		 */
+		priv->attach_state = IFACE_ATTACH_DETACHING;
+		if (kthread_create(ng_iface_detach_thread, priv, NULL,
+				   "ng_iface_detach") == 0)
+			own_ifp = 0;
+		else
+			own_ifp = 1;	/* leak rather than panic */
+		break;
+	default:
+		own_ifp = 0;
+		break;
+	}
+	spin_unlock(&priv->attach_spin);
+
+	if (own_ifp) {
+		kfree(priv->ifp, M_NETGRAPH);
+		priv->ifp = NULL;
+		ng_iface_free_unit(priv->unit);
+		kfree(priv, M_NETGRAPH);
+	}
 	node->private = NULL;
 	ng_unref(node);
 	return (0);