On Fri, Oct 30, 2009 at 01:08:08AM +0200, Giorgos Keramidas wrote: > On Thu, 29 Oct 2009 14:34:24 +0000 (UTC), Konstantin Belousov > <k...@freebsd.org> wrote: > > Author: kib > > Date: Thu Oct 29 14:34:24 2009 > > New Revision: 198590 > > URL: http://svn.freebsd.org/changeset/base/198590 > > > > Log: > > Trapsignal() calls kern_sigprocmask() when delivering catched signal > > with proc lock held. > > > > Reported and tested by: Mykola Dzham freebsd at levsha org ua > > MFC after: 1 month > > Hi Konstantin, > > Some of the recent kern_sig changes end up recursing on a non-recursive > mutex in kern_sigprocmask() -> reschedule_signals(): > > panic: _mtx_lock_sleep: recursed on non-recursive mutex sigacts @ > /usr/src/sys/kern/kern_sig.c:2422 > (kgdb) bt > #0 doadump () at pcpu.h:246 > #1 0xc0680bee in boot (howto=260) at /usr/src/sys/kern/kern_shutdown.c:416 > #2 0xc0680ec2 in panic (fmt=Variable "fmt" is not available. > ) at /usr/src/sys/kern/kern_shutdown.c:579 > #3 0xc06716ea in _mtx_lock_sleep (m=0xc8154aa8, tid=3332925072, opts=0, > file=0xc09bb332 "/usr/src/sys/kern/kern_sig.c", line=2422) > at /usr/src/sys/kern/kern_mutex.c:341 > #4 0xc0671907 in _mtx_lock_flags (m=0xc8154aa8, opts=0, file=0xc09bb332 > "/usr/src/sys/kern/kern_sig.c", line=2422) > at /usr/src/sys/kern/kern_mutex.c:203 > #5 0xc0683434 in reschedule_signals (p=0xc71172a8, block={__bits = {0, 0, 0, > 0}}) at /usr/src/sys/kern/kern_sig.c:2422 > #6 0xc0683751 in kern_sigprocmask (td=0xc6a86690, how=1, set=0xe9005cd4, > oset=0x0, flags=2) at /usr/src/sys/kern/kern_sig.c:1027 > #7 0xc0684801 in postsig (sig=20) at /usr/src/sys/kern/kern_sig.c:2743 > #8 0xc06be228 in ast (framep=0xe9005d38) at /usr/src/sys/kern/subr_trap.c:234 > #9 0xc0920624 in doreti_ast () at /usr/src/sys/i386/i386/exception.s:365 > > I think the change that started causing this for MT applications was > change 197963 in /head that added this bit of code in kern_sig.c inside > kern_sigprocmask(): > > : @@ -1012,7 +1012,20 @@ > : break; > : } > : } > : - PROC_UNLOCK(td->td_proc); > : + /* > : + * The new_block set contains signals that were not previosly > : + * blocked, but are blocked now. > : + * > : + * In case we block any signal that was not previously blocked > : + * for td, and process has the signal pending, try to schedule > : + * signal delivery to some thread that does not block the signal, > : + * possibly waking it up. > : + */ > : + if (p->p_numthreads != 1) > : + reschedule_signals(p, new_block); > : + > : + PROC_UNLOCK(p); > : return (error); No, this was caused by the r198507 fragment of postsig(), as well as a fragment from the trapsignal(), that added the call to kern_sigprocmask() instead of direct manipulation of thread signal mask.
>
> AFAICT, postsig() is called with proc->p_sigacts->ps_mtx locked, so when
> we are recursing when reschedule_signals() tries to lock it once more.
Yes, the same statement holds for trapsignal().
>
> Since we are holding the proc lock in kern_sigprocmask(), is it safe to
> assert that we own ps_mtx, drop it and re-acquire it immediately after
> calling reschedule_signals()?
No. Most callers of kern_sigprocmask() do not hold curproc->ps_mtx.
Only postsig() and trapsig() call kern_sigprocmask() with ps_mtx locked,
so I have to special-case them.
Could you, please, test the following patch ? What application did
exposed the issue ?
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 7f5cfa3..e174df1 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -220,7 +220,7 @@ static int sigproptbl[NSIG] = {
SA_KILL|SA_PROC, /* SIGUSR2 */
};
-static void reschedule_signals(struct proc *p, sigset_t block);
+static void reschedule_signals(struct proc *p, sigset_t block, int flags);
static void
sigqueue_start(void)
@@ -1024,7 +1024,7 @@ kern_sigprocmask(struct thread *td, int how, sigset_t
*set, sigset_t *oset,
* possibly waking it up.
*/
if (p->p_numthreads != 1)
- reschedule_signals(p, new_block);
+ reschedule_signals(p, new_block, flags);
if (!(flags & SIGPROCMASK_PROC_LOCKED))
PROC_UNLOCK(p);
@@ -1859,13 +1859,11 @@ trapsignal(struct thread *td, ksiginfo_t *ksi)
#endif
(*p->p_sysent->sv_sendsig)(ps->ps_sigact[_SIG_IDX(sig)],
ksi, &td->td_sigmask);
- SIGSETOR(td->td_sigmask, ps->ps_catchmask[_SIG_IDX(sig)]);
- if (!SIGISMEMBER(ps->ps_signodefer, sig)) {
- SIGEMPTYSET(mask);
+ mask = ps->ps_catchmask[_SIG_IDX(sig)];
+ if (!SIGISMEMBER(ps->ps_signodefer, sig))
SIGADDSET(mask, sig);
- kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
- SIGPROCMASK_PROC_LOCKED);
- }
+ kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
+ SIGPROCMASK_PROC_LOCKED | SIGPROCMASK_PS_LOCKED);
if (SIGISMEMBER(ps->ps_sigreset, sig)) {
/*
* See kern_sigaction() for origin of this code.
@@ -2401,7 +2399,7 @@ stopme:
}
static void
-reschedule_signals(struct proc *p, sigset_t block)
+reschedule_signals(struct proc *p, sigset_t block, int flags)
{
struct sigacts *ps;
struct thread *td;
@@ -2419,12 +2417,14 @@ reschedule_signals(struct proc *p, sigset_t block)
td = sigtd(p, i, 0);
signotify(td);
- mtx_lock(&ps->ps_mtx);
+ if (!(flags & SIGPROCMASK_PS_LOCKED))
+ mtx_lock(&ps->ps_mtx);
if (p->p_flag & P_TRACED || SIGISMEMBER(ps->ps_sigcatch, i))
tdsigwakeup(td, i, SIG_CATCH,
(SIGISMEMBER(ps->ps_sigintr, i) ? EINTR :
ERESTART));
- mtx_unlock(&ps->ps_mtx);
+ if (!(flags & SIGPROCMASK_PS_LOCKED))
+ mtx_unlock(&ps->ps_mtx);
}
}
@@ -2452,7 +2452,7 @@ tdsigcleanup(struct thread *td)
SIGFILLSET(unblocked);
SIGSETNAND(unblocked, td->td_sigmask);
SIGFILLSET(td->td_sigmask);
- reschedule_signals(p, unblocked);
+ reschedule_signals(p, unblocked, 0);
}
@@ -2734,15 +2734,11 @@ postsig(sig)
} else
returnmask = td->td_sigmask;
- kern_sigprocmask(td, SIG_BLOCK,
- &ps->ps_catchmask[_SIG_IDX(sig)], NULL,
- SIGPROCMASK_PROC_LOCKED);
- if (!SIGISMEMBER(ps->ps_signodefer, sig)) {
- SIGEMPTYSET(mask);
+ mask = ps->ps_catchmask[_SIG_IDX(sig)];
+ if (!SIGISMEMBER(ps->ps_signodefer, sig))
SIGADDSET(mask, sig);
- kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
- SIGPROCMASK_PROC_LOCKED);
- }
+ kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
+ SIGPROCMASK_PROC_LOCKED | SIGPROCMASK_PS_LOCKED);
if (SIGISMEMBER(ps->ps_sigreset, sig)) {
/*
diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h
index b9a54f0..c27a128 100644
--- a/sys/sys/signalvar.h
+++ b/sys/sys/signalvar.h
@@ -319,6 +319,7 @@ extern int kern_logsigexit; /* Sysctl variable
kern.logsigexit */
/* flags for kern_sigprocmask */
#define SIGPROCMASK_OLD 0x0001
#define SIGPROCMASK_PROC_LOCKED 0x0002
+#define SIGPROCMASK_PS_LOCKED 0x0004
/*
* Machine-independent functions:
pgpqWkf2i5vvy.pgp
Description: PGP signature
