Please review: fix panics on kldload/kldunload fasttrap

[Mark Johnston]

On Fri, Nov 15, 2013 at 01:31:26PM -0700, Alan Somers wrote:
> 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.

This looks good to me. Thanks!

> 
> * 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

> 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);

> _______________________________________________
> freebsd-dtrace at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-dtrace
> To unsubscribe, send any mail to "freebsd-dtrace-unsubscribe at freebsd.org"