netgraph.h:

  Change a prototype.
  Add a function version of ng_ref_node() when debugging so
  a breakpoint can be set on it.
ng_base.c:
  add 'node' as an argument to ng_apply_item so that it is up
  to the caller to take over and release the item's reference on
  the node. If the release reports back that the node went away
  due to the reference going to 0, the caller should cease referencing
  the now defunct node. (e.g. the item was a 'kill node' message).
  Alter ng_unref_node to report back the residual references as a result.
ng_pptpgre.c:
  Don't reference a node after we dropped a reference to it.
  (What if it was the last?)
Fixes a node leak reported by Harti Brandt <brandt@fokus.gmd.de>
 which was due to an incorrect earlier attempt to fix the
 "accessing node after dropping the last reference" problem.

3 files changed, 65 insertions, 24 deletions

diff --git a/sys/netgraph/netgraph.h b/sys/netgraph/netgraph.h
index e6b997e..27278b3 100644
--- a/sys/netgraph/netgraph.h
+++ b/sys/netgraph/netgraph.h
@@ -357,7 +357,7 @@ struct ng_node {
  * Public methods for nodes.
  * If you can't do it with these you probably shouldn't be doing it.
  */
-void	ng_unref_node(node_p node); /* don't move this */
+int	ng_unref_node(node_p node); /* don't move this */
 #define _NG_NODE_NAME(node)	((node)->nd_name + 0)
 #define _NG_NODE_HAS_NAME(node)	((node)->nd_name[0] + 0)
 #define _NG_NODE_ID(node)	((node)->nd_ID + 0)
@@ -396,8 +396,9 @@ static void __inline _chknode(node_p node, char *file, int line);
 static __inline char * _ng_node_name(node_p node, char *file, int line);
 static __inline int _ng_node_has_name(node_p node, char *file, int line);
 static __inline ng_ID_t _ng_node_id(node_p node, char *file, int line);
+void ng_ref_node(node_p node);
 static __inline void _ng_node_ref(node_p node, char *file, int line);
-static __inline void _ng_node_unref(node_p node, char *file, int line);
+static __inline int _ng_node_unref(node_p node, char *file, int line);
 static __inline void _ng_node_set_private(node_p node, void * val,
 							char *file, int line);
 static __inline void * _ng_node_private(node_p node, char *file, int line);
@@ -444,14 +445,15 @@ static __inline void
 _ng_node_ref(node_p node, char *file, int line)
 {
 	_chknode(node, file, line);
-	_NG_NODE_REF(node);
+	/*_NG_NODE_REF(node);*/
+	ng_ref_node(node);
 }
 
-static __inline void
+static __inline int
 _ng_node_unref(node_p node, char *file, int line)
 {
 	_chknode(node, file, line);
-	_NG_NODE_UNREF(node);
+	return (_NG_NODE_UNREF(node));
 }
 
 static __inline void
diff --git a/sys/netgraph/ng_base.c b/sys/netgraph/ng_base.c
index ed08373..a115df4 100644
--- a/sys/netgraph/ng_base.c
+++ b/sys/netgraph/ng_base.c
@@ -187,7 +187,7 @@ static ng_ID_t	ng_decodeidname(const char *name);
 static int	ngb_mod_event(module_t mod, int event, void *data);
 static void	ng_worklist_remove(node_p node);
 static void	ngintr(void);
-static int	ng_apply_item(item_p item);
+static int	ng_apply_item(node_p node, item_p item);
 static void	ng_flush_input_queue(struct ng_queue * ngq);
 static void	ng_setisr(node_p node);
 static node_p	ng_ID2noderef(ng_ID_t ID);
@@ -740,23 +740,32 @@ ng_rmnode(node_p node, hook_p dummy1, void *dummy2, int dummy3)
 	NG_NODE_UNREF(node);
 }
 
+#ifdef	NETGRAPH_DEBUG
+void
+ng_ref_node(node_p node)
+{
+	_NG_NODE_REF(node);
+}
+#endif
+
 /*
- * Remove a reference to the node, possibly the last
+ * Remove a reference to the node, possibly the last.
+ * deadnode always acts as it it were the last.
  */
-void
+int
 ng_unref_node(node_p node)
 {
 	int     v;
 
 	if (node == &ng_deadnode) {
-		return;
+		return (0);
 	}
 
 	do {
-		v = node->nd_refs;
-	} while (! atomic_cmpset_int(&node->nd_refs, v, v - 1));
+		v = node->nd_refs - 1;
+	} while (! atomic_cmpset_int(&node->nd_refs, v + 1, v));
 
