From owner-freebsd-bugs@FreeBSD.ORG Thu Sep 6 01:30:03 2012 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0BD67106566B for ; Thu, 6 Sep 2012 01:30:03 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id D06C08FC1B for ; Thu, 6 Sep 2012 01:30:02 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.5/8.14.5) with ESMTP id q861U2Ln071326 for ; Thu, 6 Sep 2012 01:30:02 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.5/8.14.5/Submit) id q861U2Gr071309; Thu, 6 Sep 2012 01:30:02 GMT (envelope-from gnats) Resent-Date: Thu, 6 Sep 2012 01:30:02 GMT Resent-Message-Id: <201209060130.q861U2Gr071309@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Mark Johnston Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id B3023106564A for ; Thu, 6 Sep 2012 01:29:13 +0000 (UTC) (envelope-from nobody@FreeBSD.org) Received: from red.freebsd.org (red.freebsd.org [IPv6:2001:4f8:fff6::22]) by mx1.freebsd.org (Postfix) with ESMTP id 9D3EA8FC0A for ; Thu, 6 Sep 2012 01:29:13 +0000 (UTC) Received: from red.freebsd.org (localhost [127.0.0.1]) by red.freebsd.org (8.14.5/8.14.5) with ESMTP id q861TDj8082206 for ; Thu, 6 Sep 2012 01:29:13 GMT (envelope-from nobody@red.freebsd.org) Received: (from nobody@localhost) by red.freebsd.org (8.14.5/8.14.5/Submit) id q861TDOC082205; Thu, 6 Sep 2012 01:29:13 GMT (envelope-from nobody) Message-Id: <201209060129.q861TDOC082205@red.freebsd.org> Date: Thu, 6 Sep 2012 01:29:13 GMT From: Mark Johnston To: freebsd-gnats-submit@FreeBSD.org X-Send-Pr-Version: www-3.1 Cc: Subject: kern/171360: [patch][dtrace] the fasttrap fork handler recurses on the child proc mutex X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Sep 2012 01:30:03 -0000 >Number: 171360 >Category: kern >Synopsis: [patch][dtrace] the fasttrap fork handler recurses on the child proc mutex >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Thu Sep 06 01:30:02 UTC 2012 >Closed-Date: >Last-Modified: >Originator: Mark Johnston >Release: CURRENT >Organization: >Environment: FreeBSD oddish 10.0-CURRENT FreeBSD 10.0-CURRENT #7 r240137=b105ea9-dirty: Wed Sep 5 14:02:01 EDT 2012 mark@oddish:/usr/obj/usr/home/mark/src/freebsd/sys/GENERIC amd64 >Description: The fasttrap fork handler calls fasttrap_tracepoint_remove() with the child proc mutex held (acquired in do_fork()), which then calls fasttrap_isa.c:proc_ops(), which acquires the proc mutex again (in PHOLD()). With INVARIANTS enabled, this causes a panic. >How-To-Repeat: An easy way to reproduce this is by running dtrace -n 'pid$target::main:entry {}' -c /bin/sh and then running ls at the sh prompt. If INVARIANTS is enabled, you'll get a panic that looks like this: panic: _mtx_lock_sleep: recursed on non-recursive mutex process lock @ /usr/home/mark/src/freebsd/sys/modules/dtrace/fasttrap/../../../cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c:77 (kgdb) bt #0 doadump (textdump=1) at /usr/home/mark/src/freebsd/sys/kern/kern_shutdown.c:266 #1 0xffffffff8063f262 in kern_reboot (howto=260) at /usr/home/mark/src/freebsd/sys/kern/kern_shutdown.c:449 #2 0xffffffff8063ec8a in panic (fmt=0x0) at /usr/home/mark/src/freebsd/sys/kern/kern_shutdown.c:637 #3 0xffffffff8062ccff in _mtx_lock_sleep (m=Variable "m" is not available. ) at /usr/home/mark/src/freebsd/sys/kern/kern_mutex.c:363 #4 0xffffffff8062ce11 in _mtx_lock_flags (m=0xfffffe0008dec598, opts=0, file=0xffffffff816908f8 "/usr/home/mark/src/freebsd/sys/modules/dtrace/fasttrap/../../../cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c", line=77) at /usr/home/mark/src/freebsd/sys/kern/kern_mutex.c:212 #5 0xffffffff8168df91 in proc_ops (op=Variable "op" is not available. ) at /usr/home/mark/src/freebsd/sys/modules/dtrace/fasttrap/../../../cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c:77 #6 0xffffffff8168e1c8 in fasttrap_tracepoint_remove (p=0xfffffe0008dec4a0, tp=0xfffffe0008b9f680) at /usr/home/mark/src/freebsd/sys/modules/dtrace/fasttrap/../../../cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c:693 #7 0xffffffff8168cf17 in fasttrap_fork (p=Variable "p" is not available. ) at /usr/home/mark/src/freebsd/sys/modules/dtrace/fasttrap/../../../cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c:515 #8 0xffffffff8061072a in fork1 (td=0xfffffe000b176900, flags=20, pages=Variable "pages" is not available. ) at /usr/home/mark/src/freebsd/sys/kern/kern_fork.c:693 #9 0xffffffff80610f12 in sys_fork (td=0xfffffe000b176900, uap=Variable "uap" is not available. ) at /usr/home/mark/src/freebsd/sys/kern/kern_fork.c:110 >Fix: It doesn't seem to me that either the child or parent proc mutexes need to be held in the fasttrap fork handler, so I've attached a patch which just releases them at the beginning of the handler. This is what the exec() and exit() handlers also do. The patch fixes the problem for me so that I can go on playing around with userland dtrace without disabling INVARIANTS, but I'm more than happy to test other fixes. Patch attached with submission follows: diff --git a/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c b/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c index 4599a32..bc2f5e7 100644 --- a/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c +++ b/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c @@ -450,31 +450,31 @@ fasttrap_fork(proc_t *p, proc_t *cp) #if defined(sun) ASSERT(curproc == p); ASSERT(p->p_proc_flag & P_PR_LOCK); -#else - PROC_LOCK_ASSERT(p, MA_OWNED); -#endif -#if defined(sun) ASSERT(p->p_dtrace_count > 0); #else + PROC_LOCK_ASSERT(p, MA_OWNED); + PROC_LOCK_ASSERT(cp, MA_OWNED); + + PROC_UNLOCK(p); + PROC_UNLOCK(cp); if (p->p_dtrace_helpers) { /* * dtrace_helpers_duplicate() allocates memory. */ - _PHOLD(cp); - PROC_UNLOCK(p); - PROC_UNLOCK(cp); + PHOLD(cp); dtrace_helpers_duplicate(p, cp); - PROC_LOCK(cp); - PROC_LOCK(p); - _PRELE(cp); + PRELE(cp); } /* * This check is purposely here instead of in kern_fork.c because, * for legal resons, we cannot include the dtrace_cddl.h header * inside kern_fork.c and insert if-clause there. */ - if (p->p_dtrace_count == 0) + if (p->p_dtrace_count == 0) { + PROC_LOCK(cp); + PROC_LOCK(p); return; + } #endif ASSERT(cp->p_dtrace_count == 0); @@ -497,7 +497,7 @@ fasttrap_fork(proc_t *p, proc_t *cp) sprlock_proc(cp); mtx_unlock_spin(&cp->p_slock); #else - _PHOLD(cp); + PHOLD(cp); #endif /* @@ -532,6 +532,8 @@ fasttrap_fork(proc_t *p, proc_t *cp) mutex_enter(&cp->p_lock); sprunlock(cp); #else + PROC_LOCK(cp); + PROC_LOCK(p); _PRELE(cp); #endif } >Release-Note: >Audit-Trail: >Unformatted: