Fix several races between socket closure and node/hook

destruction:
  - Backout 1.62, since it doesn't fix all possible
    problems.
  - Upon node creation, put an additional reference on node.
  - Add a mutex and refcounter to struct ngsock. Netgraph node,
    control socket and data socket all count as references.
  - Introduce ng_socket_free_priv() which removes one reference
    from ngsock, and frees it when all references has gone.
  - No direct pointers between pcbs and node, all pointing
    is done via struct ngsock and protected with mutex.

1 files changed, 97 insertions, 77 deletions

diff --git a/sys/netgraph/ng_socket.c b/sys/netgraph/ng_socket.c
index 2326aa315153..e3399c50781d 100644
--- a/sys/netgraph/ng_socket.c
+++ b/sys/netgraph/ng_socket.c
@@ -122,7 +122,8 @@ static ng_disconnect_t	ngs_disconnect;
 static int	ng_attach_data(struct socket *so);
 static int	ng_attach_cntl(struct socket *so);
 static int	ng_attach_common(struct socket *so, int type);
-static void	ng_detach_common(node_p node, hook_p hook, void *arg1, int which);
+static void	ng_detach_common(struct ngpcb *pcbp, int type);
+static void	ng_socket_free_priv(struct ngsock *priv);
 /*static int	ng_internalize(struct mbuf *m, struct thread *p); */
 
 static int	ng_connect_data(struct sockaddr *nam, struct ngpcb *pcbp);
@@ -191,16 +192,7 @@ ngc_detach(struct socket *so)
 
 	if (pcbp == NULL)
 		return (EINVAL);
-
-	/*
-	 * If there is a node, then obtain netgraph locking first.
-	 */
-	if (pcbp->sockdata != NULL)
-		ng_send_fn1(pcbp->sockdata->node, NULL, &ng_detach_common,
-		    pcbp, NG_CONTROL, NG_WAITOK);
-	else
-		ng_detach_common(NULL, NULL, pcbp, NG_CONTROL);
-
+	ng_detach_common(pcbp, NG_CONTROL);
 	return (0);
 }
 
@@ -410,16 +402,7 @@ ngd_detach(struct socket *so)
 
 	if (pcbp == NULL)
 		return (EINVAL);
-
-	/*
-	 * If there is a node, then obtain netgraph locking first.
-	 */
-	if (pcbp->sockdata != NULL)
-		ng_send_fn1(pcbp->sockdata->node, NULL, &ng_detach_common,
-		    pcbp, NG_DATA, NG_WAITOK);
-	else
-		ng_detach_common(NULL, NULL, pcbp, NG_DATA);
-
+	ng_detach_common(pcbp, NG_DATA);
 	return (0);
 }
 
