Revert "killpg(): close a race with fork(), part 2"

This reverts commits 81a37995c7 and
565a343ae3.

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

@@ -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;

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

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.

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
 

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

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)