-	if (v == 1) { /* we were the last */
+	if (v == 0) { /* we were the last */
 
 		mtx_lock(&ng_nodelist_mtx);
 		node->nd_type->refs--; /* XXX maybe should get types lock? */
@@ -770,6 +779,7 @@ ng_unref_node(node_p node)
 		mtx_destroy(&node->nd_input_queue.q_mtx);
 		NG_FREE_NODE(node);
 	}
+	return (v);
 }
 
 /************************************************************************
@@ -1265,7 +1275,7 @@ ng_con_part2(node_p node, hook_p hook, void *arg1, int arg2)
 	
 	/*
 	 * We now have a symetrical situation, where both hooks have been
-	 * linked to theur nodes, the newhook methods have been called
+	 * linked to their nodes, the newhook methods have been called
 	 * And the references are all correct. The hooks are still marked
 	 * as invalid, as we have not called the 'connect' methods
 	 * yet.
@@ -2214,7 +2224,13 @@ ng_snd_item(item_p item, int queue)
 #ifdef	NETGRAPH_DEBUG
         _ngi_check(item, __FILE__, __LINE__);
 #endif
-	ierror = ng_apply_item(item); /* drops r/w lock when done */
+	/*
+	 * Take over the reference frm the item.
+	 * Hold it until the called function returns.
+	 */
+	NGI_GET_NODE(item, node); /* zaps stored node */
+
+	ierror = ng_apply_item(node, item); /* drops r/w lock when done */
 
 	/* only return an error if it was our initial item.. (compat hack) */
 	if (oitem == item) {
@@ -2222,6 +2238,15 @@ ng_snd_item(item_p item, int queue)
 	}
 
 	/*
+	 * If the node goes away when we remove the reference, 
+	 * whatever we just did caused it.. hwatever we do, DO NOT
+	 * access the node again!
+	 */
+	if (NG_NODE_UNREF(node) == 0) {
+		return (error);
+	}
+
+	/*
 	 * Now we've handled the packet we brought, (or a friend of it) let's
 	 * look for any other packets that may have been queued up. We hold
 	 * no locks, so if someone puts something in the queue after
@@ -2244,10 +2269,17 @@ ng_snd_item(item_p item, int queue)
 		mtx_unlock_spin(&ngq->q_mtx);
 
 		/*
+		 * Take over the reference frm the item.
+		 * Hold it until the called function returns.
+		 */
+
+		NGI_GET_NODE(item, node); /* zaps stored node */
+
+		/*
 		 * We have the appropriate lock, so run the item.
 		 * When finished it will drop the lock accordingly
 		 */
-		ierror = ng_apply_item(item);
+		ierror = ng_apply_item(node, item);
 
 		/*
 		 * only return an error if it was our initial
@@ -2256,6 +2288,15 @@ ng_snd_item(item_p item, int queue)
 		if (oitem == item) {
 			error = ierror;
 		}
+
+		/*
+		 * If the node goes away when we remove the reference, 
+		 * whatever we just did caused it.. hwatever we do, DO NOT
+		 * access the node again!
+		 */
+		if (NG_NODE_UNREF(node) == 0) {
+			break;
+		}
 	}
 	return (error);
 }
@@ -2266,9 +2307,8 @@ ng_snd_item(item_p item, int queue)
  * to run it on the appropriate node/hook.
  */
 static int
-ng_apply_item(item_p item)
+ng_apply_item(node_p node, item_p item)
 {
-	node_p	node;
 	hook_p  hook;
 	int	was_reader = ((item->el_flags & NGQF_RW));
 	int	error = 0;
@@ -2276,7 +2316,6 @@ ng_apply_item(item_p item)
 	ng_rcvmsg_t *rcvmsg;
 
 	NGI_GET_HOOK(item, hook); /* clears stored hook */
-	NGI_GET_NODE(item, node); /* clears stored node */
 #ifdef	NETGRAPH_DEBUG
         _ngi_check(item, __FILE__, __LINE__);
 #endif
@@ -2394,7 +2433,6 @@ ng_apply_item(item_p item)
 	} else {
 		ng_leave_write(&node->nd_input_queue);
 	}
-	NG_NODE_UNREF(node);
 	return (error);
 }
 
@@ -3272,7 +3310,9 @@ ngintr(void)
 				break; /* go look for another node */
 			} else {
 				mtx_unlock_spin(&node->nd_input_queue.q_mtx);
-				ng_apply_item(item);
+				NGI_GET_NODE(item, node); /* zaps stored node */
+				ng_apply_item(node, item);
+				NG_NODE_UNREF(node);
 			}
 		}
 		NG_NODE_UNREF(node);
@@ -3544,7 +3584,6 @@ ng_send_fn(node_p node, hook_p hook, ng_item_fn *fn, void * arg1, int arg2)
 		return (ENOMEM);
 	}
 	item->el_flags = NGQF_FN | NGQF_WRITER;
-	NG_NODE_REF(node); /* One for us */
 	NG_NODE_REF(node); /* and one for the item */
 	NGI_SET_NODE(item, node);
 	if (hook) {
@@ -3554,8 +3593,7 @@ ng_send_fn(node_p node, hook_p hook, ng_item_fn *fn, void * arg1, int arg2)
 	NGI_FN(item) = fn;
 	NGI_ARG1(item) = arg1;
 	NGI_ARG2(item) = arg2;
-	return (ng_snd_item(item, 0));
-	NG_NODE_UNREF(node);
+	return(ng_snd_item(item, 0));
 }
 
 /*
diff --git a/sys/netgraph/ng_pptpgre.c b/sys/netgraph/ng_pptpgre.c
index c9a83ac..1a1f6ca 100644
--- a/sys/netgraph/ng_pptpgre.c
+++ b/sys/netgraph/ng_pptpgre.c
@@ -869,8 +869,8 @@ ng_pptpgre_send_ack_timeout(void *arg)
 		splx(s);
 		return;
 	}
-	NG_NODE_UNREF(node);
 	if (a->sackTimerPtr != arg) {	/* timer stopped race condition */
+		NG_NODE_UNREF(node);
 		splx(s);
 		return;
 	}
@@ -878,6 +878,7 @@ ng_pptpgre_send_ack_timeout(void *arg)
 
 	/* Send a frame with an ack but no payload */
   	ng_pptpgre_xmit(node, NULL);
+	NG_NODE_UNREF(node);
 	splx(s);
 }