From nobody Wed Jul 26 15:22:08 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4R9yLW1pVPz4ph29; Wed, 26 Jul 2023 15:22:09 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4R9yLS6VhMz3xlp; Wed, 26 Jul 2023 15:22:08 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1690384929; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=dI8yetSxyE3kPrYXfTlBDsLfPFZhv5YWIG15QMgZAxg=; b=jsCE38TJ1B+1Aj50YP1TF+fcEm4SJYxwvCTubp9bBVTn5rWz4Zz3QmvlsNqn4d9StJGqCC 7KLXItJgFeF/s5j16dqHbq6LfjSgG7Dyz7vM6Z/ezrEI3WfpnpFSM8pIN6S9Wj2ygOgM9U ynjsCH0eCzE1mjxtgTYEm1lctZtUha8O4lhyQG5hiK/1DmAvJmRa/IPsRv5s0MYmpurPaj HcC7DoYIyv5p2FVW1KU8TylfbgzVBjxAF8iqC1InBKPXxTfXqkSJgXmsqnhSoyxBvu/GSb iAgvTvKsRDy1/sk+8nPrhAq5R4YjqNhLkXCfmfoUHrNjmYQqMq04GcHQdzifeA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1690384929; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=dI8yetSxyE3kPrYXfTlBDsLfPFZhv5YWIG15QMgZAxg=; b=gRvBmcIvsqCs0Um0PpT++Q758bbmPDogsjO4jRr5eHbTSBuXEP6896KKuwUMC1tBFKNb0L GaunZYbXJG5kf5B8Epi0VB6mbLFx5KqA+6L87l9YQTsUqwrpc4cZgbHxLTtQkmgehtqtyS 1UjEdE4+6FNpvGrzocTSjhTAM2YC8Y9+o6izCPomMuVpTvjcDYpmPiWk3oUUXOgo9Ghopq HeQkv1yxorpDw5RUb8woBsgH5MLU1BZ1A3dAAUMGq0P2OUNkDEweYDBAsxZ6wgyTMeA06z OBQ87zvnxLL5B9Tvn7xmeyjhKyrKgcQ7dr/I+Pw2UY8Ibn9El5RVyWWXZKeMiQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1690384929; a=rsa-sha256; cv=none; b=V3wTttwhTgy1BaVRbf7ZrzOw0lVg7z8h0uF/Rf/cQO4ztca3Zc0JUic1naUzKXGnujA2DX Y4M0+WwUIesvaehBCLJ7sMozzP3M1Fz7uVCGWXdBdG8Db/cdwkxYMO5efl3NtE/MjF/YeP rA9VGqs/FTvkIatZBVXxrZh3Sn8XXlbZIEFBvMdKkdZ/JlfuZKmwXaJJTdDDJlgjHOzoRs zCiBn0sQNPUWbzGwe7eZCIdAx0DlLeVfGyTXVGHbN9wUQQ6+Wst3+XUA7EqOqqkVNnUdeD uXkcBth7S/qZLEJtwDbUBDumusqXh/A/MOG8kDVxkWGUFKDf8F6/Yd9phTQ5ag== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4R9yLS5QGsznCJ; Wed, 26 Jul 2023 15:22:08 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.17.1/8.17.1) with ESMTP id 36QFM8m7017917; Wed, 26 Jul 2023 15:22:08 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.17.1/8.17.1/Submit) id 36QFM8nb017916; Wed, 26 Jul 2023 15:22:08 GMT (envelope-from git) Date: Wed, 26 Jul 2023 15:22:08 GMT Message-Id: <202307261522.36QFM8nb017916@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Konstantin Belousov Subject: git: 232b922cb363 - main - killpg(): close a race with fork(), part 2 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: kib X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 232b922cb363e01ac0dd2a277d93cf74d8485e79 Auto-Submitted: auto-generated The branch main has been updated by kib: URL: https://cgit.FreeBSD.org/src/commit/?id=232b922cb363e01ac0dd2a277d93cf74d8485e79 commit 232b922cb363e01ac0dd2a277d93cf74d8485e79 Author: Konstantin Belousov AuthorDate: 2023-07-20 21:19:03 +0000 Commit: Konstantin Belousov CommitDate: 2023-07-26 15:13:02 +0000 killpg(): close a race with fork(), part 2 When we are sending terminating signal to the group, killpg() needs to guarantee that all group members are to be terminated (it does not need to ensure that they are terminated on return from killpg()). The pg_killsx change eliminates the largest window there, but still, if a multithreaded process is signalled, the following could happen: - thread 1 is selected for the signal delivery and gets descheduled - thread 2 waits for pg_killsx lock, obtains it and forks - thread 1 continue executing and terminates the process This scenario allows the child to escape still. Fix it by single-threading forking parent if a conflict with pg_killsx is noted. We try to lock pg_killsx without sleeping, and failure to acquire it means that a parallel killpg(2) is executed. Then, stop other threads for running and in particular, receive signals, to avoid the situation explained above. Reviewed by: markj Tested by: pho Sponsored by: The FreeBSD Foundation MFC after: 1 week Differential revision: https://reviews.freebsd.org/D41128 --- sys/kern/kern_fork.c | 47 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index 81bee99fa1ca..e64a91ea5f80 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -862,7 +862,7 @@ fork1(struct thread *td, struct fork_req *fr) static int curfail; static struct timeval lastfail; int flags, pages; - bool killsx_locked; + bool killsx_locked, singlethreaded; flags = fr->fr_flags; pages = fr->fr_pages; @@ -920,6 +920,7 @@ fork1(struct thread *td, struct fork_req *fr) newproc = NULL; vm2 = NULL; killsx_locked = false; + singlethreaded = false; /* * Increment the nprocs resource before allocations occur. @@ -950,14 +951,37 @@ fork1(struct thread *td, struct fork_req *fr) } /* - * Atomically check for signals and block threads from sending - * a signal to our process group until the child is visible. + * If we are possibly multi-threaded, and there is a process + * sending a signal to our group right now, ensure that our + * other threads cannot be chosen for the signal queueing. + * Otherwise, this might delay signal action, and make the new + * child escape the signaling. */ pg = p1->p_pgrp; - if (sx_slock_sig(&pg->pg_killsx) != 0) { + if (p1->p_numthreads > 1) { + if (sx_try_slock(&pg->pg_killsx) != 0) { + killsx_locked = true; + } else { + PROC_LOCK(p1); + if (thread_single(p1, SINGLE_BOUNDARY)) { + PROC_UNLOCK(p1); + error = ERESTART; + goto fail2; + } + PROC_UNLOCK(p1); + singlethreaded = true; + } + } + + /* + * Atomically check for signals and block processes from sending + * a signal to our process group until the child is visible. + */ + if (!killsx_locked && sx_slock_sig(&pg->pg_killsx) != 0) { error = ERESTART; goto fail2; - } else if (__predict_false(p1->p_pgrp != pg || sig_intr() != 0)) { + } + if (__predict_false(p1->p_pgrp != pg || sig_intr() != 0)) { /* * Either the process was moved to other process * group, or there is pending signal. sx_slock_sig() @@ -1062,8 +1086,8 @@ fork1(struct thread *td, struct fork_req *fr) } do_fork(td, fr, newproc, td2, vm2, fp_procdesc); - sx_sunlock(&pg->pg_killsx); - return (0); + error = 0; + goto cleanup; fail0: error = EAGAIN; #ifdef MAC @@ -1081,9 +1105,16 @@ fail2: fdrop(fp_procdesc, td); } atomic_add_int(&nprocs, -1); +cleanup: if (killsx_locked) sx_sunlock(&pg->pg_killsx); - pause("fork", hz / 2); + if (singlethreaded) { + PROC_LOCK(p1); + thread_single_end(p1, SINGLE_BOUNDARY); + PROC_UNLOCK(p1); + } + if (error != 0) + pause("fork", hz / 2); return (error); }