Page MenuHomeFreeBSD

thread_suspend_check: don't sleep if return_instead is 1
ClosedPublic

Authored by badger on Feb 10 2017, 5:37 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

badger retitled this revision from to thread_suspend_check: don't sleep if return_instead is 1.Feb 10 2017, 5:37 PM
badger updated this object.
badger edited the test plan for this revision. (Show Details)
badger updated this revision to Diff 24975.
badger added a reviewer: kib.Feb 10 2017, 5:46 PM
badger added a subscriber: dab.Feb 10 2017, 5:55 PM
kib edited edge metadata.Feb 10 2017, 6:26 PM

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().

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 edited edge metadata.Feb 10 2017, 7:27 PM
badger updated this revision to Diff 24983.
  • Remove early return that can no longer be hit. Move general case early return after more specific case.
kib added a comment.Feb 10 2017, 8:14 PM

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.

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 updated this revision to Diff 24987.Feb 10 2017, 8:31 PM
  • remove redundant conditional

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 added a comment.Feb 10 2017, 8:44 PM

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 updated this revision to Diff 25015.Feb 11 2017, 4:34 PM
  • undo thread_suspend_check_changes
  • sleepq_catch_signals: check for suspension before signals

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 edited edge metadata.Feb 11 2017, 4:47 PM
kib accepted this revision.
This revision is now accepted and ready to land.Feb 11 2017, 4:47 PM
kib added inline comments.Feb 11 2017, 5:00 PM
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 edited edge metadata.Feb 11 2017, 5:15 PM
badger updated this revision to Diff 25019.
  • 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)
This revision now requires review to proceed.Feb 11 2017, 5:15 PM
badger marked an inline comment as done.Feb 11 2017, 5:16 PM
This revision was automatically updated to reflect the committed changes.