From owner-freebsd-dtrace@FreeBSD.ORG Sun Nov 17 03:31:33 2013 Return-Path: Delivered-To: freebsd-dtrace@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 396152DF; Sun, 17 Nov 2013 03:31:33 +0000 (UTC) Received: from mail-ie0-x22c.google.com (mail-ie0-x22c.google.com [IPv6:2607:f8b0:4001:c03::22c]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 0331C250F; Sun, 17 Nov 2013 03:31:32 +0000 (UTC) Received: by mail-ie0-f172.google.com with SMTP id to1so7177043ieb.17 for ; Sat, 16 Nov 2013 19:31:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=7mNtsOVF9hJoyjI9cfH11tco9rpTOfaH+T2auRq0Au0=; b=m8Dm6tmsc8apPbTsxsrluxhTJ/6mFFBa2RbHQPUy3a0pvV1Yi56GR58YL2XhR0ykH+ 5/UHF2g0NvaK+Ox704LmooKVz7hNy93kUH/aRPGKFLQ4jdAnNhQfYRxUaG5HGjysSBp4 r1WMqsWK6HryuFZfUpz9+JBDMiYDW/3u5w9ZCavS2iWreSzgsOkSxZZLZknRC5z9oERL sOUrsBX16CIveyJZ2u7yDJ3LPGH+K5wbNr41kVZdsfNWLMNOMYSIqCGI5xPLjadaO7kS XvsO/qTdsfxWJ/zAL/LS9NsjMy+4EYn5AGP8vqGCHDyrgBI4zoTegf2nSXWLjxtYD47U +9Yw== X-Received: by 10.50.39.45 with SMTP id m13mr9317908igk.14.1384659091979; Sat, 16 Nov 2013 19:31:31 -0800 (PST) Received: from charmander (192-0-203-16.cpe.teksavvy.com. [192.0.203.16]) by mx.google.com with ESMTPSA id m1sm6223233igj.10.2013.11.16.19.31.30 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 16 Nov 2013 19:31:31 -0800 (PST) Sender: Mark Johnston Date: Sat, 16 Nov 2013 23:31:27 -0500 From: Mark Johnston To: Alan Somers Subject: Re: Please review: fix panics on kldload/kldunload fasttrap Message-ID: <20131117043127.GA64214@charmander> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.22 (2013-10-16) Cc: freebsd-dtrace@freebsd.org X-BeenThere: freebsd-dtrace@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: "A discussion list for developers working on DTrace in FreeBSD." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 Nov 2013 03:31:33 -0000 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"