kern: restore signal mask before ast() for pselect/ppoll

[kevans]

Summary

It's possible to take a signal after pselect/ppoll have set their return
value, but before we actually return to userland. This results in
taking a signal without reflecting it in the return value, which weakens
the guarantees provided by these functions.

Switch both to restore the signal mask before we return. If a signal
was received after the wait was over, then we'll just have the signal
queued up for the next time it comes unblocked. The modified signal
mask is retained if we were interrupted so that ast() actually handles
the signal, at which point the signal mask is restored.

Sponsored by: Klara, Inc.
Sponsored by: NetApp, Inc.

[des]

I can confirm that this fixes the test cases in D47738.

[markj]

sys/kern/sys_generic.c

1061 ↗

(On Diff #146922)

Should this be conditional on uset != NULL?

[kevans]

All of this should be conditional on uset != NULL; in the NULL case, the
old sigmask is not valid and we don't need to do anything special on the way
out.

[kib]

Is the problem caused by the fact that AST_SIG is processed before AST_SIGSUSPEND? Would reordering the AST indexes be enough?

As I understand, the complain is that signal ast handler might push the signal frame before sigsuspend handler restores the sigmask which marks the signal as blocked?

[kevans]

In D47741#1088787, @kib wrote:

Is the problem caused by the fact that AST_SIG is processed before AST_SIGSUSPEND? Would reordering the AST indexes be enough?

As I understand, the complain is that signal ast handler might push the signal frame before sigsuspend handler restores the sigmask which marks the signal as blocked?

AST_SIG has to be processed before AST_SIGSUSPEND in order for the temporary signal mask stuff to work, AFAICT, but the complaint is specifically that we can catch a signal that's only temporarily unmasked after the timeout has expired (the window from kern_poll_kfds return -> AST_SIG), so we don't get EINTR but the signal was still handled. It's even more confusing when you're trying to ktrace around this, and it looks like the signal was handled between ppoll() calls when it should be blocked.

[kevans]

It looks on inspection like Linux provides the stronger guarantee that you won't see a signal handled if it didn't interrupt the sleep, but OpenBSD does not.

[kib]

In D47741#1088794, @kevans wrote:

In D47741#1088787, @kib wrote:

Is the problem caused by the fact that AST_SIG is processed before AST_SIGSUSPEND? Would reordering the AST indexes be enough?

As I understand, the complain is that signal ast handler might push the signal frame before sigsuspend handler restores the sigmask which marks the signal as blocked?

AST_SIG has to be processed before AST_SIGSUSPEND in order for the temporary signal mask stuff to work, AFAICT,

Do you mean AST_SIGSUSPEND scheduling from kern_sigsuspend()? If yes, then perhaps we need AST_SIGSUSPEND2, which is scheduled from pselect/ppoll and processed after AST_SIG.

but the complaint is specifically that we can catch a signal that's only temporarily unmasked after the timeout has expired (the window from kern_poll_kfds return -> AST_SIG), so we don't get EINTR but the signal was still handled. It's even more confusing when you're trying to ktrace around this, and it looks like the signal was handled between ppoll() calls when it should be blocked.

Yes, the idea is to ensure that AST_SIG processing code see the blocking mask. But IMO there is no need to inline the AST_SIGSUSPEND handler.

[kevans]

In D47741#1088837, @kib wrote:

In D47741#1088794, @kevans wrote:

In D47741#1088787, @kib wrote:

Is the problem caused by the fact that AST_SIG is processed before AST_SIGSUSPEND? Would reordering the AST indexes be enough?

As I understand, the complain is that signal ast handler might push the signal frame before sigsuspend handler restores the sigmask which marks the signal as blocked?

AST_SIG has to be processed before AST_SIGSUSPEND in order for the temporary signal mask stuff to work, AFAICT,

Do you mean AST_SIGSUSPEND scheduling from kern_sigsuspend()? If yes, then perhaps we need AST_SIGSUSPEND2, which is scheduled from pselect/ppoll and processed after AST_SIG.

I mean AST_SIGSUSPEND as scheduled from here- if seltdwait()'s sleep was interrupted by a signal, then my understanding is that we need to keep the temporary signal mask installed all the way until AST_SIG is done in order to process signals that are blocked by the old signal mask, but not by the temporary signal mask.

but the complaint is specifically that we can catch a signal that's only temporarily unmasked after the timeout has expired (the window from kern_poll_kfds return -> AST_SIG), so we don't get EINTR but the signal was still handled. It's even more confusing when you're trying to ktrace around this, and it looks like the signal was handled between ppoll() calls when it should be blocked.

Yes, the idea is to ensure that AST_SIG processing code see the blocking mask. But IMO there is no need to inline the AST_SIGSUSPEND handler.

What we need is for AST_SIGSUSPEND to process before AST_SIG in this one unique case, but I don't think there's a good way to do that- would it make more sense to register AST_SIGSUSPEND2 with the same callback that runs before AST_SIG to avoid processing any of the blocked signals after the non-interrupted sleep?

[kib]

In D47741#1088847, @kevans wrote:

In D47741#1088837, @kib wrote:

In D47741#1088794, @kevans wrote:

In D47741#1088787, @kib wrote:

Is the problem caused by the fact that AST_SIG is processed before AST_SIGSUSPEND? Would reordering the AST indexes be enough?

As I understand, the complain is that signal ast handler might push the signal frame before sigsuspend handler restores the sigmask which marks the signal as blocked?

AST_SIG has to be processed before AST_SIGSUSPEND in order for the temporary signal mask stuff to work, AFAICT,

Do you mean AST_SIGSUSPEND scheduling from kern_sigsuspend()? If yes, then perhaps we need AST_SIGSUSPEND2, which is scheduled from pselect/ppoll and processed after AST_SIG.

I mean AST_SIGSUSPEND as scheduled from here- if seltdwait()'s sleep was interrupted by a signal, then my understanding is that we need to keep the temporary signal mask installed all the way until AST_SIG is done in order to process signals that are blocked by the old signal mask, but not by the temporary signal mask.

You mean that you intend is to actually deliver the signals that interrupted the select' sleep? Hmm, we never behaved this way. I think if we want to, the kern_select() function needs to grow the cursig/postsig loop. But do we really have to do this?

but the complaint is specifically that we can catch a signal that's only temporarily unmasked after the timeout has expired (the window from kern_poll_kfds return -> AST_SIG), so we don't get EINTR but the signal was still handled. It's even more confusing when you're trying to ktrace around this, and it looks like the signal was handled between ppoll() calls when it should be blocked.

Yes, the idea is to ensure that AST_SIG processing code see the blocking mask. But IMO there is no need to inline the AST_SIGSUSPEND handler.

What we need is for AST_SIGSUSPEND to process before AST_SIG in this one unique case, but I don't think there's a good way to do that- would it make more sense to register AST_SIGSUSPEND2 with the same callback that runs before AST_SIG to avoid processing any of the blocked signals after the non-interrupted sleep?

Yes, AST_SIGSUSPEND2 is exactly what I propose for now.

[kevans]

Add a new ast flag to do the same thing, using a shared handler with TDA_SIGSUSPEND. This constitutes a KBI break, so to MFC I'll likely inline ast_sigsuspend() at the call sites since they should be more or less functionally equivalent except in some weirder cases.

This will be split into two commits, one to add TDA_SIGSUSPEND_EARLY and one to use it.

[kib]

sys/sys/proc.h

497

TDA_PSELECT (in-line with TDA_SIGSUSPEND) ?

[kevans]

Rename to TDA_PSELECT, since this is generally the expected-typical behavior of pselect (and ppoll) (vs. the typical behavior of sigsuspend)