Details

Reviewers

markj
jhb

Commits

rGf829268bcc89: Remove TDF_DOING_SA
rG5e5675cb4bfa: Remove struct proc p_singlethr member
rG2842ec6d99ce: REAP_KILL_PROC: kill processes in the threaded taskqueue context
rG5e9bba94bd7f: fork_norfproc(): unlock p1 before retrying
rG0a4f2ac3b78a: kern_sig.c: style
rGcdb58f9d04cf: ksiginfo_tryfree(): change return type to bool
rGcc29f221aaa2: ksiginfo_alloc(): change to directly take M_WAITOK/NOWAIT flags
rG5c78797e4249: reap_kill_proc_locked(): remove outdated part of the comment
rG30b16a6bcf01: exit1(): update comment about thread_single()
rGf835be58226a: sleepq_set_timeout_sbt(): correct comment to not talk about ticks
rGda39a100dbfe: sleepq_check_ast_sc_locked(): update comment
rGbd76586bb705: fork_norfproc(): style

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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Aug 15 2022, 9:24 PM

Herald added a subscriber: imp. · View Herald TranscriptAug 15 2022, 9:24 PM

kib requested review of this revision.Aug 15 2022, 9:24 PM

markj added inline comments.Aug 17 2022, 2:48 PM

sys/kern/kern_procctl.c
249	Can we use a more descriptive name than `p2`? Maybe `targetproc`.
288	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–294
370
427
538	Why not keep `w` on the stack?

kib marked 6 inline comments as done.Aug 17 2022, 4:20 PM

kib added inline comments.

sys/kern/kern_procctl.c
288	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.
538	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.

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

markj added inline comments.Aug 17 2022, 4:23 PM

sys/kern/kern_procctl.c
538	Then `w->ksi` must be dynamically allocated as well.

commit 1ee16a0f94678293613ee0eb8b872663c3f87869
Author: Konstantin Belousov <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 added inline comments.Aug 18 2022, 1:39 PM

sys/kern/kern_procctl.c
440	Why are we checking P2_WEXIT and not P_WEXIT?
535	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 marked 3 inline comments as done.Aug 18 2022, 1:49 PM

kib added inline comments.

sys/kern/kern_procctl.c
440	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.

Remove cast.
Set task context to w once.

markj added inline comments.Aug 18 2022, 1:58 PM

sys/kern/kern_procctl.c
440	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 added inline comments.Aug 18 2022, 2:04 PM

sys/kern/kern_procctl.c
440	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 added inline comments.Aug 18 2022, 7:17 PM

sys/kern/kern_procctl.c
440	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.
538	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...)