Rework locking, that I have introduced recently, since it was incorrect:

  First, mutexed callouts are incompatible with netgraph nodes, because
  netgraph(4) can guarantee that the function will be called with mutex
  held.

  Second, nodes should not send data to their neighbor holding their
  mutex. A node does not know what stack can it enter sending data in
  some direction. May be executing will encounter a place to sleep.

New locking:

  - ng_pptpgre_recv() and ng_pptpgre_xmit() must be entered with mutex held.
  - ng_pptpgre_recv() and ng_pptpgre_xmit() unlock mutex before
    sending data and then return unlocked.
  - callout routines acquire mutex themselves.

1 files changed, 54 insertions, 35 deletions

diff --git a/sys/netgraph/ng_pptpgre.c b/sys/netgraph/ng_pptpgre.c
index 2f57ce6..4c39256 100644
--- a/sys/netgraph/ng_pptpgre.c
+++ b/sys/netgraph/ng_pptpgre.c
@@ -286,8 +286,8 @@ ng_pptpgre_constructor(node_p node)
 
 	/* Initialize state */
 	mtx_init(&priv->mtx, "ng_pptp", NULL, MTX_DEF);
-	ng_callout_init_mtx(&priv->ackp.sackTimer, &priv->mtx);
-	ng_callout_init_mtx(&priv->ackp.rackTimer, &priv->mtx);
+	ng_callout_init(&priv->ackp.sackTimer);
+	ng_callout_init(&priv->ackp.rackTimer);
 
 	/* Done */
 	return (0);
@@ -409,7 +409,7 @@ ng_pptpgre_rcvdata(hook_p hook, item_p item)
 	else
 		panic("%s: weird hook", __func__);
 
-	mtx_unlock(&priv->mtx);
+	mtx_assert(&priv->mtx, MA_NOTOWNED);
 
 	return (rval);
 }
@@ -475,6 +475,8 @@ ng_pptpgre_xmit(node_p node, item_p item)
 	int grelen, error;
 	struct mbuf *m;
 
+	mtx_assert(&priv->mtx, MA_OWNED);
+
 	if (item) {
 		NGI_GET_M(item, m);
 	} else {
@@ -489,18 +491,14 @@ ng_pptpgre_xmit(node_p node, item_p item)
 			if ((u_int32_t)PPTP_SEQ_DIFF(priv->xmitSeq,
 			    priv->recvAck) >= a->xmitWin) {
 				priv->stats.xmitDrops++;
-				NG_FREE_M(m);
-				NG_FREE_ITEM(item);
-				return (ENOBUFS);
+				ERROUT(ENOBUFS);
 			}
 		}
 
 		/* Sanity check frame length */
 		if (m != NULL && m->m_pkthdr.len > PPTP_MAX_PAYLOAD) {
 			priv->stats.xmitTooBig++;
-			NG_FREE_M(m);
-			NG_FREE_ITEM(item);
-			return (EMSGSIZE);
+			ERROUT(EMSGSIZE);
 		}
 	} else {
 		priv->stats.xmitLoneAcks++;
@@ -536,9 +534,7 @@ ng_pptpgre_xmit(node_p node, item_p item)
 		MGETHDR(m, M_DONTWAIT, MT_DATA);
 		if (m == NULL) {
 			priv->stats.memoryFailures++;
-			if (item)
-				NG_FREE_ITEM(item);
-			return (ENOBUFS);
+			ERROUT(ENOBUFS);
 		}
 		m->m_len = m->m_pkthdr.len = grelen;
 		m->m_pkthdr.rcvif = NULL;
@@ -547,9 +543,7 @@ ng_pptpgre_xmit(node_p node, item_p item)
 		if (m == NULL || (m->m_len < grelen
 		    && (m = m_pullup(m, grelen)) == NULL)) {
 			priv->stats.memoryFailures++;
-			if (item)
-				NG_FREE_ITEM(item);
-			return (ENOBUFS);
+			ERROUT(ENOBUFS);
 		}
 	}
 	bcopy(gre, mtod(m, u_char *), grelen);
@@ -558,6 +552,15 @@ ng_pptpgre_xmit(node_p node, item_p item)
 	priv->stats.xmitPackets++;
 	priv->stats.xmitOctets += m->m_pkthdr.len;
 
+	/*
+	 * XXX: we should reset timer only after an item has been sent
+	 * successfully.
+	 */
+	if (gre->hasSeq && priv->xmitSeq == priv->recvAck + 1)
+		ng_pptpgre_start_recv_ack_timer(node);
+
+	mtx_unlock(&priv->mtx);
+
 	/* Deliver packet */
 	if (item) {
 		NG_FWD_NEW_DATA(error, item, priv->lower, m);
@@ -565,10 +568,13 @@ ng_pptpgre_xmit(node_p node, item_p item)
 		NG_SEND_DATA_ONLY(error, priv->lower, m);
 	}
 
+	return (error);
 
-	/* Start receive ACK timer if data was sent and not already running */
-	if (error == 0 && gre->hasSeq && priv->xmitSeq == priv->recvAck + 1)
-		ng_pptpgre_start_recv_ack_timer(node);
+done:
+	mtx_unlock(&priv->mtx);
+	NG_FREE_M(m);
+	if (item)
+		NG_FREE_ITEM(item);
 	return (error);
 }
 
@@ -585,6 +591,8 @@ ng_pptpgre_recv(node_p node, item_p item)
 	int error = 0;
 	struct mbuf *m;
 
+	mtx_assert(&priv->mtx, MA_OWNED);
+
 	NGI_GET_M(item, m);
 	/* Update stats */
 	priv->stats.recvPackets++;
