Date: Fri, 7 Sep 2012 19:52:10 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: arch@freebsd.org Subject: Re: [PATCH] Close a race between exit1() and SIGSTOP Message-ID: <20120907165210.GC33100@deviant.kiev.zoral.com.ua> In-Reply-To: <201209071217.47439.jhb@freebsd.org> References: <201209071217.47439.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--mcEcqdJNNSWmslGf
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Fri, Sep 07, 2012 at 12:17:47PM -0400, John Baldwin wrote:
> We ran into a bug at work recently where processes were reporting an
> exit status from wait(2) of WIFSIGNALED() with WTERMSIG() of SIGSTOP.
> I narrowed this down to a race in exit1(). Specifically, exit1()
> sets p->p_xstat to the passed in exit status and sets P_WEXIT. It
> then drops the PROC_LOCK() to do other work such as freeing file
> descriptors, etc. Later it reacquires the PROC_LOCK(), marks the
> process as a zombie, and terminates. During the window where the
> PROC_LOCK() is not held, if a stop signal arrives (SIGSTOP, SIGTSTP,
> etc.), then the signal code will overwrite p->p_xstat with the signal
> that initiated the stop. The end result is that setting from SIGSTOP
> stays in p->p_xstat and is reported via wait(2).
>
> My first attempt at a fix was to simply ignore all signals once
> P_WEXIT is set by adding a check for P_WEXIT to the existing check for
> PRS_ZOMBIE. However, I think this is not quite right. For example, if
> an exiting process gets hung on an interruptible NFS mount while doing
> fdfree() I think we want a user to be able to kill the process to let
> it break out of NFS and finish exiting.
>
> The second fix I have is to explicitly clear P_STOPPED_SIGNAL to
> discard any pending SIGSTOP when setting P_WEXIT and to explicitly
> disallow any stop signals for a process that is currently exiting
> (that is, P_WEXIT is set). This fixes the race I ran into while still
> allowing other signals during process exit. The patch to do that is
> below. Below that is a test program that reproduces the issue.
>
> I would really like to get some feedback on which approach is best
> and if my concerns about allowing other signals during exit1() is a
> legitimate concern. (For some reason I feel like I've seen an argument
> for allowing that before.)
>
Argument ? Please see r163541. I hope you agree with the author, who
basically provided the same reasoning about interruptible NFS mounts.
So I agree with r163541, which log message says that I requested it.
And I do not see any other possible solution except case 2. Shouldn't
the fix actually prevent any overwrite of p_xstat for P_WEXIT process
from the signal delivery code ?
> Index: kern/kern_sig.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- kern/kern_sig.c (revision 240144)
> +++ kern/kern_sig.c (working copy)
> @@ -2134,6 +2134,8 @@
> * We try do the per-process part here.
> */
> if (P_SHOULDSTOP(p)) {
> + KASSERT(!(p->p_flag & P_WEXIT),
> + ("signal to stopped but exiting process"));
> if (sig =3D=3D SIGKILL) {
> /*
> * If traced process is already stopped,
> @@ -2248,7 +2250,7 @@
> MPASS(action =3D=3D SIG_DFL);
> =20
> if (prop & SA_STOP) {
> - if (p->p_flag & P_PPWAIT)
> + if (p->p_flag & (P_PPWAIT|P_WEXIT))
> goto out;
> p->p_flag |=3D P_STOPPED_SIG;
> p->p_xstat =3D sig;
> @@ -2410,6 +2412,7 @@
> struct proc *p =3D td->td_proc;
> =20
> PROC_LOCK_ASSERT(p, MA_OWNED);
> + KASSERT(!(p->p_flag & P_WEXIT), ("Stopping exiting process"));
> WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK,
> &p->p_mtx.lock_object, "Stopping for traced signal");
> =20
> @@ -2648,7 +2651,7 @@
> * process group, ignore tty stop signals.
> */
> if (prop & SA_STOP) {
> - if (p->p_flag & P_TRACED ||
> + if (p->p_flag & (P_TRACED|P_WEXIT) ||
> (p->p_pgrp->pg_jobc =3D=3D 0 &&
> prop & SA_TTYSTOP))
> break; /* =3D=3D ignore */
> Index: kern/kern_exit.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- kern/kern_exit.c (revision 240204)
> +++ kern/kern_exit.c (working copy)
> @@ -200,6 +200,16 @@
> _STOPEVENT(p, S_EXIT, rv);
> =20
> /*
> + * Ignore any pending request to stop due to a stop signal.
> + * Once P_WEXIT is set, future requests will be ignored as
> + * well.
> + *
> + * XXX: Is this correct?
> + */
> + p->p_flag &=3D ~P_STOPPED_SIG;
> + KASSERT(!P_SHOULDSTOP(p), ("exiting process is stopped"));
> +
> + /*
> * Note that we are exiting and do another wakeup of anyone in
> * PIOCWAIT in case they aren't listening for S_EXIT stops or
> * decided to wait again after we told them we are exiting.
>=20
>=20
> /*-
> * Test program to expose a race where SIGSTOP can be delivered to an
> * exiting process after it has set p_xstat to the exit status in
> * exit1() resulting in the p_xstat being overwritten.
> */
>=20
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <err.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <pthread.h>
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
>=20
> static volatile sig_atomic_t info;
>=20
> static void
> handler(int sig)
> {
>=20
> info =3D 1;
> }
>=20
> static void
> print_status(int status)
> {
>=20
> if (WIFCONTINUED(status))
> printf("CONTINUED");
> else if (WIFEXITED(status))
> printf("EXITED: %d", WEXITSTATUS(status));
> else if (WIFSIGNALED(status))
> printf("SIGNALED: %d%s", WTERMSIG(status), WCOREDUMP(status) ?
> " (core dumped" : "");
> else if (WIFSTOPPED(status))
> printf("STOPPED: %d", WSTOPSIG(status));
> else
> printf("UNKNOWN: %#x\n", status);
> }
>=20
> static void *
> kill_thread(void *arg)
> {
> pid_t pid;
>=20
> pid =3D (intptr_t)arg;
> for (;;) {
> if (kill(pid, SIGSTOP) < 0) {
> if (errno =3D=3D ESRCH)
> break;
> err(1, "kill(SIGSTOP)");
> }
> if (kill(pid, SIGCONT) < 0) {
> if (errno =3D=3D ESRCH)
> break;
> err(1, "kill(SIGCONT)");
> }
> }
> return (NULL);
> }
>=20
> int
> main(int ac, char **av)
> {
> pthread_t thread;
> pid_t pid, wpid;
> int count, error, status;
>=20
> if (signal(SIGINFO, handler) =3D=3D SIG_ERR)
> errx(1, "signal(SIGINFO)");
> for (count =3D 0;; count++) {
> if (info) {
> printf("count %d\n", count);
> info =3D 0;
> }
> pid =3D fork();
> switch (pid) {
> case -1:
> err(1, "fork");
> case 0:
> usleep(1000);
> exit(0);
> }
>=20
> error =3D pthread_create(&thread, NULL, kill_thread,
> (void *)(intptr_t)pid);
> if (error)
> errc(1, error, "pthread_create");
>=20
> wpid =3D waitpid(pid, &status, 0);
> if (wpid =3D=3D -1)
> err(1, "waitpid");
> if (wpid !=3D pid)
> errx(1, "waitpid mismatch");
> if (!WIFEXITED(status) || WEXITSTATUS(status) !=3D 0) {
> printf("abnormal status: ");
> print_status(status);
> printf(" after %d loops\n", count);
> }
>=20
> error =3D pthread_join(thread, NULL);
> if (error)
> errc(1, error, "pthread_join");
> }
>=20
> return (0);
> }
>=20
> --=20
> John Baldwin
> _______________________________________________
> freebsd-arch@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"
--mcEcqdJNNSWmslGf
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (FreeBSD)
iEYEARECAAYFAlBKJjoACgkQC3+MBN1Mb4idMQCgmsHdbxM3kPjDs3W9k8WZmuYY
DlUAoOz4Uz5cY6SR7rZsJsw6wXwFhAho
=NI4s
-----END PGP SIGNATURE-----
--mcEcqdJNNSWmslGf--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120907165210.GC33100>