Yet another round of fixes for REAP_KILL

[kib]

Summary

In particular, this fixes one syzkaller report, and another case where Peter reported unkillable process in 'T' state.

Specific commits:

commit d64f8f383004106add5ffc038bb8049d02330cd7
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Aug 12 03:37:55 2022 +0300

    fork_norfproc(): style
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      3 days

commit cacf54d2a9fd0bc2fcf7d5f29f6118644c2e4709
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Aug 12 03:19:36 2022 +0300

    sleepq_check_ast_sc_locked(): update comment
    
    The relock order is important not only for a signal delivery, but also
    for the suspension requests.
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      3 days

commit 05ebb7725ed810df466f7b10554f328223875839
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Aug 12 03:35:45 2022 +0300

    sleepq_set_timeout_sbt(): correct comment to not talk about ticks
    
    It is sbt now.  Also, explain what flags are.
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      3 days

commit 5946bbaed07483839ba256d93a580111878d7654
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Aug 12 23:16:29 2022 +0300

    exit1(): update comment about thread_single()
    
    We do not check single-threading conditions in trap, or when sleeping
    uninterruptible.
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      3 days

commit 0593efdac8346ac17319c5f095895a5fa7a8309f
Author: Konstantin Belousov <[e-mail]>
Date:   Sat Aug 13 01:12:20 2022 +0300

    reap_kill_proc_locked(): remove outdated part of the comment
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      3 days

commit 3ce79f5689d1f214c346499ab46b85a00e0d3924
Author: Konstantin Belousov <[e-mail]>
Date:   Wed Aug 10 20:03:31 2022 +0300

    fork_norfproc(): unlock p1 before retrying
    
    Reported by:    markj
    Syzkaller:      647212368c3f32c6f13f
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week

commit 2cf62f6fa16db9a7b338ba222cdc93c2a4ea12ce
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Aug 12 22:37:08 2022 +0300

    REAP_KILL_PROC: kill processes in the threaded taskqueue context
    
    There is a problem still left after the fixes to REAP_KILL_PROC.  The
    handling of the stopping signals by sig_suspend_threads() can occur
    outside the stopping process context by tdsendsignal(), and it uses
    mostly the same mechanism of aborting sleeps as suspension.  In other
    words, it badly interacts with thread_single(SINGLE_ALLPROC).
    
    But unlike single threading from the process context, we cannot wait by
    sleep for other single threading requests to pass, because we own
    spinlock(s).
    
    Fix this by moving the both the thread_single(p2, SINGLE_ALLPROC) and
    signalling, to the threaded taskqueue which cannot be single-threaded
    itself.
    
    Reported and tested by: pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 weeks

commit 56070efea6f69aa596fda84b759b82977b0ace95
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Aug 12 22:59:56 2022 +0300

    Remove struct proc p_singlethr member
    
    It does not serve any purpose after we stopped doing
    thread_single(SINGLE_ALLPROC) from stoppable user processes.
    
    Tested by:      pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 weeks

commit 12f90de3a1aca20efce26e4a1c66fbf1c7c4dbfc
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Aug 12 23:25:00 2022 +0300

    Remove TDF_DOING_SA
    
    We cannot see a thread with the flag set in unsuspend, after we stopped
    doing SINGLE_ALLPROC from user processes.
    
    Tested by:      pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 weeks

[markj]

sys/kern/kern_procctl.c

249 ↗

