| Message ID | 87ef45axa4.fsf_-_@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : Removing saved_sigmask | expand |
On Fri, Jun 7, 2019 at 2:41 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > The sigsuspend system call overrides the signal mask just > like all of the other users of set_user_sigmask, so convert > it to use the same helpers. Me likey. Whole series looks good to me, but that's just from looking at the patches. Maybe testing shows problems.. Linus
On 06/07, Eric W. Biederman wrote: > > +static int set_sigmask(sigset_t *kmask) > +{ > + set_restore_sigmask(); > + current->saved_sigmask = current->blocked; > + set_current_blocked(kmask); > + > + return 0; > +} I was going to do the same change except my version returns void ;) So ACK. As for 2-5, sorry I can't read them today, will do tomorrow. But at first glance... yes, we can remove TIF_RESTORE_SIGMASK. As for "remove saved_sigmask" I have some concerns... At least this means a user-visible change iiuc. Say, pselect unblocks a fatal signal. Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask(). Before this change the process will be killed. After this change it will be killed or not. It won't be killed if do_select() finds an already ready fd without blocking, or it finds a ready fd right after SIGINT interrupts poll_schedule_timeout(). And _to me_ the new behaviour makes more sense. But when it comes to user-visible changes you can never know if it breaks something or not. Oleg.
Oleg Nesterov <oleg@redhat.com> writes: > On 06/07, Eric W. Biederman wrote: >> >> +static int set_sigmask(sigset_t *kmask) >> +{ >> + set_restore_sigmask(); >> + current->saved_sigmask = current->blocked; >> + set_current_blocked(kmask); >> + >> + return 0; >> +} > > I was going to do the same change except my version returns void ;) > > So ACK. > > > As for 2-5, sorry I can't read them today, will do tomorrow. > > But at first glance... yes, we can remove TIF_RESTORE_SIGMASK. > > As for "remove saved_sigmask" I have some concerns... At least this > means a user-visible change iiuc. Say, pselect unblocks a fatal signal. > Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask(). > > Before this change the process will be killed. > > After this change it will be killed or not. It won't be killed if > do_select() finds an already ready fd without blocking, or it finds a > ready fd right after SIGINT interrupts poll_schedule_timeout(). Yes. Because having the signal set in real_blocked disables the immediate kill optimization, and the signal has to be delivered before we decide to kill the process. Which matters because as you say if nothing checks signal_pending() when the signals are unblocked we might not attempt to deliver the signal. So it is a matter of timing. If we have both a signal and a file descriptor become ready at the same time I would call that a race. Either could wake up the process and depending on the exact time we could return either one. So it is possible that today if the signal came just after the file descriptor ,the code might have made it to restore_saved_sigmask_unless, before __send_signal runs. I see the concern. I think in a matter like this we try it. Make the patches clean so people can bisect the problem. Then if someone runs into this problem we revert the offending patches. If it looks like bisection won't cleanly reveal the potential problem please let me know. Personally I don't think anyone sane would intentionally depend on this and I don't think there is a sufficiently reliable way to depend on this by accident that people would actually be depending on it. > And _to me_ the new behaviour makes more sense. But when it comes to > user-visible changes you can never know if it breaks something or not. True. The set of applications is larger than any developer can reasonably test. Eric
From: Eric W. Biederman > Sent: 10 June 2019 22:21 ... > > > > As for "remove saved_sigmask" I have some concerns... At least this > > means a user-visible change iiuc. Say, pselect unblocks a fatal signal. > > Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask(). > > > > Before this change the process will be killed. > > > > After this change it will be killed or not. It won't be killed if > > do_select() finds an already ready fd without blocking, or it finds a > > ready fd right after SIGINT interrupts poll_schedule_timeout(). > > Yes. Because having the signal set in real_blocked disables the > immediate kill optimization, and the signal has to be delivered before > we decide to kill the process. Which matters because as you say if > nothing checks signal_pending() when the signals are unblocked we might > not attempt to deliver the signal. > > So it is a matter of timing. > > If we have both a signal and a file descriptor become ready > at the same time I would call that a race. Either could > wake up the process and depending on the exact time we could > return either one. > > So it is possible that today if the signal came just after the file > descriptor ,the code might have made it to restore_saved_sigmask_unless, > before __send_signal runs. > > I see the concern. I think in a matter like this we try it. Make > the patches clean so people can bisect the problem. Then if someone > runs into this problem we revert the offending patches. If I have an application that has a loop with a pselect call that enables SIGINT (without a handler) and, for whatever reason, one of the fd is always 'ready' then I'd expect a SIGINT (from ^C) to terminate the program. A quick test program: #include <sys/time.h> #include <sys/types.h> #include <unistd.h> #include <sys/select.h> #include <signal.h> int main(int argc, char **argv) { fd_set readfds; sigset_t sig_int; struct timespec delay = {1, 0}; sigfillset(&sig_int); sigdelset(&sig_int, SIGINT); sighold(SIGINT); for (;;) { FD_ZERO(&readfds); FD_SET(0, &readfds); pselect(1, &readfds, NULL, NULL, &delay, &sig_int); poll(0,0,1000); } } Run under strace to see what is happening and send SIGINT from a different terminal. The program sleeps for a second in each of the pselect() and poll() calls. Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND. Run again, this time press enter - making fd 0 readable. pselect() returns 1, but the program still exits. (Tested on a 5.1.0-rc5 kernel.) If a signal handler were defined it should be called instead. FWIW is ERESTARTNOHAND actually sane here? If I've used setitimer() to get SIGALARM generated every second I'd expect select() to return EINTR every second even if I don't have a SIGALARM handler? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: David Laight > Sent: 11 June 2019 10:52 ... > If I have an application that has a loop with a pselect call that > enables SIGINT (without a handler) and, for whatever reason, > one of the fd is always 'ready' then I'd expect a SIGINT > (from ^C) to terminate the program. > > A quick test program: > > #include <sys/time.h> > #include <sys/types.h> > #include <unistd.h> > > #include <sys/select.h> > #include <signal.h> > > int main(int argc, char **argv) > { > fd_set readfds; > sigset_t sig_int; > struct timespec delay = {1, 0}; > > sigfillset(&sig_int); > sigdelset(&sig_int, SIGINT); > > sighold(SIGINT); > > for (;;) { > FD_ZERO(&readfds); > FD_SET(0, &readfds); > pselect(1, &readfds, NULL, NULL, &delay, &sig_int); > > poll(0,0,1000); > } > } > > Run under strace to see what is happening and send SIGINT from a different terminal. > The program sleeps for a second in each of the pselect() and poll() calls. > Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND. > > Run again, this time press enter - making fd 0 readable. > pselect() returns 1, but the program still exits. > (Tested on a 5.1.0-rc5 kernel.) > > If a signal handler were defined it should be called instead. If I add a signal handler for SIGINT it is called when pselect() returns regardless of the return value. If I setup SIGUSR1/2 the same way as SIGINT and get the SIGINT handler to sighold() and then raise both of them, the USR1/2 handlers are both called on the next pselect() call. (Without the extra sighold() the handlers are called when kill() returns.) I'd expect the epoll functions to work the same way. sigtimedwait is different though - it returns the number of the pending signal (and doesn't call the handler). So if two signals are pending neither handler should be called. The second signal would be returned on the following sigtimedwait call. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: David Laight > Sent: 11 June 2019 10:52 ... > FWIW is ERESTARTNOHAND actually sane here? > If I've used setitimer() to get SIGALARM generated every second I'd > expect select() to return EINTR every second even if I don't > have a SIGALARM handler? Actually no - after sigset(SIGALRM, SIG_IGN) I'd expect absolutely nothing to happen when kill(pid, SIGALRM) is called. However if I run: struct itimerval itimerval = {{1, 0}, {1, 0}}; setitimer(ITIMER_REAL, &itimerval, NULL); sigset(SIGALRM, SIG_IGN); poll(0, 0, big_timeout); I see (with strace) poll() repeatedly returning ERESTART_RESTARTBLOCK and being restarted. Replacing poll() with pselect() returns ERESTARTNOHAND. (In both cases the timeout must be updated since the application does see the timeout.) I'm sure other unix kernels completely ignore signals set to SIG_IGN. So this restart dance just isn't needed. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 06/10, Eric W. Biederman wrote: > > Personally I don't think anyone sane would intentionally depend on this > and I don't think there is a sufficiently reliable way to depend on this > by accident that people would actually be depending on it. Agreed. As I said I like these changes and I see nothing wrong. To me they fix the current behaviour, or at least make it more consistent. But perhaps this should be documented in the changelog? To make it clear that this change was intentional. Oleg.
Oleg Nesterov <oleg@redhat.com> writes: > On 06/10, Eric W. Biederman wrote: >> >> Personally I don't think anyone sane would intentionally depend on this >> and I don't think there is a sufficiently reliable way to depend on this >> by accident that people would actually be depending on it. > > Agreed. > > As I said I like these changes and I see nothing wrong. To me they fix the > current behaviour, or at least make it more consistent. > > But perhaps this should be documented in the changelog? To make it clear > that this change was intentional. Good point. I had not documented it because I thought I was only disabling an optimization. Eric
From: Oleg Nesterov [mailto:oleg@redhat.com] > Sent: 11 June 2019 19:56 > On 06/10, Eric W. Biederman wrote: > > > > Personally I don't think anyone sane would intentionally depend on this > > and I don't think there is a sufficiently reliable way to depend on this > > by accident that people would actually be depending on it. > > Agreed. > > As I said I like these changes and I see nothing wrong. To me they fix the > current behaviour, or at least make it more consistent. > > But perhaps this should be documented in the changelog? To make it clear > that this change was intentional. What happens if you run the test program I posted yesterday after the changes? It looks like pselect() and epoll_pwait() operated completely differently. pselect() would always calls the signal handlers. epoll_pwait() only calls them when EINTR is returned. So changing epoll_pwait() and pselect() to work the same way is bound to break some applications. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
David Laight <David.Laight@ACULAB.COM> writes: > From: David Laight >> Sent: 11 June 2019 10:52 > ... >> FWIW is ERESTARTNOHAND actually sane here? >> If I've used setitimer() to get SIGALARM generated every second I'd >> expect select() to return EINTR every second even if I don't >> have a SIGALARM handler? > > Actually no - after sigset(SIGALRM, SIG_IGN) I'd expect absolutely > nothing to happen when kill(pid, SIGALRM) is called. > > However if I run: > > struct itimerval itimerval = {{1, 0}, {1, 0}}; > setitimer(ITIMER_REAL, &itimerval, NULL); > sigset(SIGALRM, SIG_IGN); > > poll(0, 0, big_timeout); > > I see (with strace) poll() repeatedly returning ERESTART_RESTARTBLOCK > and being restarted. > Replacing poll() with pselect() returns ERESTARTNOHAND. > (In both cases the timeout must be updated since the application > does see the timeout.) > > I'm sure other unix kernels completely ignore signals set to SIG_IGN. > So this restart dance just isn't needed. We also ignore such signals except when the signal is blocked when it is received. We don't currently but it would be perfectly legitimate for set_current_blocked to dequeue and drop signals that have become unblocked whose handler is SIG_IGN. The dance would still be needed for the rare case where TIF_SIGPENDING gets set for a non-signal. Dropping the signal while it is blocked if it's handler is SIG_IGN looks like a bad idea. Eric
David Laight <David.Laight@ACULAB.COM> writes: > From: David Laight >> Sent: 11 June 2019 10:52 > ... >> If I have an application that has a loop with a pselect call that >> enables SIGINT (without a handler) and, for whatever reason, >> one of the fd is always 'ready' then I'd expect a SIGINT >> (from ^C) to terminate the program. >> >> A quick test program: >> >> #include <sys/time.h> >> #include <sys/types.h> >> #include <unistd.h> >> >> #include <sys/select.h> >> #include <signal.h> >> >> int main(int argc, char **argv) >> { >> fd_set readfds; >> sigset_t sig_int; >> struct timespec delay = {1, 0}; >> >> sigfillset(&sig_int); >> sigdelset(&sig_int, SIGINT); >> >> sighold(SIGINT); >> >> for (;;) { >> FD_ZERO(&readfds); >> FD_SET(0, &readfds); >> pselect(1, &readfds, NULL, NULL, &delay, &sig_int); >> >> poll(0,0,1000); >> } >> } >> >> Run under strace to see what is happening and send SIGINT from a different terminal. >> The program sleeps for a second in each of the pselect() and poll() calls. >> Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND. >> >> Run again, this time press enter - making fd 0 readable. >> pselect() returns 1, but the program still exits. >> (Tested on a 5.1.0-rc5 kernel.) >> >> If a signal handler were defined it should be called instead. > > If I add a signal handler for SIGINT it is called when pselect() > returns regardless of the return value. That is odd. Is this with Oleg's fix applied? Eric
David Laight <David.Laight@ACULAB.COM> writes: > From: Oleg Nesterov [mailto:oleg@redhat.com] >> Sent: 11 June 2019 19:56 >> On 06/10, Eric W. Biederman wrote: >> > >> > Personally I don't think anyone sane would intentionally depend on this >> > and I don't think there is a sufficiently reliable way to depend on this >> > by accident that people would actually be depending on it. >> >> Agreed. >> >> As I said I like these changes and I see nothing wrong. To me they fix the >> current behaviour, or at least make it more consistent. >> >> But perhaps this should be documented in the changelog? To make it clear >> that this change was intentional. > > What happens if you run the test program I posted yesterday after the changes? > > It looks like pselect() and epoll_pwait() operated completely differently. > pselect() would always calls the signal handlers. > epoll_pwait() only calls them when EINTR is returned. > So changing epoll_pwait() and pselect() to work the same way > is bound to break some applications. That is not the change we are discussing. We are looking at making pselect and epoll_pwait act a little more like sigtimedwait. In particular we are discussiong signals whose handler is SIG_DFL and whose default disposition is to kill the process, such as SIGINT. When those signals are delivered and they are not blocked, we take an optimized path in complete_signal and start the process tear down. That early start of process tear down does not happen if the signal is blocked or it happens to be in real_blocked (from sigtimedwait). This matters is the small time window where the signal is received while the process has temporarily unblocked the signal, and the signal is not detected by the code and blocked and oridinarily would not be delivered until next time because of restore_sigmask_unless. If the tear down has started early. Even if we would not have returned the process normally the signal can kill the process. AKA epoll_pwait can today result in it's caller being killed without -EINTR being returned. My change fixes that behavior and disables the early process tear down for those signals, by having real_blocked match the set of signals that are normally blocked by the process. The result should be the signal will have to wait for the next call that temporarily unblocked the process. The real benefit is that that the code becomes more comprehensible. It is my patch that titled: "signal: Always keep real_blocked in sync with blocked" that causes that to happen. Eric
From: Eric W. Biederman > Sent: 12 June 2019 13:56 > David Laight <David.Laight@ACULAB.COM> writes: > > > From: David Laight > >> Sent: 11 June 2019 10:52 > > ... > >> If I have an application that has a loop with a pselect call that > >> enables SIGINT (without a handler) and, for whatever reason, > >> one of the fd is always 'ready' then I'd expect a SIGINT > >> (from ^C) to terminate the program. > >> > >> A quick test program: > >> > >> #include <sys/time.h> > >> #include <sys/types.h> > >> #include <unistd.h> > >> > >> #include <sys/select.h> > >> #include <signal.h> > >> > >> int main(int argc, char **argv) > >> { > >> fd_set readfds; > >> sigset_t sig_int; > >> struct timespec delay = {1, 0}; > >> > >> sigfillset(&sig_int); > >> sigdelset(&sig_int, SIGINT); > >> > >> sighold(SIGINT); > >> > >> for (;;) { > >> FD_ZERO(&readfds); > >> FD_SET(0, &readfds); > >> pselect(1, &readfds, NULL, NULL, &delay, &sig_int); > >> > >> poll(0,0,1000); > >> } > >> } > >> > >> Run under strace to see what is happening and send SIGINT from a different terminal. > >> The program sleeps for a second in each of the pselect() and poll() calls. > >> Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND. > >> > >> Run again, this time press enter - making fd 0 readable. > >> pselect() returns 1, but the program still exits. > >> (Tested on a 5.1.0-rc5 kernel.) > >> > >> If a signal handler were defined it should be called instead. > > > > If I add a signal handler for SIGINT it is called when pselect() > > returns regardless of the return value. > > That is odd. Is this with Oleg's fix applied? No it is a 5.1.0-rc5 kernel with no related local patches. So it is the 'historic' behaviour of pselect(). But not the original one! Under 2.6.22-5-31 the signal handler isn't caller when pselect() returns 1. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 06/12, David Laight wrote: > > > > If I add a signal handler for SIGINT it is called when pselect() > > > returns regardless of the return value. > > > > That is odd. Is this with Oleg's fix applied? > > No it is a 5.1.0-rc5 kernel with no related local patches. > So it is the 'historic' behaviour of pselect(). No, this is not historic behaviour, > But not the original one! Under 2.6.22-5-31 the signal handler isn't caller > when pselect() returns 1. This is historic behaviour. And it was broken by 854a6ed56839a4 ("signal: Add restore_user_sigmask()"). And this is what we already discussed many, many times in this thread ;) Oleg.
From: Oleg Nesterov > Sent: 12 June 2019 14:35 > On 06/12, David Laight wrote: > > > > > > If I add a signal handler for SIGINT it is called when pselect() > > > > returns regardless of the return value. > > > > > > That is odd. Is this with Oleg's fix applied? > > > > No it is a 5.1.0-rc5 kernel with no related local patches. > > So it is the 'historic' behaviour of pselect(). > > No, this is not historic behaviour, > > > But not the original one! Under 2.6.22-5-31 the signal handler isn't caller > > when pselect() returns 1. > > This is historic behaviour. > > And it was broken by 854a6ed56839a4 ("signal: Add restore_user_sigmask()"). > > And this is what we already discussed many, many times in this thread ;) My brain hurts :-) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 06/11, David Laight wrote: > > If I have an application that has a loop with a pselect call that > enables SIGINT (without a handler) and, for whatever reason, > one of the fd is always 'ready' then I'd expect a SIGINT > (from ^C) to terminate the program. This was never true. Before Eric's patches SIGINT can kill a process or not, depending on timing. In particular, if SIGINT was already pending before pselect() and it finds an already ready fd, the program won't terminate. After the Eric's patches SIGINT will only kill the program if pselect() does not find a ready fd. And this is much more consistent. Now we can simply say that the signal will be delivered only if pselect() fails and returns -EINTR. If it doesn't have a handler the process will be killed, otherwise the handler will be called. Oleg.
From: Oleg Nesterov > Sent: 12 June 2019 14:46 > On 06/11, David Laight wrote: > > > > If I have an application that has a loop with a pselect call that > > enables SIGINT (without a handler) and, for whatever reason, > > one of the fd is always 'ready' then I'd expect a SIGINT > > (from ^C) to terminate the program. > > This was never true. > > Before Eric's patches SIGINT can kill a process or not, depending on timing. > In particular, if SIGINT was already pending before pselect() and it finds > an already ready fd, the program won't terminate. Which matches what I see on a very old Linux system. > After the Eric's patches SIGINT will only kill the program if pselect() does > not find a ready fd. > > And this is much more consistent. Now we can simply say that the signal will > be delivered only if pselect() fails and returns -EINTR. If it doesn't have > a handler the process will be killed, otherwise the handler will be called. But is it what the standards mandate? Can anyone check how Solaris and any of the BSDs behave? I don't have access to any solaris systems (I doubt I'll get the disk to spin on the one in my garage). I can check NetBSD when I get home. The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html says: "If sigmask is not a null pointer, then the pselect() function shall replace the signal mask of the caller by the set of signals pointed to by sigmask before examining the descriptors, and shall restore the signal mask of the calling thread before returning." Note that it says 'before examining the descriptors' not 'before blocking'. Under the general description about signals it also says that the signal handler will be called (or other action happen) when a pending signal is unblocked. So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors should be enough to cause the process to exit. The fact that signal handlers are not called until 'return to user' is really an implementation choice - but (IMHO) it should appear as if they were called at the time they became unmasked. If nothing else the man pages need a note about the standards and portability. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
David Laight <David.Laight@ACULAB.COM> writes: > From: Oleg Nesterov >> Sent: 12 June 2019 14:46 >> On 06/11, David Laight wrote: >> > >> > If I have an application that has a loop with a pselect call that >> > enables SIGINT (without a handler) and, for whatever reason, >> > one of the fd is always 'ready' then I'd expect a SIGINT >> > (from ^C) to terminate the program. I think this gets into a quality of implementation. I suspect that set_user_sigmask should do: if (signal_pending()) return -ERESTARNOSIGHAND; /* -EINTR that restarts if nothing was pending */ Which should be safe as nothing has blocked yet to consume any of the timeouts, and it should ensure that none of the routines miss a signal. >> This was never true. >> >> Before Eric's patches SIGINT can kill a process or not, depending on timing. >> In particular, if SIGINT was already pending before pselect() and it finds >> an already ready fd, the program won't terminate. > > Which matches what I see on a very old Linux system. > >> After the Eric's patches SIGINT will only kill the program if pselect() does >> not find a ready fd. >> >> And this is much more consistent. Now we can simply say that the signal will >> be delivered only if pselect() fails and returns -EINTR. If it doesn't have >> a handler the process will be killed, otherwise the handler will be called. > > But is it what the standards mandate? > Can anyone check how Solaris and any of the BSDs behave? > I don't have access to any solaris systems (I doubt I'll get the disk to > spin on the one in my garage). > I can check NetBSD when I get home. > > The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html says: > "If sigmask is not a null pointer, then the pselect() function shall replace > the signal mask of the caller by the set of signals pointed to by sigmask > before examining the descriptors, and shall restore the signal mask of the > calling thread before returning." > Note that it says 'before examining the descriptors' not 'before blocking'. > Under the general description about signals it also says that the signal handler > will be called (or other action happen) when a pending signal is unblocked. > So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors > should be enough to cause the process to exit. > The fact that signal handlers are not called until 'return to user' > is really an implementation choice - but (IMHO) it should appear as if they > were called at the time they became unmasked. > > If nothing else the man pages need a note about the standards and portability. I think the entire point is so that signals can be added to an event loop in a race free way. *cough* signalfd *cough*. I think if you are setting SIGINT to SIG_DFL you would leave SIGINT unblocked so it can happen anywhere. I can see more utility in having a SIGINT handler and you block SIGINT except in your polling loop so you can do something deterministic when SIGINT comes in. Which makes it independent of my patches, but still worth fixing. Eric
On 06/12, Eric W. Biederman wrote: > David Laight <David.Laight@ACULAB.COM> writes: > > > From: Oleg Nesterov > >> Sent: 12 June 2019 14:46 > >> On 06/11, David Laight wrote: > >> > > >> > If I have an application that has a loop with a pselect call that > >> > enables SIGINT (without a handler) and, for whatever reason, > >> > one of the fd is always 'ready' then I'd expect a SIGINT > >> > (from ^C) to terminate the program. > > I think this gets into a quality of implementation. > > I suspect that set_user_sigmask should do: > if (signal_pending()) > return -ERESTARNOSIGHAND; /* -EINTR that restarts if nothing was pending */ > > Which should be safe as nothing has blocked yet to consume any of the > timeouts, and it should ensure that none of the routines miss a signal. Why? I don't think this makes any sense. Perhaps we could do this _after_ set_current_blocked() for the case when the already pending SIGINT was unblocked but a) I am not sure this would be really better and b) I think it is too late to change this. Oleg.
From: David Laight > Sent: 12 June 2019 15:18 > From: Oleg Nesterov > > Sent: 12 June 2019 14:46 > > On 06/11, David Laight wrote: > > > > > > If I have an application that has a loop with a pselect call that > > > enables SIGINT (without a handler) and, for whatever reason, > > > one of the fd is always 'ready' then I'd expect a SIGINT > > > (from ^C) to terminate the program. > > > > This was never true. > > > > Before Eric's patches SIGINT can kill a process or not, depending on timing. > > In particular, if SIGINT was already pending before pselect() and it finds > > an already ready fd, the program won't terminate. > > Which matches what I see on a very old Linux system. > > > After the Eric's patches SIGINT will only kill the program if pselect() does > > not find a ready fd. > > > > And this is much more consistent. Now we can simply say that the signal will > > be delivered only if pselect() fails and returns -EINTR. If it doesn't have > > a handler the process will be killed, otherwise the handler will be called. > > But is it what the standards mandate? > Can anyone check how Solaris and any of the BSDs behave? > I don't have access to any solaris systems (I doubt I'll get the disk to > spin on the one in my garage). > I can check NetBSD when I get home. I tested NetBSD last night. pselect() always calls the signal handlers even when an fd is ready. I'm beginning to suspect that this is the 'standards conforming' behaviour. I don't remember when pselect() was added to the ToG specs, it didn't go through XNET while I was going to the meetings. David > > The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html > says: > "If sigmask is not a null pointer, then the pselect() function shall replace > the signal mask of the caller by the set of signals pointed to by sigmask > before examining the descriptors, and shall restore the signal mask of the > calling thread before returning." > Note that it says 'before examining the descriptors' not 'before blocking'. > Under the general description about signals it also says that the signal handler > will be called (or other action happen) when a pending signal is unblocked. > So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors > should be enough to cause the process to exit. > The fact that signal handlers are not called until 'return to user' > is really an implementation choice - but (IMHO) it should appear as if they > were called at the time they became unmasked. > > If nothing else the man pages need a note about the standards and portability. > > David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 06/13, David Laight wrote: > > I tested NetBSD last night. > pselect() always calls the signal handlers even when an fd is ready. > I'm beginning to suspect that this is the 'standards conforming' behaviour. May be. May be not. I have no idea. > > The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html > > says: > > "If sigmask is not a null pointer, then the pselect() function shall replace > > the signal mask of the caller by the set of signals pointed to by sigmask > > before examining the descriptors, and shall restore the signal mask of the > > calling thread before returning." > > Note that it says 'before examining the descriptors' not 'before blocking'. And you interpret this as if a pending signal should be delivered in any case, even if pselect succeeds. Again, perhaps you are right, but to me this is simply undocumented. However, linux never did this. Until the commit 854a6ed56839 ("signal: Add restore_user_sigmask()"). This commit caused regression. We had to revert it. > > If nothing else the man pages need a note about the standards and portability. Agreed. Oleg.
From: Oleg Nesterov > Sent: 13 June 2019 10:43 > On 06/13, David Laight wrote: > > > > I tested NetBSD last night. > > pselect() always calls the signal handlers even when an fd is ready. > > I'm beginning to suspect that this is the 'standards conforming' behaviour. > > May be. May be not. I have no idea. > > > > The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html > > > says: > > > "If sigmask is not a null pointer, then the pselect() function shall replace > > > the signal mask of the caller by the set of signals pointed to by sigmask > > > before examining the descriptors, and shall restore the signal mask of the > > > calling thread before returning." > > > > Note that it says 'before examining the descriptors' not 'before blocking'. > > And you interpret this as if a pending signal should be delivered in any case, > even if pselect succeeds. Again, perhaps you are right, but to me this is simply > undocumented. This text (from http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) is moderately clear: ... if all threads within the process block delivery of the signal, the signal shall remain pending on the process until a thread calls a sigwait() function selecting that signal, a thread unblocks delivery of the signal, or the action associated with the signal is set to ignore the signal. So when pselect() 'replaces the signal mask' any pending signals should be delivered. And 'delivery' means 'call the signal handler'. All Unix systems will defer calling the signal handler until the system call returns, but this is not mandated by Posix. > However, linux never did this. Until the commit 854a6ed56839 ("signal: Add > restore_user_sigmask()"). This commit caused regression. We had to revert it. That change wasn't expected to change the behaviour... David > > > If nothing else the man pages need a note about the standards and portability. > > Agreed. > > Oleg. - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 06/13, David Laight wrote: > > > And you interpret this as if a pending signal should be delivered in any case, > > even if pselect succeeds. Again, perhaps you are right, but to me this is simply > > undocumented. > > This text (from http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) is moderately clear: > ... if all threads within the process block delivery of the signal, the signal shall > remain pending on the process until a thread calls a sigwait() function selecting that > signal, a thread unblocks delivery of the signal, or the action associated with the signal > is set to ignore the signal. > > So when pselect() 'replaces the signal mask' any pending signals should be delivered. I fail to understand this logic. > > However, linux never did this. Until the commit 854a6ed56839 ("signal: Add > > restore_user_sigmask()"). This commit caused regression. We had to revert it. > > That change wasn't expected to change the behaviour... Yes. And the changed behaviour matched your understanding of standard. We had to change it back. So what do you want from me? ;) Oleg.
diff --git a/kernel/signal.c b/kernel/signal.c index 6f72cff043f0..bfa36320a4f7 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2932,6 +2932,15 @@ int sigprocmask(int how, sigset_t *set, sigset_t *oldset) } EXPORT_SYMBOL(sigprocmask); +static int set_sigmask(sigset_t *kmask) +{ + set_restore_sigmask(); + current->saved_sigmask = current->blocked; + set_current_blocked(kmask); + + return 0; +} + /* * The api helps set app-provided sigmasks. * @@ -2952,11 +2961,7 @@ int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize) if (copy_from_user(&kmask, umask, sizeof(sigset_t))) return -EFAULT; - set_restore_sigmask(); - current->saved_sigmask = current->blocked; - set_current_blocked(&kmask); - - return 0; + return set_sigmask(&kmask); } #ifdef CONFIG_COMPAT @@ -2972,11 +2977,7 @@ int set_compat_user_sigmask(const compat_sigset_t __user *umask, if (get_compat_sigset(&kmask, umask)) return -EFAULT; - set_restore_sigmask(); - current->saved_sigmask = current->blocked; - set_current_blocked(&kmask); - - return 0; + return set_sigmask(&kmask); } #endif @@ -4409,14 +4410,10 @@ SYSCALL_DEFINE0(pause) static int sigsuspend(sigset_t *set) { - current->saved_sigmask = current->blocked; - set_current_blocked(set); - while (!signal_pending(current)) { __set_current_state(TASK_INTERRUPTIBLE); schedule(); } - set_restore_sigmask(); return -ERESTARTNOHAND; } @@ -4430,12 +4427,10 @@ SYSCALL_DEFINE2(rt_sigsuspend, sigset_t __user *, unewset, size_t, sigsetsize) { sigset_t newset; - /* XXX: Don't preclude handling different sized sigset_t's. */ - if (sigsetsize != sizeof(sigset_t)) - return -EINVAL; - - if (copy_from_user(&newset, unewset, sizeof(newset))) + set_user_sigmask(unewset, sigsetsize); + if (!unewset) return -EFAULT; + return sigsuspend(&newset); } @@ -4444,12 +4439,10 @@ COMPAT_SYSCALL_DEFINE2(rt_sigsuspend, compat_sigset_t __user *, unewset, compat_ { sigset_t newset; - /* XXX: Don't preclude handling different sized sigset_t's. */ - if (sigsetsize != sizeof(sigset_t)) - return -EINVAL; - - if (get_compat_sigset(&newset, unewset)) + set_compat_user_sigmask(unewset, sigsetsize); + if (!unewset) return -EFAULT; + return sigsuspend(&newset); } #endif @@ -4459,6 +4452,7 @@ SYSCALL_DEFINE1(sigsuspend, old_sigset_t, mask) { sigset_t blocked; siginitset(&blocked, mask); + set_sigmask(&blocked); return sigsuspend(&blocked); } #endif @@ -4467,6 +4461,7 @@ SYSCALL_DEFINE3(sigsuspend, int, unused1, int, unused2, old_sigset_t, mask) { sigset_t blocked; siginitset(&blocked, mask); + set_sigmask(&blocked); return sigsuspend(&blocked); } #endif
The sigsuspend system call overrides the signal mask just like all of the other users of set_user_sigmask, so convert it to use the same helpers. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- kernel/signal.c | 43 +++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 24 deletions(-)