> Date: Fri, 8 Nov 2013 12:34:42 +0100
> From: Martin Pieuchot <mpieuc...@nolizard.org>
>
> On 20/06/13(Thu) 15:18, Martin Pelikan wrote:
> > > And p_sigmask is copied during fork, so that needs a bit of thought.
> > > I guess it doesn't matter for the new child, as it isn't running yet
> > > and therefore can't invoke sigprocmask(2). And the parent should be
> > > safe as well, as it is in fork(2) and therefore can't call
> > > sigprocmask(2) either.
> >
> > memcpy(p_startcopy) which is, tadaaa, p_sigmask! From implementations
> > of memcpy I've seen (usage of SSE or non-temporal instructions to begin
> > with) it seems that no guarantee about how big a chunk does it copy
> > atomically can be assumed...
> >
> > So a multithreaded process doing sigprocmask(2) in one thread and
> > simultaneously fork(2) in the other? Not a problem either, because
> > 1003.1 2004 edition says that sigprocmask(2) is equivalent to
> > pthread_sigmask(3) and the fork only copies the other thread, not
> > bothering with the sigprocmask-ing one.
> > I haven't checked the code that thoroughly but this is what POSIX says.
> >
> > > There's also a couple of additional p->p_sigmask |= ... in kern_sig.c.
> > >
> > > All of these need to become atomic_setbits_int/atomic_clearbits_int calls.
> >
> > Does the third one do the charm? I haven't compiled the other arch's,
> > but i386 is running fine.
>
> So what's going on with this diff? fwiw I tried it on macppc and amd64.
>
> I'm also wondering, don't you think ptsignal() needs to be modified as
> well? Because it might access p_sigmask various times, so are you sure
> your diff won't introduce any side effect if the mask is modified in
> between? Or is it simply impossible?
I came to the conclusion that in its current form, this diff is *not*
safe :(. There is indeed a problem in ptsignal(). Essentially there
is a race between deciding to which thread to deliver the signal to
and the actual delivery. This particularly applies to the case where
you are blocking a signal; unblocking a signal is probably safe.
> > Index: arch/hp300/hp300/trap.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/hp300/hp300/trap.c,v
> > retrieving revision 1.62
> > diff -u -p -r1.62 trap.c
> > --- arch/hp300/hp300/trap.c 31 Dec 2012 06:46:13 -0000 1.62
> > +++ arch/hp300/hp300/trap.c 20 Jun 2013 12:51:46 -0000
> > @@ -321,7 +321,7 @@ dopanic:
> > i = sigmask(SIGILL);
> > p->p_sigacts->ps_sigignore &= ~i;
> > p->p_sigacts->ps_sigcatch &= ~i;
> > - p->p_sigmask &= ~i;
> > + atomic_clearbits_int(&p->p_sigmask, i);
> > i = SIGILL;
> > ucode = frame.f_format; /* XXX was ILL_RESAD_FAULT */
> > typ = ILL_COPROC;
> > Index: arch/mvme68k/mvme68k/trap.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/mvme68k/mvme68k/trap.c,v
> > retrieving revision 1.77
> > diff -u -p -r1.77 trap.c
> > --- arch/mvme68k/mvme68k/trap.c 31 Dec 2012 06:46:13 -0000 1.77
> > +++ arch/mvme68k/mvme68k/trap.c 20 Jun 2013 12:51:47 -0000
> > @@ -260,7 +260,7 @@ copyfault:
> > i = sigmask(SIGILL);
> > p->p_sigacts->ps_sigignore &= ~i;
> > p->p_sigacts->ps_sigcatch &= ~i;
> > - p->p_sigmask &= ~i;
> > + atomic_clearbits_int(&p->p_sigmask, i);
> > i = SIGILL;
> > ucode = frame.f_format; /* XXX was ILL_RESAD_FAULT */
> > typ = ILL_COPROC;
> > Index: compat/linux/linux_signal.c
> > ===================================================================
> > RCS file: /cvs/src/sys/compat/linux/linux_signal.c,v
> > retrieving revision 1.15
> > diff -u -p -r1.15 linux_signal.c
> > --- compat/linux/linux_signal.c 19 Jun 2012 11:35:29 -0000 1.15
> > +++ compat/linux/linux_signal.c 20 Jun 2013 12:51:49 -0000
> > @@ -584,7 +584,6 @@ linux_sys_sigprocmask(p, v, retval)
> > linux_old_sigset_t ss;
> > sigset_t bs;
> > int error = 0;
> > - int s;
> >
> > *retval = 0;
> >
> > @@ -604,19 +603,18 @@ linux_sys_sigprocmask(p, v, retval)
> >
> > linux_old_to_bsd_sigset(&ss, &bs);
> >
> > - s = splhigh();
> > -
> > + bs &= ~sigcantmask;
> > switch (SCARG(uap, how)) {
> > case LINUX_SIG_BLOCK:
> > - p->p_sigmask |= bs & ~sigcantmask;
> > + atomic_setbits_int(&p->p_sigmask, bs);
> > break;
> >
> > case LINUX_SIG_UNBLOCK:
> > - p->p_sigmask &= ~bs;
> > + atomic_clearbits_int(&p->p_sigmask, bs);
> > break;
> >
> > case LINUX_SIG_SETMASK:
> > - p->p_sigmask = bs & ~sigcantmask;
> > + p->p_sigmask = bs;
> > break;
> >
> > default:
> > @@ -624,8 +622,6 @@ linux_sys_sigprocmask(p, v, retval)
> > break;
> > }
> >
> > - splx(s);
> > -
> > return (error);
> > }
> >
> > @@ -644,7 +640,6 @@ linux_sys_rt_sigprocmask(p, v, retval)
> > linux_sigset_t ls;
> > sigset_t bs;
> > int error = 0;
> > - int s;
> >
> > if (SCARG(uap, sigsetsize) != sizeof(linux_sigset_t))
> > return (EINVAL);
> > @@ -667,19 +662,18 @@ linux_sys_rt_sigprocmask(p, v, retval)
> >
> > linux_to_bsd_sigset(&ls, &bs);
> >
> > - s = splhigh();
> > -
> > + bs &= ~sigcantmask;
> > switch (SCARG(uap, how)) {
> > case LINUX_SIG_BLOCK:
> > - p->p_sigmask |= bs & ~sigcantmask;
> > + atomic_setbits_int(&p->p_sigmask, bs);
> > break;
> >
> > case LINUX_SIG_UNBLOCK:
> > - p->p_sigmask &= ~bs;
> > + atomic_clearbits_int(&p->p_sigmask, bs);
> > break;
> >
> > case LINUX_SIG_SETMASK:
> > - p->p_sigmask = bs & ~sigcantmask;
> > + p->p_sigmask = bs;
> > break;
> >
> > default:
> > @@ -687,8 +681,6 @@ linux_sys_rt_sigprocmask(p, v, retval)
> > break;
> > }
> >
> > - splx(s);
> > -
> > return (error);
> > }
> >
> > @@ -727,16 +719,13 @@ linux_sys_sigsetmask(p, v, retval)
> > } */ *uap = v;
> > linux_old_sigset_t mask;
> > sigset_t bsdsig;
> > - int s;
> >
> > bsd_to_linux_old_sigset(&p->p_sigmask, (linux_old_sigset_t *)retval);
> >
> > mask = SCARG(uap, mask);
> > bsd_to_linux_old_sigset(&bsdsig, &mask);
> >
> > - s = splhigh();
> > p->p_sigmask = bsdsig & ~sigcantmask;
> > - splx(s);
> >
> > return (0);
> > }
> > Index: kern/init_sysent.c
> > ===================================================================
> > RCS file: /cvs/src/sys/kern/init_sysent.c,v
> > retrieving revision 1.145
> > diff -u -p -r1.145 init_sysent.c
> > --- kern/init_sysent.c 9 Jun 2013 13:10:27 -0000 1.145
> > +++ kern/init_sysent.c 20 Jun 2013 12:51:49 -0000
> > @@ -136,7 +136,7 @@ struct sysent sysent[] = {
> > sys_sigaction }, /* 46 = sigaction */
> > { 0, 0, 0,
> > sys_getgid }, /* 47 = getgid */
> > - { 2, s(struct sys_sigprocmask_args), 0,
> > + { 2, s(struct sys_sigprocmask_args), SY_NOLOCK | 0,
> > sys_sigprocmask }, /* 48 = sigprocmask */
> > { 2, s(struct sys_getlogin_args), 0,
> > sys_getlogin }, /* 49 = getlogin */
> > Index: kern/kern_sig.c
> > ===================================================================
> > RCS file: /cvs/src/sys/kern/kern_sig.c,v
> > retrieving revision 1.152
> > diff -u -p -r1.152 kern_sig.c
> > --- kern/kern_sig.c 1 Jun 2013 04:05:26 -0000 1.152
> > +++ kern/kern_sig.c 20 Jun 2013 12:51:49 -0000
> > @@ -429,28 +429,25 @@ sys_sigprocmask(struct proc *p, void *v,
> > syscallarg(sigset_t) mask;
> > } */ *uap = v;
> > int error = 0;
> > - int s;
> > sigset_t mask;
> >
> > *retval = p->p_sigmask;
> > - mask = SCARG(uap, mask);
> > - s = splhigh();
> > + mask = SCARG(uap, mask) &~ sigcantmask;
> >
> > switch (SCARG(uap, how)) {
> > case SIG_BLOCK:
> > - p->p_sigmask |= mask &~ sigcantmask;
> > + atomic_setbits_int(&p->p_sigmask, mask);
> > break;
> > case SIG_UNBLOCK:
> > - p->p_sigmask &= ~mask;
> > + atomic_clearbits_int(&p->p_sigmask, mask);
> > break;
> > case SIG_SETMASK:
> > - p->p_sigmask = mask &~ sigcantmask;
> > + p->p_sigmask = mask;
> > break;
> > default:
> > error = EINVAL;
> > break;
> > }
> > - splx(s);
> > return (error);
> > }
> >
> > @@ -739,7 +736,7 @@ trapsignal(struct proc *p, int signum, u
> > p->p_ru.ru_nsignals++;
> > (*p->p_emul->e_sendsig)(ps->ps_sigact[signum], signum,
> > p->p_sigmask, trapno, code, sigval);
> > - p->p_sigmask |= ps->ps_catchmask[signum];
> > + atomic_setbits_int(&p->p_sigmask, ps->ps_catchmask[signum]);
> > if ((ps->ps_sigreset & mask) != 0) {
> > ps->ps_sigcatch &= ~mask;
> > if (signum != SIGCONT && sigprop[signum] & SA_IGNORE)
> > @@ -1334,7 +1331,7 @@ postsig(int signum)
> > } else {
> > returnmask = p->p_sigmask;
> > }
> > - p->p_sigmask |= ps->ps_catchmask[signum];
> > + atomic_setbits_int(&p->p_sigmask, ps->ps_catchmask[signum]);
> > if ((ps->ps_sigreset & mask) != 0) {
> > ps->ps_sigcatch &= ~mask;
> > if (signum != SIGCONT && sigprop[signum] & SA_IGNORE)
> > Index: kern/syscalls.master
> > ===================================================================
> > RCS file: /cvs/src/sys/kern/syscalls.master,v
> > retrieving revision 1.133
> > diff -u -p -r1.133 syscalls.master
> > --- kern/syscalls.master 9 Jun 2013 13:10:19 -0000 1.133
> > +++ kern/syscalls.master 20 Jun 2013 12:51:49 -0000
> > @@ -123,7 +123,7 @@
> > const struct sigaction *nsa, \
> > struct sigaction *osa); }
> > 47 STD { gid_t sys_getgid(void); }
> > -48 STD { int sys_sigprocmask(int how, sigset_t mask); }
> > +48 STD NOLOCK { int sys_sigprocmask(int how, sigset_t mask); }
> > 49 STD { int sys_getlogin(char *namebuf, u_int namelen); }
> > 50 STD { int sys_setlogin(const char *namebuf); }
> > #ifdef ACCOUNTING
> >
>
