Date: Sat, 16 Nov 2013 23:31:27 -0500 From: Mark Johnston <markj@freebsd.org> To: Alan Somers <asomers@freebsd.org> Cc: freebsd-dtrace@freebsd.org Subject: Re: Please review: fix panics on kldload/kldunload fasttrap Message-ID: <20131117043127.GA64214@charmander> In-Reply-To: <CAOtMX2irjVOnTjv%2B_HpeRKLvwGpLGMjfWRw_d8vXbSdiFy4MrQ@mail.gmail.com> References: <CAOtMX2irjVOnTjv%2B_HpeRKLvwGpLGMjfWRw_d8vXbSdiFy4MrQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-dtrace
> To unsubscribe, send any mail to "freebsd-dtrace-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131117043127.GA64214>