Please review: fix panics on kldload/kldunload fasttrap

[Alan Somers]

I've found a few problems with fasttrap that can cause panics on
kldload and kldunload.  Can someone please review this patch?  I've
also attached an ATF test case for it.  The test case loads and
unloads the fasttrap module 500 times while several sh processes are
forking, execing, and exiting at about 600 times/second/cpu.

* kproc_create(fasttrap_pid_cleanup_cb, ...) gets called before
fasttrap_provs.fth_table gets allocated.  This can lead to a panic on
module load, because fasttrap_pid_cleanup_cb references
fasttrap_provs.fth_table.  My patch moves kproc_create down after the
point that fasttrap_provs.fth_table gets allocated, and modifies the
error handling accordingly.

* dtrace_fasttrap_{fork,exec,exit} weren't getting NULLed until after
fasttrap_provs.fth_table got freed.  That caused panics on module
unload because fasttrap_exec_exit calls fasttrap_provider_retire,
which references fasttrap_provs.fth_table.  My patch NULLs those
function pointers earlier.

* There isn't any code to destroy the
fasttrap_{tpoints,provs,procs}.fth_table mutexes on module unload,
leading to a resource leak when WITNESS is enabled.  My patch destroys
those mutexes during fasttrap_unload().

-Alan
-------------- next part --------------
Index: sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
===================================================================
--- sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c	(revision 257803)
+++ sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c	(working copy)
@@ -2284,13 +2284,6 @@
 	mutex_init(&fasttrap_count_mtx, "fasttrap count mtx", MUTEX_DEFAULT,
 	    NULL);
 
-	ret = kproc_create(fasttrap_pid_cleanup_cb, NULL,
-	    &fasttrap_cleanup_proc, 0, 0, "ftcleanup");
-	if (ret != 0) {
-		destroy_dev(fasttrap_cdev);
-		return (ret);
-	}
-
 #if defined(sun)
 	fasttrap_max = ddi_getprop(DDI_DEV_T_ANY, devi, DDI_PROP_DONTPASS,
 	    "fasttrap-max-probes", FASTTRAP_MAX_DEFAULT);
@@ -2344,6 +2337,24 @@
 		    "providers bucket mtx", MUTEX_DEFAULT, NULL);
 #endif
 
+	ret = kproc_create(fasttrap_pid_cleanup_cb, NULL,
+	    &fasttrap_cleanup_proc, 0, 0, "ftcleanup");
+	if (ret != 0) {
+		destroy_dev(fasttrap_cdev);
+#if !defined(sun)
+		for (i = 0; i < fasttrap_provs.fth_nent; i++)
+			mutex_destroy(&fasttrap_provs.fth_table[i].ftb_mtx);
+		for (i = 0; i < fasttrap_tpoints.fth_nent; i++)
+			mutex_destroy(&fasttrap_tpoints.fth_table[i].ftb_mtx);
+#endif
+		kmem_free(fasttrap_provs.fth_table, fasttrap_provs.fth_nent *
+		    sizeof (fasttrap_bucket_t));
+		mtx_destroy(&fasttrap_cleanup_mtx);
+		mutex_destroy(&fasttrap_count_mtx);
+		return (ret);
+	}
+
+
 	/*
 	 * ... and the procs hash table.
 	 */
@@ -2436,6 +2447,20 @@
 		return (-1);
 	}
 
+	/*
+	 * Stop new processes from entering these hooks now, before the
+	 * fasttrap_cleanup thread runs.  That way all processes will hopefully
+	 * be out of these hooks before we free fasttrap_provs.fth_table
+	 */
+	ASSERT(dtrace_fasttrap_fork == &fasttrap_fork);
+	dtrace_fasttrap_fork = NULL;
+
+	ASSERT(dtrace_fasttrap_exec == &fasttrap_exec_exit);
+	dtrace_fasttrap_exec = NULL;
+
+	ASSERT(dtrace_fasttrap_exit == &fasttrap_exec_exit);
+	dtrace_fasttrap_exit = NULL;
+
 	mtx_lock(&fasttrap_cleanup_mtx);
 	fasttrap_cleanup_drain = 1;
 	/* Wait for the cleanup thread to finish up and signal us. */
@@ -2451,6 +2476,14 @@
 	mutex_exit(&fasttrap_count_mtx);
 #endif
 
+#if !defined(sun)
+	for (i = 0; i < fasttrap_tpoints.fth_nent; i++)
+		mutex_destroy(&fasttrap_tpoints.fth_table[i].ftb_mtx);
+	for (i = 0; i < fasttrap_provs.fth_nent; i++)
+		mutex_destroy(&fasttrap_provs.fth_table[i].ftb_mtx);
+	for (i = 0; i < fasttrap_procs.fth_nent; i++)
+		mutex_destroy(&fasttrap_procs.fth_table[i].ftb_mtx);
+#endif
 	kmem_free(fasttrap_tpoints.fth_table,
 	    fasttrap_tpoints.fth_nent * sizeof (fasttrap_bucket_t));
 	fasttrap_tpoints.fth_nent = 0;
@@ -2463,22 +2496,6 @@
 	    fasttrap_procs.fth_nent * sizeof (fasttrap_bucket_t));
 	fasttrap_procs.fth_nent = 0;
 
-	/*
-	 * We know there are no tracepoints in any process anywhere in
-	 * the system so there is no process which has its p_dtrace_count
-	 * greater than zero, therefore we know that no thread can actively
-	 * be executing code in fasttrap_fork(). Similarly for p_dtrace_probes
-	 * and fasttrap_exec() and fasttrap_exit().
-	 */
-	ASSERT(dtrace_fasttrap_fork == &fasttrap_fork);
-	dtrace_fasttrap_fork = NULL;
-
-	ASSERT(dtrace_fasttrap_exec == &fasttrap_exec_exit);
-	dtrace_fasttrap_exec = NULL;
-
-	ASSERT(dtrace_fasttrap_exit == &fasttrap_exec_exit);
-	dtrace_fasttrap_exit = NULL;
-
 #if !defined(sun)
 	destroy_dev(fasttrap_cdev);
 	mutex_destroy(&fasttrap_count_mtx);