> 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);
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