@@ -514,33 +497,40 @@ ng_setsockaddr(struct socket *so, struct sockaddr **addr)
 {
 	struct ngpcb *pcbp;
 	struct sockaddr_ng *sg;
-	int sg_len, namelen, s;
+	int sg_len;
+	int error = 0;
 
 	/* Why isn't sg_data a `char[1]' ? :-( */
 	sg_len = sizeof(struct sockaddr_ng) - sizeof(sg->sg_data) + 1;
 
-	s = splnet();
 	pcbp = sotongpcb(so);
-	if ((pcbp == NULL) || (pcbp->sockdata == NULL)) {
-		splx(s);
+	if ((pcbp == NULL) || (pcbp->sockdata == NULL))
+		/* XXXGL: can this still happen? */
 		return (EINVAL);
-	}
 
-	namelen = 0;		/* silence compiler ! */
-	if ( NG_NODE_HAS_NAME(pcbp->sockdata->node))
-		sg_len += namelen = strlen(NG_NODE_NAME(pcbp->sockdata->node));
+	mtx_lock(&pcbp->sockdata->mtx);
+	if (pcbp->sockdata->node != NULL) {
+		node_p node = pcbp->sockdata->node;
+		int namelen = 0;	/* silence compiler! */
 
-	MALLOC(sg, struct sockaddr_ng *, sg_len, M_SONAME, M_WAITOK | M_ZERO);
+		if (NG_NODE_HAS_NAME(node))
+			sg_len += namelen = strlen(NG_NODE_NAME(node));
 
-	if (NG_NODE_HAS_NAME(pcbp->sockdata->node))
-		bcopy(NG_NODE_NAME(pcbp->sockdata->node), sg->sg_data, namelen);
-	splx(s);
+		sg = malloc(sg_len, M_SONAME, M_WAITOK | M_ZERO);
 
-	sg->sg_len = sg_len;
-	sg->sg_family = AF_NETGRAPH;
-	*addr = (struct sockaddr *)sg;
+		if (NG_NODE_HAS_NAME(node))
+			bcopy(NG_NODE_NAME(node), sg->sg_data, namelen);
 
-	return (0);
+		sg->sg_len = sg_len;
+		sg->sg_family = AF_NETGRAPH;
+		*addr = (struct sockaddr *)sg;
+		mtx_unlock(&pcbp->sockdata->mtx);
+	} else {
+		mtx_unlock(&pcbp->sockdata->mtx);
+		error = EINVAL;
+	}
+
+	return (error);
 }
 
 /*
@@ -552,37 +542,43 @@ ng_setsockaddr(struct socket *so, struct sockaddr **addr)
 static int
 ng_attach_cntl(struct socket *so)
 {
-	struct ngsock *privdata;
+	struct ngsock *priv;
 	struct ngpcb *pcbp;
 	int error;
 
+	/* Allocate node private info */
+	MALLOC(priv, struct ngsock *,
+	    sizeof(*priv), M_NETGRAPH_SOCK, M_WAITOK | M_ZERO);
+	if (priv == NULL)
+		return (ENOMEM);
+
 	/* Setup protocol control block */
-	if ((error = ng_attach_common(so, NG_CONTROL)) != 0)
+	if ((error = ng_attach_common(so, NG_CONTROL)) != 0) {
+		FREE(priv, M_NETGRAPH_SOCK);
 		return (error);
+	}
 	pcbp = sotongpcb(so);
 
-	/* Allocate node private info */
-	MALLOC(privdata, struct ngsock *,
-	    sizeof(*privdata), M_NETGRAPH_SOCK, M_WAITOK | M_ZERO);
-	if (privdata == NULL) {
-		ng_detach_common(NULL, NULL, pcbp, NG_CONTROL);
-		return (ENOMEM);
-	}
+	/* Link the pcb the private data. */
+	priv->ctlsock = pcbp;
+	pcbp->sockdata = priv;
+	priv->refs++;
+
+	/* Initialize mutex. */
+	mtx_init(&priv->mtx, "ng_socket", NULL, MTX_DEF);
 
 	/* Make the generic node components */
-	if ((error = ng_make_node_common(&typestruct, &privdata->node)) != 0) {
-		FREE(privdata, M_NETGRAPH_SOCK);
-		ng_detach_common(NULL, NULL, pcbp, NG_CONTROL);
+	if ((error = ng_make_node_common(&typestruct, &priv->node)) != 0) {
+		FREE(priv, M_NETGRAPH_SOCK);
+		ng_detach_common(pcbp, NG_CONTROL);
 		return (error);
 	}
-	NG_NODE_SET_PRIVATE(privdata->node, privdata);
 
-	mtx_init(&privdata->mtx, "ng_socket", NULL, MTX_DEF);
+	/* Link the node and the private data. */
+	NG_NODE_SET_PRIVATE(priv->node, priv);
+	NG_NODE_REF(priv->node);
+	priv->refs++;
 
-	/* Link the pcb and the node private data */
-	privdata->ctlsock = pcbp;
-	pcbp->sockdata = privdata;
-	privdata->refs++;
 	return (0);
 }
 
