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?
> --
> Martin Pelikan
>
>
> 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
>
