From nobody Wed Jul 26 15:22:06 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 4R9yLV6Y8jz4ph28; Wed, 26 Jul 2023 15:22:07 +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 4R9yLQ5SKvz3xck; Wed, 26 Jul 2023 15:22:06 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1690384927; 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=+bHgcZigZ0TZEF/+xtUFw6SDhobUruS7Hv/aUS3N9nI=; b=ZOcgzD4f4MBYbCHuJB0ggQVcq8GYauao/oi0J0lRH4llxo1+WDPeG/PWMhOZVEtSoOUNT5 MpJIOb6y9w6ge9j4IZS6LyvYzU3ff99L0GeJ/jm0VOvpjo+s4lkbplFvvuudKzVOxSiMya 1e32cHZrASvU2uMvQRfxcuKrUO+sTJ3XO/7T28l70FEX4uiXs0VUviOoyN5z5reI0235Sk doLbk2ODuiil4f7Tllh2K9D0G8VxYjGp3hRzNZSDJmKD1CN7RQYyoTxX2iX45yQeI+yHTh piMuHkfV1j/HkCy3A56JSs/PlSFgg4OmsnCaJAMllgp4OKAufGgy52b7OhW0eQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1690384927; 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=+bHgcZigZ0TZEF/+xtUFw6SDhobUruS7Hv/aUS3N9nI=; b=ciE6NOMfxQTasszvyAV8Y7PVV+QcD1M24SA9UzkMOH5UPkIeSr14xF/JmGdQO5I+ZzMTVI M6+WvJdKWi6t4PLMURMoG4FSQbxPd8loDB5Q8q53+EToCTJY0cWznA9FGNmQW7oSr7HymM qdKzGQBkyLkVzUlkmSDoyNwySbSUZlgTiGTMYc3pknY4BKUu/ErwXDq91nGdyIJjX2FwNa 2D4gHdRg7WRUYEXjDL9RHRtttVkoxkbplPVUrQEBR7JX8AtMwXHCZRlvrj+quXOCvGSV09 VBfnM5bF8j5ZZUY58MDgiVYJ+63bmoUpaePieHy6vgFr7zt4wqim657bleqVRA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1690384927; a=rsa-sha256; cv=none; b=Omhwj7I/II0zYX9bQPPaXyrU5EuA3BapkwxAztPq43vCQ1QbOBNcREyasMt8VKpHVa/BZb qy6y0oBpJLwUPJ5MNiGyoIQ5tvnfYD8IHr4QZzKNEGBQs0T0iOZv+kFCicDl0wHox4iwp8 o5NXCPst/SzcOWHd9SuuZzZwQvZH0X7rJ+eA6YC1s6vixbGnsFfOM7zhebuSmpQFLNcYR5 F2TSVMumlFIB6oDS/gntwVKV2FXZfzWGi4t/Lnd5tRFfSkDsDJo1ZE2gzVRNCLFX/6OkXA US81Mf2BYqLBkRSf9Pu/WulA8pmnbliBLqryKGU1VPu+7nQZuOmUXW1FBjZqEQ== 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 4R9yLQ3yJ8znMC; Wed, 26 Jul 2023 15:22:06 +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 36QFM6vo017870; Wed, 26 Jul 2023 15:22:06 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.17.1/8.17.1/Submit) id 36QFM6Ij017869; Wed, 26 Jul 2023 15:22:06 GMT (envelope-from git) Date: Wed, 26 Jul 2023 15:22:06 GMT Message-Id: <202307261522.36QFM6Ij017869@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: aaa924138a31 - main - Revert "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: aaa924138a31078a1742029ee2d3489aaaa11299 Auto-Submitted: auto-generated The branch main has been updated by kib: URL: https://cgit.FreeBSD.org/src/commit/?id=aaa924138a31078a1742029ee2d3489aaaa11299 commit aaa924138a31078a1742029ee2d3489aaaa11299 Author: Konstantin Belousov AuthorDate: 2023-07-20 20:59:41 +0000 Commit: Konstantin Belousov CommitDate: 2023-07-26 15:12:55 +0000 Revert "killpg(): close a race with fork(), part 2" This reverts commits 81a37995c757b4e3ad8a5c699864197fd1ebdcf5 and 565a343ae3a30bc2973182ff8dfd2fa37d7f615f. There is still a leakage of the p_killpg_cnt, some but not all sources of which were identified. Second, and more important, is that there is a fundamental issue with blocked signals having KSI_KILLPG flag set. Queueing of such signal increments p_killpg_cnt, but it cannot be decremented until the signal is delivered. If, for instance, a single-threaded process with blocked signal receives killpg-kill and executes fork(2), the fork enter check returns with ERESTART. And since signal is blocked, the condition cannot be cleared. Sponsored by: The FreeBSD Foundation MFC after: 1 week Differential revision: https://reviews.freebsd.org/D41128 --- sys/kern/kern_exit.c | 13 +------------ sys/kern/kern_fork.c | 3 +-- sys/kern/kern_sig.c | 39 ++++++--------------------------------- sys/kern/kern_thread.c | 6 +++--- sys/sys/proc.h | 2 -- sys/sys/signalvar.h | 3 +-- 6 files changed, 12 insertions(+), 54 deletions(-) diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c index a6f3ca2a2d66..a92d5cc0f642 100644 --- a/sys/kern/kern_exit.c +++ b/sys/kern/kern_exit.c @@ -220,19 +220,13 @@ proc_set_p2_wexit(struct proc *p) p->p_flag2 |= P2_WEXIT; } -void -exit1(struct thread *td, int rval, int signo) -{ - exit2(td, rval, signo, false); -} - /* * Exit: deallocate address space and other resources, change proc state to * zombie, and unlink proc from allproc and parent's lists. Save exit status * and rusage for wait(). Check for child processes and orphan them. */ void -exit2(struct thread *td, int rval, int signo, bool dec_killpg_cnt) +exit1(struct thread *td, int rval, int signo) { struct proc *p, *nq, *q, *t; struct thread *tdt; @@ -310,11 +304,6 @@ exit2(struct thread *td, int rval, int signo, bool dec_killpg_cnt) ("exit1: proc %p exiting with %d threads", p, p->p_numthreads)); racct_sub(p, RACCT_NTHR, 1); - if (dec_killpg_cnt) { - MPASS(atomic_load_int(&p->p_killpg_cnt) > 0); - atomic_add_int(&p->p_killpg_cnt, -1); - } - /* Let event handler change exit status */ p->p_xexit = rval; p->p_xsig = signo; diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index 180c96ae33ef..81bee99fa1ca 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -957,8 +957,7 @@ fork1(struct thread *td, struct fork_req *fr) if (sx_slock_sig(&pg->pg_killsx) != 0) { error = ERESTART; goto fail2; - } else if (__predict_false(p1->p_pgrp != pg || sig_intr() != 0 || - atomic_load_int(&p1->p_killpg_cnt) != 0)) { + } else 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() diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index b15ad12724f8..de42255017d8 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -120,7 +120,6 @@ static int filt_signal(struct knote *kn, long hint); static struct thread *sigtd(struct proc *p, int sig, bool fast_sigblock); static void sigqueue_start(void); static void sigfastblock_setpend(struct thread *td, bool resched); -static void sigexit1(struct thread *td, int sig, ksiginfo_t *ksi) __dead2; static uma_zone_t ksiginfo_zone = NULL; struct filterops sig_filtops = { @@ -371,15 +370,6 @@ sigqueue_start(void) TDP_OLDMASK, ast_sigsuspend); } -static void -sig_handle_killpg(struct proc *p, ksiginfo_t *ksi) -{ - if ((ksi->ksi_flags & KSI_KILLPG) != 0 && p != NULL) { - MPASS(atomic_load_int(&p->p_killpg_cnt) > 0); - atomic_add_int(&p->p_killpg_cnt, -1); - } -} - ksiginfo_t * ksiginfo_alloc(int mwait) { @@ -479,7 +469,6 @@ sigqueue_take(ksiginfo_t *ksi) p = sq->sq_proc; TAILQ_REMOVE(&sq->sq_list, ksi, ksi_link); ksi->ksi_sigq = NULL; - sig_handle_killpg(p, ksi); if (!(ksi->ksi_flags & KSI_EXT) && p != NULL) p->p_pendingcnt--; @@ -577,7 +566,6 @@ sigqueue_flush(sigqueue_t *sq) while ((ksi = TAILQ_FIRST(&sq->sq_list)) != NULL) { TAILQ_REMOVE(&sq->sq_list, ksi, ksi_link); ksi->ksi_sigq = NULL; - sig_handle_killpg(p, ksi); if (ksiginfo_tryfree(ksi) && p != NULL) p->p_pendingcnt--; } @@ -653,7 +641,6 @@ sigqueue_delete_set(sigqueue_t *sq, const sigset_t *set) if (SIGISMEMBER(*set, ksi->ksi_signo)) { TAILQ_REMOVE(&sq->sq_list, ksi, ksi_link); ksi->ksi_sigq = NULL; - sig_handle_killpg(p, ksi); if (ksiginfo_tryfree(ksi) && p != NULL) p->p_pendingcnt--; } @@ -682,7 +669,7 @@ sigqueue_delete_set_proc(struct proc *p, const sigset_t *set) PROC_LOCK_ASSERT(p, MA_OWNED); - sigqueue_init(&worklist, p); + sigqueue_init(&worklist, NULL); sigqueue_move_set(&p->p_sigqueue, &worklist, set); FOREACH_THREAD_IN_PROC(p, td0) @@ -1470,7 +1457,7 @@ kern_sigtimedwait(struct thread *td, sigset_t waitset, ksiginfo_t *ksi, #endif if (sig == SIGKILL) { proc_td_siginfo_capture(td, &ksi->ksi_info); - sigexit1(td, sig, ksi); + sigexit(td, sig); } } PROC_UNLOCK(p); @@ -1948,10 +1935,8 @@ kern_kill(struct thread *td, pid_t pid, int signum) case -1: /* broadcast signal */ return (killpg1(td, signum, 0, 1, &ksi)); case 0: /* signal own process group */ - ksi.ksi_flags |= KSI_KILLPG; return (killpg1(td, signum, 0, 0, &ksi)); default: /* negative explicit process group */ - ksi.ksi_flags |= KSI_KILLPG; return (killpg1(td, signum, -pid, 0, &ksi)); } /* NOTREACHED */ @@ -2002,7 +1987,6 @@ okillpg(struct thread *td, struct okillpg_args *uap) ksi.ksi_code = SI_USER; ksi.ksi_pid = td->td_proc->p_pid; ksi.ksi_uid = td->td_ucred->cr_ruid; - ksi.ksi_flags |= KSI_KILLPG; return (killpg1(td, uap->signum, uap->pgid, 0, &ksi)); } #endif /* COMPAT_43 */ @@ -2371,10 +2355,6 @@ tdsendsignal(struct proc *p, struct thread *td, int sig, ksiginfo_t *ksi) ret = sigqueue_add(sigqueue, sig, ksi); if (ret != 0) return (ret); - if ((ksi->ksi_flags & KSI_KILLPG) != 0) { - sx_assert(&p->p_pgrp->pg_killsx, SX_XLOCKED); - atomic_add_int(&p->p_killpg_cnt, 1); - } signotify(td); /* * Defer further processing for signals which are held, @@ -3425,7 +3405,7 @@ postsig(int sig) */ mtx_unlock(&ps->ps_mtx); proc_td_siginfo_capture(td, &ksi.ksi_info); - sigexit1(td, sig, &ksi); + sigexit(td, sig); /* NOTREACHED */ } else { /* @@ -3453,7 +3433,6 @@ postsig(int sig) if (p->p_sig == sig) { p->p_sig = 0; } - sig_handle_killpg(p, &ksi); (*p->p_sysent->sv_sendsig)(action, &ksi, &returnmask); postsig_done(sig, td, ps); } @@ -3611,8 +3590,8 @@ killproc(struct proc *p, const char *why) * If dumping core, save the signal number for the debugger. Calls exit and * does not return. */ -static void -sigexit1(struct thread *td, int sig, ksiginfo_t *ksi) +void +sigexit(struct thread *td, int sig) { struct proc *p = td->td_proc; @@ -3651,16 +3630,10 @@ sigexit1(struct thread *td, int sig, ksiginfo_t *ksi) sig & WCOREFLAG ? " (core dumped)" : ""); } else PROC_UNLOCK(p); - exit2(td, 0, sig, ksi != NULL && (ksi->ksi_flags & KSI_KILLPG) != 0); + exit1(td, 0, sig); /* NOTREACHED */ } -void -sigexit(struct thread *td, int sig) -{ - sigexit1(td, sig, NULL); -} - /* * Send queued SIGCHLD to parent when child process's state * is changed. diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c index fad1abd1be6c..67712450c128 100644 --- a/sys/kern/kern_thread.c +++ b/sys/kern/kern_thread.c @@ -99,7 +99,7 @@ _Static_assert(offsetof(struct proc, p_pid) == 0xc4, "struct proc KBI p_pid"); _Static_assert(offsetof(struct proc, p_filemon) == 0x3c8, "struct proc KBI p_filemon"); -_Static_assert(offsetof(struct proc, p_comm) == 0x3e4, +_Static_assert(offsetof(struct proc, p_comm) == 0x3e0, "struct proc KBI p_comm"); _Static_assert(offsetof(struct proc, p_emuldata) == 0x4d0, "struct proc KBI p_emuldata"); @@ -119,9 +119,9 @@ _Static_assert(offsetof(struct proc, p_pid) == 0x78, "struct proc KBI p_pid"); _Static_assert(offsetof(struct proc, p_filemon) == 0x270, "struct proc KBI p_filemon"); -_Static_assert(offsetof(struct proc, p_comm) == 0x288, +_Static_assert(offsetof(struct proc, p_comm) == 0x284, "struct proc KBI p_comm"); -_Static_assert(offsetof(struct proc, p_emuldata) == 0x31c, +_Static_assert(offsetof(struct proc, p_emuldata) == 0x318, "struct proc KBI p_emuldata"); #endif diff --git a/sys/sys/proc.h b/sys/sys/proc.h index 5c77c2d683c1..d79b7a440168 100644 --- a/sys/sys/proc.h +++ b/sys/sys/proc.h @@ -722,7 +722,6 @@ struct proc { int p_pendingexits; /* (c) Count of pending thread exits. */ struct filemon *p_filemon; /* (c) filemon-specific data. */ int p_pdeathsig; /* (c) Signal from parent on exit. */ - int p_killpg_cnt; /* End area that is zeroed on creation. */ #define p_endzero p_magic @@ -1237,7 +1236,6 @@ void userret(struct thread *, struct trapframe *); void cpu_exit(struct thread *); void exit1(struct thread *, int, int) __dead2; -void exit2(struct thread *, int, int, bool) __dead2; void cpu_copy_thread(struct thread *td, struct thread *td0); bool cpu_exec_vmspace_reuse(struct proc *p, struct vm_map *map); int cpu_fetch_syscall_args(struct thread *td); diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h index 1db8813b6bf0..251775c37259 100644 --- a/sys/sys/signalvar.h +++ b/sys/sys/signalvar.h @@ -240,8 +240,7 @@ typedef struct ksiginfo { #define KSI_SIGQ 0x08 /* Generated by sigqueue, might ret EAGAIN. */ #define KSI_HEAD 0x10 /* Insert into head, not tail. */ #define KSI_PTRACE 0x20 /* Generated by ptrace. */ -#define KSI_KILLPG 0x40 /* killpg - update p_killpg_cnt */ -#define KSI_COPYMASK (KSI_TRAP | KSI_SIGQ | KSI_PTRACE | KSI_KILLPG) +#define KSI_COPYMASK (KSI_TRAP | KSI_SIGQ | KSI_PTRACE) #define KSI_ONQ(ksi) ((ksi)->ksi_sigq != NULL)