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?

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.

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.

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.

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?

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.