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

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.

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)

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.

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.