@@ -631,15 +627,13 @@ ng_attach_common(struct socket *so, int type)
  * then shut down the entire node. Shared code for control and data sockets.
  */
 static void
-ng_detach_common(node_p node, hook_p hook, void *arg1, int which)
+ng_detach_common(struct ngpcb *pcbp, int which)
 {
-	struct ngpcb *pcbp = arg1;
+	struct ngsock *priv = pcbp->sockdata;
 
-	if (pcbp->sockdata) {
-		struct ngsock *priv;
+	if (priv != NULL) {
+		mtx_lock(&priv->mtx);
 
-		priv = pcbp->sockdata;
-		pcbp->sockdata = NULL;
 		switch (which) {
 		case NG_CONTROL:
 			priv->ctlsock = NULL;
@@ -650,17 +644,45 @@ ng_detach_common(node_p node, hook_p hook, void *arg1, int which)
 		default:
 			panic(__func__);
 		}
-		if ((--priv->refs == 0) && (priv->node != NULL))
-			ng_rmnode_self(priv->node);
+		pcbp->sockdata = NULL;
+
+		ng_socket_free_priv(priv);
 	}
+
 	pcbp->ng_socket->so_pcb = NULL;
-	pcbp->ng_socket = NULL;
 	mtx_lock(&ngsocketlist_mtx);
 	LIST_REMOVE(pcbp, socks);
 	mtx_unlock(&ngsocketlist_mtx);
 	FREE(pcbp, M_PCB);
 }
 
+/*
+ * Remove a reference from node private data.
+ */
+static void
+ng_socket_free_priv(struct ngsock *priv)
+{
+	mtx_assert(&priv->mtx, MA_OWNED);
+
+	priv->refs--;
+
+	if (priv->refs == 0) {
+		mtx_destroy(&priv->mtx);
+		FREE(priv, M_NETGRAPH_SOCK);
+		return;
+	}
+
+	if ((priv->refs == 1) && (priv->node != NULL)) {
+		node_p node = priv->node;
+
+		priv->node = NULL;
+		mtx_unlock(&priv->mtx);
+		NG_NODE_UNREF(node);
+		ng_rmnode_self(node);
+	} else
+		mtx_unlock(&priv->mtx);
+}
+
 #ifdef NOTYET
 /*
  * File descriptors can be passed into an AF_NETGRAPH socket.
@@ -774,9 +796,11 @@ ng_connect_data(struct sockaddr *nam, struct ngpcb *pcbp)
 	 * Link the PCB and the private data struct. and note the extra
 	 * reference. Drop the extra reference on the node.
 	 */
+	mtx_lock(&priv->mtx);
 	priv->datasock = pcbp;
 	pcbp->sockdata = priv;
-	priv->refs++; /* XXX possible race if it's being freed */
+	priv->refs++;
+	mtx_unlock(&priv->mtx);
 	NG_FREE_ITEM(item);	/* drop the reference to the node */
 	return (0);
 }
@@ -1037,22 +1061,18 @@ ngs_shutdown(node_p node)
 	struct ngpcb *const dpcbp = priv->datasock;
 	struct ngpcb *const pcbp = priv->ctlsock;
 
-	if (dpcbp != NULL) {
+	if (dpcbp != NULL)
 		soisdisconnected(dpcbp->ng_socket);
-		dpcbp->sockdata = NULL;
-		priv->datasock = NULL;
-		priv->refs--;
-	}
-	if (pcbp != NULL) {
+
+	if (pcbp != NULL)
 		soisdisconnected(pcbp->ng_socket);
-		pcbp->sockdata = NULL;
-		priv->ctlsock = NULL;
-		priv->refs--;
-	}
+
+	mtx_lock(&priv->mtx);
+	priv->node = NULL;
 	NG_NODE_SET_PRIVATE(node, NULL);
+	ng_socket_free_priv(priv);
+
 	NG_NODE_UNREF(node);
-	mtx_destroy(&priv->mtx);
-	FREE(priv, M_NETGRAPH_SOCK);
 	return (0);
 }