Date: Thu, 6 Sep 2012 01:29:13 GMT From: Mark Johnston <markjdb@gmail.com> To: freebsd-gnats-submit@FreeBSD.org Subject: kern/171360: [patch][dtrace] the fasttrap fork handler recurses on the child proc mutex Message-ID: <201209060129.q861TDOC082205@red.freebsd.org> Resent-Message-ID: <201209060130.q861U2Gr071309@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201209060129.q861TDOC082205>