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 */