[CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case

[Konstantin Belousov]

On Thu, Nov 27, 2014 at 08:14:09PM +0300, Eygene Ryabinkin wrote:
> Thu, Nov 27, 2014 at 12:14:06PM +0200, Konstantin Belousov wrote:
> > On Thu, Nov 27, 2014 at 07:21:14AM +0300, Eygene Ryabinkin wrote:
> > > "PROC_LOCK_ASSERT(td->td_proc, MA_OWNED);" be also present single
> > > kern_sigprocmask() is called with SIGPROCMASK_PROC_LOCKED?
> >
> > This is what I suggested in my earlier mail. When I started looking into
> > details, I decided not to do it. The reasoning is that the function
> > itself touches no process state protected by the proc lock. It is
> > kern_sigprocmask which works with process lock-protected data. On the
> > other hand, the function works with p_sigacts, which is covered by
> > ps_mtx, and I asserted it.
> 
> But sendsig_done() calls kern_sigprocmask with
> (SIGPROCMASK_PROC_LOCKED | SIGPROCMASK_PS_LOCKED) thus guaranteeing
> some set of locks, so its better to have some ways to support this
> assertion.  I understand that the current implementation of
> kern_sigprocmask() will test such an assertion, but things may change
> with time.
kern_sigprocmask (will) assert this on its own. (will == after the patch
is committed, which I am going to do right now).

> 
> Moreover, having all asserts at the beginning of a function makes
> at least myself to immediately recognise which lock(s) should be taken
> before this function is to be called, so it is a kind of a documentation.
> 
> > I believe that the following patch is due.  It is better way to ensure
> > the invariants, instead of adding assert to postsig_done().
> 
> These are good, but, probably, you can do something like
> {{{
> PROC_LOCK_ASSERT(p, (flags & SIGPROCMASK_PROC_LOCKED) ? MA_OWNED : MA_NOTOWNED)
> }}}
> inside kern_sigprocmask to have a bit stronger assertion that covers
> unlocked case.
This is not needed.  The process lock is not recursive, so the mere call
to acquire the lock triggers the assertion on recursive acquire.

> 
> > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> > index 15638e0..502f334 100644
> > --- a/sys/kern/kern_sig.c
> > +++ b/sys/kern/kern_sig.c
> > @@ -998,8 +998,12 @@ kern_sigprocmask(struct thread *td, int how, sigset_t *set, sigset_t *oset,
> >  	int error;
> >  
> >  	p = td->td_proc;
> > -	if (!(flags & SIGPROCMASK_PROC_LOCKED))
> > +	if ((flags & SIGPROCMASK_PROC_LOCKED) != 0)
> > +		PROC_LOCK_ASSERT(p, MA_OWNED);
> > +	else
> >  		PROC_LOCK(p);
> > +	mtx_assert(&p->p_sigacts->ps_mtx, (flags & SIGPROCMASK_PS_LOCKED) != 0
> > +	    ? MA_OWNED : MA_NOTOWNED);
> >  	if (oset != NULL)
> >  		*oset = td->td_sigmask;
> >  
> > @@ -2510,9 +2514,11 @@ reschedule_signals(struct proc *p, sigset_t block, int flags)
> >  	int sig;
> >  
> >  	PROC_LOCK_ASSERT(p, MA_OWNED);
> > +	ps = p->p_sigacts;
> > +	mtx_assert(&ps->ps_mtx, (flags & SIGPROCMASK_PS_LOCKED) != 0 ?
> > +	    MA_OWNED : MA_NOTOWNED);
> >  	if (SIGISEMPTY(p->p_siglist))
> >  		return;
> > -	ps = p->p_sigacts;
> >  	SIGSETAND(block, p->p_siglist);
> >  	while ((sig = sig_ffs(&block)) != 0) {
> >  		SIGDELSET(block, sig);
> > 
> 
> And, being in the nit-picking mode, I'd apply the following patch
> {{{
> Index: sys/kern/kern_sig.c
> ===================================================================
> --- sys/kern/kern_sig.c	(revision 275189)
> +++ sys/kern/kern_sig.c	(working copy)
> @@ -1043,7 +1043,7 @@
>  		 * signal, possibly waking it up.
>  		 */
>  		if (p->p_numthreads != 1)
> -			reschedule_signals(p, new_block, flags);
> +			reschedule_signals(p, new_block, flags & SIGPROCMASK_PS_LOCKED);
>  	}
>  
>  out:
> }}}
> because reschedule_signals() is interested only in SIGPROCMASK_PS_LOCKED,
> so there is no reason to pass other bits, since it may lead to some
> confusion.

I disagree, I wrote reschedule_signals() to take the same set of flags as
kern_sigprocmask().