On Thu, Jun 20, 2013 at 12:35:32PM +0200, Martin Pelikan wrote:
> > I think this is only changed on the context of curproc, so there will
> > only be one dude modifying it at a time.
> > Problem I see is if there is other code which reads p_sigmask twice,
> > since it losed the atomicity between two reads, don't know what can
> > happen.
>
> This was exactly my point. Now that we've established aligned int writes
> are fine, there's the wrong assumption that two reads from that memory
> (even created by the compiler) must have to have the same value.
>
> > On a quick look it doesn't seem to happen.
>
> Looking at the code more closely:
>
> - DIAGNOSTIC panic("postsig action") could be triggered due to this
> (maybe not, I haven't checked where is postsig() called from and if
> the user has a chance to change the sigmask, but I don't see the point
> of that panic there -- what do others think?)
> - I completely forgot about compat_linux syscalls. This new version
> fixes them as well (yes the code is duplicated, do we want to change
> that?) and then there was one useless splhigh().
>
> > Also, is there any situation where reading the old value is not ok ?
> > this would only happen if another cpu tries to check p_sigmask of
> > something else than curproc.
>
> It seems there isn't.
> --
> Martin Pelikan
>
>
> 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 10:12:40 -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);
I'm pretty sure this is not correct with your change to bs before the
switch statement.
Same for the similar code below.
> 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 10:12:41 -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 10:12:41 -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);
> }
>
> 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 10:12:41 -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
>
