On Tue, Feb 07, 2023 at 05:42:40PM +0300, Vitaliy Makkoveev wrote:
>
> > > 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.
>
Now, I want to keep dosigsuspend() under the kernel lock. To me it's
better to unlock it together with sigsuspend(2) to separate possible
signal related fallout. This still makes sense because signal mask is
the optional arg for pselect(2) only. Except the local event data
conversion, poll(2) internals are the same as select(2) internals, it
makes sense to unlock them both.
This doesn't mean I dropped dosigsuspend(), I'm still waiting feedback
for barriers.
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 8 Feb 2023 10:56:08 -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;
@@ -643,8 +644,11 @@ dopselect(struct proc *p, int nd, fd_set
}
#endif
- if (sigmask)
+ if (sigmask) {
+ KERNEL_LOCK();
dosigsuspend(p, *sigmask &~ sigcantmask);
+ KERNEL_UNLOCK();
+ }
/* Register kqueue events */
error = pselregister(p, pibits, pobits, nd, &nevents, &ncollected);
@@ -948,8 +952,11 @@ doppoll(struct proc *p, struct pollfd *f
if ((error = copyin(fds, pl, sz)) != 0)
goto bad;
- if (sigmask)
+ if (sigmask) {
+ KERNEL_LOCK();
dosigsuspend(p, *sigmask &~ sigcantmask);
+ KERNEL_UNLOCK();
+ }
/* Register kqueue events */
ppollregister(p, pl, nfds, &nevents, &ncollected);
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 8 Feb 2023 10:56:10 -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, \
@@ -230,10 +230,10 @@
u_int flags, int atflags); }
108 STD NOLOCK { int sys_pledge(const char *promises, \
const char *execpromises); }
-109 STD { int sys_ppoll(struct pollfd *fds, \
+109 STD NOLOCK { 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); }
@@ -448,7 +448,7 @@
250 STD NOLOCK { int sys_minherit(void *addr, size_t len, \
int inherit); }
251 OBSOL rfork
-252 STD { int sys_poll(struct pollfd *fds, \
+252 STD NOLOCK { int sys_poll(struct pollfd *fds, \
u_int nfds, int timeout); }
253 STD NOLOCK { int sys_issetugid(void); }
254 STD { int sys_lchown(const char *path, uid_t uid, gid_t
gid); }
