On Tue, Feb 07, 2023 at 02:29:13PM +0100, Claudio Jeker wrote:
> > I could be wrong, but membar_exit_before_atomic(9) between `p_sigmask'
> > assignment and atomic P_SIGSUSPEND flag setting should be enough for
> > that:
> >
> > p->p_oldmask = p->p_sigmask;
> > p->p_sigmask = newmask;
> > membar_exit_before_atomic();
> > atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
> >
>
> Is this good enough? Do we need a barrier in ptsignal() as well to ensure
> that the order of operation (p_sigmask check before P_SIGSUSPEND check)
> remains in order?
>
TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
if ((q->p_sigmask & mask) != 0)
continue;
p = q;
if (q->p_flag & P_SIGSUSPEND)
break;
}
I doubt the `p_sigmask' check and P_SIGSUSPEND check could be reordered,
because we loose "p = q;" assignment if so. In additional the
membar_consumer(9) between "p = q;" and P_SIGSUSPEND flag check should
enforce P_SIGSUSPEND check be the last.
> > Otherwise, if you are concerning about serialized `p_sigmask' and
> > P_SIGSUSPEND, dosigsuspend() should be left under kernel lock:
> >
> > if (sigmask) {
> > KERNEL_LOCK();
> > dosigsuspend(p, *sigmask &~ sigcantmask);
> > KERNEL_UNLOCK();
> > }
>
> This is a hack but would allow progress. Now I would prefer if
> dosigsuspend() is made safe so that the KERNEL_LOCK is not needed. Which
> would also allow sigsuspend to be called with NOLOCK.
>
Makes sense, but sigsuspend(2) will not be unlocked together with
select(2), so dosigsuspend() could be left under kernel lock for a
while.
Index: sys/kern/kern_sig.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.304
diff -u -p -r1.304 kern_sig.c
--- sys/kern/kern_sig.c 31 Jan 2023 15:18:56 -0000 1.304
+++ sys/kern/kern_sig.c 7 Feb 2023 14:29:38 -0000
@@ -504,8 +504,9 @@ dosigsuspend(struct proc *p, sigset_t ne
KASSERT(p == curproc);
p->p_oldmask = p->p_sigmask;
- atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
p->p_sigmask = newmask;
+ membar_exit_before_atomic();
+ atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
}
/*
@@ -972,6 +973,8 @@ ptsignal(struct proc *p, int signum, enu
/* okay, could send to this thread */
p = q;
+
+ membar_consumer();
/*
* sigsuspend, sigwait, ppoll/pselect, etc?
Index: sys/kern/sys_generic.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.151
diff -u -p -r1.151 sys_generic.c
--- sys/kern/sys_generic.c 27 Dec 2022 20:13:03 -0000 1.151
+++ sys/kern/sys_generic.c 7 Feb 2023 14:29:38 -0000
@@ -596,15 +596,16 @@ dopselect(struct proc *p, int nd, fd_set
struct timespec zerots = {};
fd_mask bits[6];
fd_set *pibits[3], *pobits[3];
- int error, ncollected = 0, nevents = 0;
+ int error, nfiles, ncollected = 0, nevents = 0;
u_int ni;
if (nd < 0)
return (EINVAL);
- if (nd > p->p_fd->fd_nfiles) {
- /* forgiving; slightly wrong */
- nd = p->p_fd->fd_nfiles;
- }
+
+ nfiles = READ_ONCE(p->p_fd->fd_nfiles);
+ if (nd > nfiles)
+ nd = nfiles;
+
ni = howmany(nd, NFDBITS) * sizeof(fd_mask);
if (ni > sizeof(bits[0])) {
caddr_t mbits;
Index: sys/kern/syscalls.master
===================================================================
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.239
diff -u -p -r1.239 syscalls.master
--- sys/kern/syscalls.master 7 Jan 2023 05:24:58 -0000 1.239
+++ sys/kern/syscalls.master 7 Feb 2023 14:29:38 -0000
@@ -165,7 +165,7 @@
struct itimerval *oitv); }
70 STD NOLOCK { int sys_getitimer(int which, \
struct itimerval *itv); }
-71 STD { int sys_select(int nd, fd_set *in, fd_set *ou, \
+71 STD NOLOCK { int sys_select(int nd, fd_set *in, fd_set *ou, \
fd_set *ex, struct timeval *tv); }
72 STD NOLOCK { int sys_kevent(int fd, \
const struct kevent *changelist, int nchanges, \
@@ -233,7 +233,7 @@
109 STD { int sys_ppoll(struct pollfd *fds, \
u_int nfds, const struct timespec *ts, \
const sigset_t *mask); }
-110 STD { int sys_pselect(int nd, fd_set *in, fd_set *ou, \
+110 STD NOLOCK { int sys_pselect(int nd, fd_set *in, fd_set *ou, \
fd_set *ex, const struct timespec *ts, \
const sigset_t *mask); }
111 STD { int sys_sigsuspend(int mask); }
Index: sys/sys/proc.h
===================================================================
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.339
diff -u -p -r1.339 proc.h
--- sys/sys/proc.h 16 Jan 2023 07:09:11 -0000 1.339
+++ sys/sys/proc.h 7 Feb 2023 14:29:38 -0000
@@ -386,7 +386,7 @@ struct proc {
struct user *p_addr; /* Kernel virtual addr of u-area */
struct mdproc p_md; /* Any machine-dependent fields. */
- sigset_t p_oldmask; /* Saved mask from before sigpause */
+ sigset_t p_oldmask; /* [o] Saved mask from before sigpause */
int p_sisig; /* For core dump/debugger XXX */
union sigval p_sigval; /* For core dump/debugger XXX */
long p_sitrapno; /* For core dump/debugger XXX */