@@ -593,26 +601,21 @@ ng_pptpgre_recv(node_p node, item_p item)
 	/* Sanity check packet length */
 	if (m->m_pkthdr.len < sizeof(*ip) + sizeof(*gre)) {
 		priv->stats.recvRunts++;
-bad:
-		NG_FREE_M(m);
-		NG_FREE_ITEM(item);
-		return (EINVAL);
+		ERROUT(EINVAL);
 	}
 
 	/* Safely pull up the complete IP+GRE headers */
 	if (m->m_len < sizeof(*ip) + sizeof(*gre)
 	    && (m = m_pullup(m, sizeof(*ip) + sizeof(*gre))) == NULL) {
 		priv->stats.memoryFailures++;
-		NG_FREE_ITEM(item);
-		return (ENOBUFS);
+		ERROUT(ENOBUFS);
 	}
 	ip = mtod(m, const struct ip *);
 	iphlen = ip->ip_hl << 2;
 	if (m->m_len < iphlen + sizeof(*gre)) {
 		if ((m = m_pullup(m, iphlen + sizeof(*gre))) == NULL) {
 			priv->stats.memoryFailures++;
-			NG_FREE_ITEM(item);
-			return (ENOBUFS);
+			ERROUT(ENOBUFS);
 		}
 		ip = mtod(m, const struct ip *);
 	}
@@ -620,13 +623,12 @@ bad:
 	grelen = sizeof(*gre) + sizeof(u_int32_t) * (gre->hasSeq + gre->hasAck);
 	if (m->m_pkthdr.len < iphlen + grelen) {
 		priv->stats.recvRunts++;
-		goto bad;
+		ERROUT(EINVAL);
 	}
 	if (m->m_len < iphlen + grelen) {
 		if ((m = m_pullup(m, iphlen + grelen)) == NULL) {
 			priv->stats.memoryFailures++;
-			NG_FREE_ITEM(item);
-			return (ENOBUFS);
+			ERROUT(ENOBUFS);
 		}
 		ip = mtod(m, const struct ip *);
 		gre = (const struct greheader *)((const u_char *)ip + iphlen);
@@ -637,16 +639,16 @@ bad:
 	    - (iphlen + grelen + gre->hasSeq * (u_int16_t)ntohs(gre->length));
 	if (extralen < 0) {
 		priv->stats.recvBadGRE++;
-		goto bad;
+		ERROUT(EINVAL);
 	}
 	if ((ntohl(*((const u_int32_t *)gre)) & PPTP_INIT_MASK)
 	    != PPTP_INIT_VALUE) {
 		priv->stats.recvBadGRE++;
-		goto bad;
+		ERROUT(EINVAL);
 	}
 	if (ntohs(gre->cid) != priv->conf.cid) {
 		priv->stats.recvBadCID++;
-		goto bad;
+		ERROUT(EINVAL);
 	}
 
 	/* Look for peer ack */
@@ -711,7 +713,7 @@ badAck:
 				priv->stats.recvDuplicates++;
 			else
 				priv->stats.recvOutOfOrder++;
-			goto bad;		/* out-of-order or dup */
+			ERROUT(EINVAL);
 		}
 		priv->recvSeq = seq;
 
@@ -723,9 +725,10 @@ badAck:
 			maxWait = (a->rtt >> 2);
 
 			/* If delayed ACK is disabled, send it now */
-			if (!priv->conf.enableDelayedAck)	/* ack now */
+			if (!priv->conf.enableDelayedAck) {	/* ack now */
 				ng_pptpgre_xmit(node, NULL);
-			else {					/* ack later */
+				mtx_lock(&priv->mtx);
+			} else {				/* ack later */
 				if (maxWait < PPTP_MIN_ACK_DELAY)
 					maxWait = PPTP_MIN_ACK_DELAY;
 				if (maxWait > PPTP_MAX_ACK_DELAY)
@@ -739,13 +742,22 @@ badAck:
 		if (extralen > 0)
 			m_adj(m, -extralen);
 
+		mtx_unlock(&priv->mtx);
 		/* Deliver frame to upper layers */
 		NG_FWD_NEW_DATA(error, item, priv->upper, m);
 	} else {
 		priv->stats.recvLoneAcks++;
+		mtx_unlock(&priv->mtx);
 		NG_FREE_ITEM(item);
 		NG_FREE_M(m);		/* no data to deliver */
 	}
+
+	return (error);
+
+done:
+	mtx_unlock(&priv->mtx);
+	NG_FREE_ITEM(item);
+	NG_FREE_M(m);
 	return (error);
 }
 
@@ -811,6 +823,7 @@ ng_pptpgre_recv_ack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
 	const priv_p priv = NG_NODE_PRIVATE(node);
 	struct ng_pptpgre_ackp *const a = &priv->ackp;
 
+	mtx_lock(&priv->mtx);
 
 	/* Update adaptive timeout stuff */
 	priv->stats.recvAckTimeouts++;
@@ -832,6 +845,8 @@ ng_pptpgre_recv_ack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
 	priv->recvAck = priv->xmitSeq;		/* pretend we got the ack */
 	a->xmitWin = (a->xmitWin + 1) / 2;	/* shrink transmit window */
 	a->winAck = priv->recvAck + a->xmitWin;	/* reset win expand time */
+
+	mtx_unlock(&priv->mtx);
 }
 
 /*
@@ -872,8 +887,12 @@ ng_pptpgre_stop_send_ack_timer(node_p node)
 static void
 ng_pptpgre_send_ack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
 {
+	const priv_p priv = NG_NODE_PRIVATE(node);
+
+	mtx_lock(&priv->mtx);
 	/* Send a frame with an ack but no payload */
   	ng_pptpgre_xmit(node, NULL);
+	mtx_assert(&priv->mtx, MA_NOTOWNED);
 }
 
 /*************************************************************************