On 07/07/14(Mon) 13:46, Matthew Dempsky wrote:
> p_sigmask is only modified by the owning thread from process context
> (e.g., sys_sigprocmask(), sys_sigreturn(), userret(), or postsig()),
> but it can be accessed anywhere (e.g., interrupts or threads on other
> CPUs). Currently sys_sigprocmask() protects p_sigmask with splhigh(),
> but that's not MP-safe.
>
>
> The simpler solution is to take advantage of our atomics APIs.
> Unfortunately for implementing SIG_SETMASK, we don't have an
> atomic_store_int() function, and I can't bring myself to abuse
> "volatile" for this purpose, so I've resorted to atomic_swap_uint().
>
> While here, I also refactored the P_SIGSUSPEND code a bit so there's
> less code duplication.
>
> I've included just amd64 and i386's machdep.c for now, because those
> were the only ones I'm readily able to test. The others should be
> easy to similarly fix though, and I can send a followup diff for them.
There was a previous attempt to mark sigpromask(2) as nolock but if I
recall properly setting p_sigmask atomically is not enough and there's
still a possible race in ptsignal().
Have you looked into this function, is it safe now?
> Index: sys/proc.h
> ===================================================================
> RCS file: /home/matthew/cvs-mirror/cvs/src/sys/sys/proc.h,v
> retrieving revision 1.186
> diff -u -p -r1.186 proc.h
> --- sys/proc.h 15 May 2014 03:52:25 -0000 1.186
> +++ sys/proc.h 7 Jul 2014 19:23:06 -0000
> @@ -520,6 +520,8 @@ int proc_cansugid(struct proc *);
> void proc_finish_wait(struct proc *, struct proc *);
> void process_zap(struct process *);
> void proc_free(struct proc *);
> +void proc_sigsetmask(struct proc *, sigset_t);
> +void proc_sigsuspend(struct proc *, sigset_t);
>
> struct sleep_state {
> int sls_s;
> @@ -559,4 +561,3 @@ struct cpu_info *cpuset_first(struct cpu
>
> #endif /* _KERNEL */
> #endif /* !_SYS_PROC_H_ */
> -
> Index: kern/kern_sig.c
> ===================================================================
> RCS file: /home/matthew/cvs-mirror/cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.166
> diff -u -p -r1.166 kern_sig.c
> --- kern/kern_sig.c 4 May 2014 05:03:26 -0000 1.166
> +++ kern/kern_sig.c 7 Jul 2014 19:24:07 -0000
> @@ -62,6 +62,7 @@
> #include <sys/ptrace.h>
> #include <sys/sched.h>
> #include <sys/user.h>
> +#include <sys/atomic.h>
>
> #include <sys/mount.h>
> #include <sys/syscallargs.h>
> @@ -439,37 +440,55 @@ sys_sigprocmask(struct proc *p, void *v,
> syscallarg(int) how;
> syscallarg(sigset_t) mask;
> } */ *uap = v;
> - int error = 0;
> - int s;
> - sigset_t mask;
> + sigset_t mask = SCARG(uap, mask);
>
> *retval = p->p_sigmask;
> - mask = SCARG(uap, mask);
> - s = splhigh();
>
> switch (SCARG(uap, how)) {
> case SIG_BLOCK:
> - p->p_sigmask |= mask &~ sigcantmask;
> + atomic_setbits_int(&p->p_sigmask, mask &~ sigcantmask);
> break;
> case SIG_UNBLOCK:
> - p->p_sigmask &= ~mask;
> + atomic_clearbits_int(&p->p_sigmask, mask);
> break;
> case SIG_SETMASK:
> - p->p_sigmask = mask &~ sigcantmask;
> + proc_sigsetmask(p, mask &~ sigcantmask);
> break;
> default:
> - error = EINVAL;
> - break;
> + return (EINVAL);
> }
> - splx(s);
> - return (error);
> +
> + return (0);
> +}
> +
> +void
> +proc_sigsetmask(struct proc *p, sigset_t mask)
> +{
> + KASSERT(p == curproc);
> +
> + /* XXX: An atomic store would suffice here. */
> + (void)atomic_swap_uint(&p->p_sigmask, mask);
> +}
> +
> +/*
> + * Temporarily replace calling proc's signal mask for the duration of a
> + * system call. Original signal mask will be restored by userret().
> + */
> +void
> +proc_sigsuspend(struct proc *p, sigset_t mask)
> +{
> + KASSERT(p == curproc);
> + KASSERT(!(p->p_flag & P_SIGSUSPEND));
> +
> + p->p_oldmask = p->p_sigmask;
> + atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
> + proc_sigsetmask(p, mask);
> }
>
> /* ARGSUSED */
> int
> sys_sigpending(struct proc *p, void *v, register_t *retval)
> {
> -
> *retval = p->p_siglist;
> return (0);
> }
> @@ -489,16 +508,7 @@ sys_sigsuspend(struct proc *p, void *v,
> struct process *pr = p->p_p;
> struct sigacts *ps = pr->ps_sigacts;
>
> - /*
> - * When returning from sigpause, we want
> - * the old mask to be restored after the
> - * signal handler has finished. Thus, we
> - * save it here and mark the sigacts structure
> - * to indicate this.
> - */
> - p->p_oldmask = p->p_sigmask;
> - atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
> - p->p_sigmask = SCARG(uap, mask) &~ sigcantmask;
> + proc_sigsuspend(p, SCARG(uap, mask) &~ sigcantmask);
> while (tsleep(ps, PPAUSE|PCATCH, "pause", 0) == 0)
> /* void */;
> /* always return EINTR rather than ERESTART... */
> Index: kern/sys_generic.c
> ===================================================================
> RCS file: /home/matthew/cvs-mirror/cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.86
> diff -u -p -r1.86 sys_generic.c
> --- kern/sys_generic.c 30 Mar 2014 21:54:48 -0000 1.86
> +++ kern/sys_generic.c 7 Jul 2014 19:24:07 -0000
> @@ -658,9 +620,7 @@ dopselect(struct proc *p, int nd, fd_set
> timo = 0;
>
> if (sigmask) {
> - p->p_oldmask = p->p_sigmask;
> - atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
> - p->p_sigmask = *sigmask &~ sigcantmask;
> + proc_sigsuspend(p, *sigmask &~ sigcantmask);
> }
>
> retry:
> @@ -966,9 +926,7 @@ doppoll(struct proc *p, struct pollfd *f
> timo = 0;
>
> if (sigmask) {
> - p->p_oldmask = p->p_sigmask;
> - atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
> - p->p_sigmask = *sigmask &~ sigcantmask;
> + proc_sigsuspend(p, *sigmask &~ sigcantmask);
> }
>
> retry:
> Index: compat/linux/linux_signal.c
> ===================================================================
> RCS file: /home/matthew/cvs-mirror/cvs/src/sys/compat/linux/linux_signal.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 linux_signal.c
> --- compat/linux/linux_signal.c 26 Mar 2014 05:23:42 -0000 1.17
> +++ compat/linux/linux_signal.c 7 Jul 2014 19:24:07 -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,17 @@ linux_sys_sigprocmask(p, v, retval)
>
> linux_old_to_bsd_sigset(&ss, &bs);
>
> - s = splhigh();
> -
> switch (SCARG(uap, how)) {
> case LINUX_SIG_BLOCK:
> - p->p_sigmask |= bs & ~sigcantmask;
> + atomic_setbits_int(&p->p_sigmask, bs &~ sigcantmask);
> 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;
> + proc_sigsetmask(p, bs &~ sigcantmask);
> break;
>
> default:
> @@ -624,8 +621,6 @@ linux_sys_sigprocmask(p, v, retval)
> break;
> }
>
> - splx(s);
> -
> return (error);
> }
>
> @@ -644,7 +639,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 +661,17 @@ linux_sys_rt_sigprocmask(p, v, retval)
>
> linux_to_bsd_sigset(&ls, &bs);
>
> - s = splhigh();
> -
> switch (SCARG(uap, how)) {
> case LINUX_SIG_BLOCK:
> - p->p_sigmask |= bs & ~sigcantmask;
> + atomic_setbits_int(&p->p_sigmask, bs &~ sigcantmask);
> 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;
> + proc_sigsetmask(p, bs &~ sigcantmask);
> break;
>
> default:
> @@ -687,8 +679,6 @@ linux_sys_rt_sigprocmask(p, v, retval)
> break;
> }
>
> - splx(s);
> -
> return (error);
> }
>
> @@ -727,16 +717,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);
> + proc_sigsetmask(p, bsdsig &~ sigcantmask);
>
> return (0);
> }
> Index: arch/amd64/amd64/machdep.c
> ===================================================================
> RCS file: /home/matthew/cvs-mirror/cvs/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 machdep.c
> --- arch/amd64/amd64/machdep.c 15 Jun 2014 11:43:24 -0000 1.180
> +++ arch/amd64/amd64/machdep.c 7 Jul 2014 19:24:07 -0000
> @@ -670,7 +670,7 @@ sys_sigreturn(struct proc *p, void *v, r
> bcopy(&ksc, tf, sizeof(*tf));
>
> /* Restore signal mask. */
> - p->p_sigmask = ksc.sc_mask & ~sigcantmask;
> + proc_sigsetmask(p, ksc.sc_mask & ~sigcantmask);
>
> /*
> * sigreturn() needs to return to userspace via the 'iretq'
> Index: arch/i386/i386/machdep.c
> ===================================================================
> RCS file: /home/matthew/cvs-mirror/cvs/src/sys/arch/i386/i386/machdep.c,v
> retrieving revision 1.539
> diff -u -p -r1.539 machdep.c
> --- arch/i386/i386/machdep.c 15 Jun 2014 11:43:24 -0000 1.539
> +++ arch/i386/i386/machdep.c 7 Jul 2014 19:24:07 -0000
> @@ -2491,7 +2491,7 @@ sys_sigreturn(struct proc *p, void *v, r
> p->p_md.md_flags |= MDP_USEDFPU;
> }
>
> - p->p_sigmask = context.sc_mask & ~sigcantmask;
> + proc_sigsetmask(p, context.sc_mask & ~sigcantmask);
>
> return (EJUSTRETURN);
> }
> @@ -3990,4 +3990,3 @@ intr_handler(struct intrframe *frame, st
> #endif
> return rc;
> }
> -
>