(On Diff #109367)

Can we use a more descriptive name than p2? Maybe targetproc.

288 ↗

(On Diff #109367)

What happens if w->p2 is waiting for the current process to single-thread itself, e.g., if two processes use REAP_KILL at the same time to target each other? Is it possible for them to deadlock?

292 ↗

(On Diff #109367)

370 ↗

(On Diff #109367)

427 ↗

(On Diff #109367)

537 ↗

(On Diff #109367)

Why not keep w on the stack?

[kib]

sys/kern/kern_procctl.c

288 ↗

(On Diff #109367)

First, there is the use of stop_all_proc_block() around REAP_KILL, which only allows one syscall to act in a time. Second, thread_single() and signalling are executed in the threaded taskqueue context, which serializes all thread_single(SINGLE_ALLPROC) blocks among REAP_KILL on the per-target-process basis.

537 ↗

(On Diff #109367)

Because we sleep waiting for taskqueue to do it's stuff, which potentially allows for the stack to be swapped out. I do not want phold the curproc for this.

[kib]

(void)pksignal()
s/p2/target/ in the struct work

[markj]

sys/kern/kern_procctl.c

537 ↗

(On Diff #109367)

Then w->ksi must be dynamically allocated as well.

[kib]

commit 1ee16a0f94678293613ee0eb8b872663c3f87869
Author: Konstantin Belousov <[e-mail]>
Date:   Wed Aug 17 20:01:13 2022 +0300

    kern_sig.c: style
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 weeks
    Differential revision:  https://reviews.freebsd.org/D36207

commit 9abe373f0689554ffe6f7948cccc211a182bad57
Author: Konstantin Belousov <[e-mail]>
Date:   Wed Aug 17 19:58:46 2022 +0300

    ksiginfo_tryfree(): change return type to bool
    
    The function result is already used as bool.
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 weeks
    Differential revision:  https://reviews.freebsd.org/D36207

commit 19c49f9db222d9aaf9859f5cd1906082749b4219
Author: Konstantin Belousov <[e-mail]>
Date:   Wed Aug 17 19:57:20 2022 +0300

    ksiginfo_alloc(): change to directly take M_WAITOK/NOWAIT flags
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 weeks
    Differential revision:  https://reviews.freebsd.org/D36207

commit e769ed275e91c6d2b6e607dd31ab262da89f7ce0
Author: Konstantin Belousov <[e-mail]>
Date:   Wed Aug 17 19:34:57 2022 +0300

    reap_kill(): allocate ksi in zone, instead of stack
    
    reap_kill_subtree() may sleep, which allows the thread to be swapped
    out, in which case ksi points to unmapped memory.
    
    Noted by:       markj
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 weeks
    Differential revision:  https://reviews.freebsd.org/D36207

[markj]

sys/kern/kern_procctl.c

440 ↗

(On Diff #109473)

Why are we checking P2_WEXIT and not P_WEXIT?

536 ↗

(On Diff #109473)

Can't we set ta_context here by passing w as the last parameter of TASK_INIT()? Rather than doing it in reap_kill_subtree_once().

sys/kern/kern_sig.c

374 ↗

(On Diff #109473)

No need for a cast here.

[kib]

sys/kern/kern_procctl.c

440 ↗

(On Diff #109473)

That is the whole purpose of P2_WEXIT. The flag is set before exit1() single-threads the exiting process, while P_WEXIT is set after. If we attempt to single-thread after P_WEXIT is set, we never succeed because there is no more any suspension points in the target process, in other words, we hang in T state.

[kib]

Remove cast.
Set task context to w once.

[markj]

sys/kern/kern_procctl.c

440 ↗

(On Diff #109473)

But after we release the proc lock here, exit1() can resume execution and single-thread the process anyway. It will block after that, since the process is held.

[kib]

sys/kern/kern_procctl.c

440 ↗

(On Diff #109473)

When we (as in taskqueue) release the process lock, we do not care anymore.

Or do you mean the lock release inside thread_single()? It is interlocked with setting P_STOPPED(), which prevents exit1() from making the progress.

Or do you mean checking P2_WEXIT there specifically? It is small optimization, avoiding the need to schedule task if we know already that it has nothing to do. We need to check for some P{,2}_WEXIT anyway, so why not employ this minor optimization?

[markj]

sys/kern/kern_procctl.c

440 ↗

(On Diff #109473)

I mean checking P2_WEXIT here specifically. It looks odd to me to check P2_WEXIT, then hold and unlock the process. But ok, it's not incorrect.

537 ↗

(On Diff #109367)

Also we need to make a copy of rk. sys_ioctl() will put small parameters (<= 128 bytes) on the stack. (I think holding curproc makes more sense at this point...)

[kib]

Hold curproc around reap_kill_subtree()