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

[Konstantin Belousov]

On Fri, Nov 21, 2014 at 10:55:55PM +0300, Eygene Ryabinkin wrote:
> Fri, Nov 21, 2014 at 06:56:58PM +0200, Konstantin Belousov wrote:
> > On Fri, Nov 21, 2014 at 07:07:14PM +0300, Eygene Ryabinkin wrote:
> > > So, it looks like a kernel bug (since if we request SA_SIGINFO, we
> > > should get the proper handler to be called even for the SA_RESETHAND
> > > case).  I see two possibilities:
> > > 
> > >  - invoke SA_RESETHAND processing inside mach-dependent code; that's
> > >    a kind of ugly and makes mach-specific code to deal with the
> > >    generic signal handling logics;
> > > 
> > >  - pass information about SA_SIGINFO "out-of-band" (not in
> > >    ps->ps_siginfo).
> > > 
> > > We can't postpone sigdflt() to be called after signal being delivered,
> > > since spec requires it to be done before calling user-space handler.
> >
> > No, userspace is not called from sv_sendsig(), it simply cannot work
> > because nested signals would cause nesting on the kernel stack frame.
> > sv_sendsig() only prepares the usermode signal stack frame and arranges
> > the saved thread CPU state so that on return from kernel to usermode,
> > the signal handler is executed.
> 
> So, if we rearrange calls to sv_sendsig() and sigdflt() there can't be
> cases when the process being signalled will pick the signal before
> sigdflt() will be called?  Just because XSI wants that handler reset
> and clearing of SIGINFO happen before handler execution,
>   http://pubs.opengroup.org/onlinepubs/009695399/functions/sigaction.html
> So, I went into more trouble of not touching the order.
We do not return into usermode in random places of the kernel.
As I explained, signal delivery is done by arranging normal return
to usermode to effectively return to new signal frame.

> 
> > And, amusingly, the only thing which sv_sendsig() methods need and which
> > is also touched by sigdflt(), is ps_siginfo.  Simply rearranging the
> > order of calls should be enough.  I put the patch at the end of message,
> > it worked for me.
> 
> If there is no change in behaviour that will arise from rearranging
> the order of calls to mach-dependent and mach-independent code,
> I'd go a bit firther and unify some repeated code,
>   http://codelabs.ru/fbsd/patches/libthr/11-CURRENT-fix-SIGINFO-processing-with-RESETHAND-v2.diff
> 
> Works for me too, just tested with the same Squid installation.
This looks correct, but is much more delicate change.
In particular, the signal mask copied to usermode as part of ucontext,
for restoration at sigreturn(2), seems to be correct in both cases, but
in the postsig() case, where sendsig_common() is put after sv_sendsig()
call, it depends on the returnmask calculation.

Can you test that signal mask after signal return is correct with your
patch ?

Two other notes about your patch, which should be changed before it is
committable.  First, the name sendsig_common() is not telling anything.
Might be, rename the function to postsig_sigprop() or like.

In the same vein, comment is too ambitious for small helper.  This is not
The common code, just a helper to handle thread signal mask and several
other details of delivery.  Just specifying that this is the thing to
call after sysent->sv_sendsig() to arrange for proper bookkeeping, is
enough.

Second, function needs asserts that process lock and sigact mutex (ps_mtx)
are locked.