thread_suspend_check: don't sleep if return_instead is 1

[badger]

Summary

Originally, "return_instead" allowed a caller of thread_suspend_check to
check for thread suspension in a context where it wasn't safe to
actually suspend. Later, this check only applied to threads with
P_SINGLE_EXIT (r134498) to avoid wakeups due to fork() (at the time of
that commit, thread_single was called in fork1).

Restore the original meaning of return_instead. This is because the one
case where return_instead is actually used is a case where
thread suspension can be unsafe. If a thread enters thread_suspend_check
from sleepq_catch_signals and discovers TDF_NEEDSUSPCHK is set but no
signal is pending, it will be suspended. It does this after checking for
signals, and drops locks before suspending. With locks dropped, another
thread may deliver a signal. When the first thread awakes, it will not
check for signals again, and will go to sleep, even though a signal that
should have woken it is pending.

Test Plan

I encountered this while working on D9260. I've written a test case which
I will include with that change.

[kib]

If you remove the P_SINGLE_EXIT check in the if () condition that early, then the later return_instead checks in the function body become non-functional. More, TDF_SBDRY is in some sense 'always return_instead' bit, while with your change the return value for return_instead becomes ERESTART unconditionally.

It sounds as if the more correct fix is to not suspend in return_instead case, i.e. put the

if (return_instead)
   return (???);

right before the block which prepares to call thread_suspend_one().

[badger]

Thanks. In fact it seems like this block:

/* Should we goto user boundary if we didn't come from there? */
if (P_SHOULDSTOP(p) == P_STOPPED_SINGLE &&
    (p->p_flag & P_SINGLE_BOUNDARY) && return_instead)
        return (ERESTART);

isn't necessary at all if "return_instead" actually causes a return. I'm reasoning based on the fact that only one caller uses return_instead == 1 and I believe it's preferable to return ERESTART to that caller. Should another caller need EINTR, I would expect them to translate the error to EINTR themselves (this may be an incorrect assumption, however).

[badger]

• Remove early return that can no longer be hit. Move general case early return after more specific case.

[kib]

There is one more return_instead check, right after thread_suspend_one() call. There, the check should be removed, but the body left as is.

I disagree with the statement that the sleepq_catch_signals() code 'prefers' ERESTART error from thread_suspend_check(). Giving wrong return value there breaks restartable vs. non-restartable syscall handling.

Of course, this change is very delicate. Now, in some situations a thread which attempts to msleep(PCATCH) would get ERESTART instead of suspending. Re-reading the problem description again, I am now not sure what the real problem is. Indeed, we may race with a signal and suspend while we still have signals pending. But suspension is not supposed to be interruptible by signals, except SIGCONT in some sense. SIGCONT action should be serialized with pending suspension. In other words, it might be argued that if we have a suspension and a signal pending simultaneously and thread goes to sleep, it is fine to race and select either suspension or signal delivery. In the later case, the thread will be suspended (and possibly terminated) anyway when returning to userspace, before executing a usermode instruction.

[badger]

Here is a more detailed description of the problem:

1. Thread enters sleepq_catch_signals() with TDF_NEEDSUSPCHK but not TDF_NEEDSIGCHK set.
2. Thread skips the early call to sleepq_switch() due to TDF_NEEDSUSPCHK.
3. Thread calls cursig(), which returns 0.
4. Thread enters thread_suspend_check(), drops locks and is suspended.
5. Some other thread now signals this thread.
6. Original thread is unsuspended. "ret" from thread_suspend_check() will be 0, so it proceeds to the second sleepq_switch().
7. Thread now remains in sleepq_switch() with signal pending. It must be woken up by some other means even though the signal should have broken it out of sleep.

I'll admit that I'm somewhat nervous about properly testing this change. However I do note that sleepq_catch_signals() can already return ERESTART due to a signal that is not a member of ps->sigintr. This is why I thought this would be preferred; breaking early due to thread suspension request seems analogous to that case.

[badger]

• remove redundant conditional

[badger]

Perhaps an alternative solution would be to recheck TDF_NEEDSIGCHK after returning from thread_suspend_check(), and only in that case do a ERESTART/EINTR? (just brainstorming)

[kib]

In D9530#196998, @badger wrote:

Perhaps an alternative solution would be to recheck TDF_NEEDSIGCHK after returning from thread_suspend_check(), and only in that case do a ERESTART/EINTR? (just brainstorming)

Thank you for the explanation of the problem, now I think I got it. Yes, rechecking the flags after return from thread_suspend_check() in case of ret == 0, and restarting the function might be good idea instead. The main advantage which I see for such approach is that error code (EINTR/ERESTART) will be calculated by cursig or TD_SBDRY_ERRNO(), ie. it should be correct.

[badger]

• undo thread_suspend_check_changes
• sleepq_catch_signals: check for suspension before signals

[badger]

I first tried putting in a loop that would retry the signal check after suspension if TDF_NEEDSIGCHK was true, but that was tricky since TDF_NEEDSIGCHK can be true even when cursig() will return 0 (there is a potential loop that needs to be broken somehow).

I think the just uploaded change makes more sense. Check for suspension first, instead of checking for signals first.

[kib]

sys/kern/subr_sleepqueue.c

489 ↗

(On Diff #25015)

Since you are reindenting the lines anyway, this can be written in -3 lines as

ret = SIGISMEMBER() ? EINTR : ERESTART;

[badger]

• thread_suspend_check: don't sleep if return_instead is 1
• Remove early return that can no longer be hit. Move
• remove redundant conditional
• undo thread_suspend_check_changes
• sleepq_catch_signals: check for suspension before signals
• Use ternary conditional to save lines (note: wrapped at 80 cols)