NetBSD Problem Report #50318

From paul@pokey.whooppee.com  Fri Oct  9 04:32:51 2015
Return-Path: <paul@pokey.whooppee.com>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id F0AC3A64EF
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  9 Oct 2015 04:32:50 +0000 (UTC)
Message-Id: <20151009043200.C283916E7B@pokey.whooppee.com>
Date: Fri,  9 Oct 2015 12:32:00 +0800 (PHT)
From: paul@whooppee.com
Reply-To: paul@whooppee.com
To: gnats-bugs@NetBSD.org
Subject: Process p_nstopchild counter not updated in suspendsched()
X-Send-Pr-Version: 3.95

>Number:         50318
>Category:       kern
>Synopsis:       Process p_nstopchild counter not updated in suspendsched()
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    pgoyette
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Oct 09 04:35:00 +0000 2015
>Closed-Date:    Sat Nov 07 22:17:34 +0000 2015
>Last-Modified:  Sat Nov 07 22:17:34 +0000 2015
>Originator:     paul@whooppee.com
>Release:        NetBSD 7.99.21
>Organization:
+------------------+--------------------------+-------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:       |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com    |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org  |
+------------------+--------------------------+-------------------------+
>Environment:


System: NetBSD pokey.whooppee.com 7.99.21 NetBSD 7.99.21 (POKEY 2015-10-08 12:18:09) #0: Fri Oct 9 10:41:35 PHT 2015 paul@pokey.whooppee.com:/build/netbsd-local/obj/amd64/sys/arch/amd64/compile/POKEY amd64
Architecture: x86_64
Machine: amd64
>Description:
	During system shutdown, immediately before syncing the disks,
	we call suspendsched() to clean up the scheduler queues.  For
	each process, we blindly set its p_stat to SSTOP but we do
	not update the parent's count of stopped children.  There is
	thus a small window where a process (usually, init) can call
	waitpid() with an inconsistent counter.

	There doesn't seem to be much real harm done here, however
	it seems to me that we really should keep the data updated
	correctly.

>How-To-Repeat:
	Found by code inspection while investigating a related issue
	(zombie process never getting reaped).

>Fix:

Index: kern_synch.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_synch.c,v
retrieving revision 1.308
diff -u -p -r1.308 kern_synch.c
--- kern_synch.c	28 Feb 2014 10:16:51 -0000	1.308
+++ kern_synch.c	9 Oct 2015 04:25:18 -0000
@@ -985,7 +985,13 @@ suspendsched(void)
 			continue;
 		}

-		p->p_stat = SSTOP;
+/* XXX PRG */
+		if (p->p_stat != SSTOP) {
+			if (p->p_stat != SZOMB && p->p_stat != SDEAD)
+				p->p_pptr->p_nstopchild++;
+			p->p_stat = SSTOP;
+		}
+/* XXX PRG */

 		LIST_FOREACH(l, &p->p_lwps, l_sibling) {
 			if (l == curlwp)

>Release-Note:

>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/50318: Process p_nstopchild counter not updated in suspendsched()
Date: Sat, 10 Oct 2015 14:24:53 +0000

 The analysis sounds plausible to me, but it also seems as though we
 ought to

 (a) audit all accounting of p_nstopchild, and
 (b) at least identify in comments, if not formally name, every state
 transition that affects it.

 A cursory examination of the grep of p_nstopchild suggests it counts

 - SSTOP children that have not been waited, and
 - SDEAD/SZOMB children.

 So there are a few state transitions that we must worry about:

 1. Child changes p_stat to {SSTOP, SDEAD, SZOMB}.
 2. Child changes p_stat from {SSTOP, SDEAD, SZOMB}.
 3. Child is waited while stopped.
 4. Child is transferred from one parent to another.

 The following updates of p_nstopchild currently exist:

 kern_exec.c:1289:               p->p_pptr->p_nstopchild++;

    Transition 1, implements proc.12345.stopexec to stop on exec (*),
    subject of PR kern/50289 concerning p_waited.

 kern_exit.c:496:        old_parent->p_nstopchild++;

    Transition 1, when process exits and goes to SDEAD before SZOMB.

    XXX Earlier in exit1, there is logic for proc.12345.stopexit to
    stop on exit (*) which transitions p to SSTOP, but it doesn't
    adjust p_pptr->p_nstopchild.

 kern_exit.c:786:                                        parent->p_nstopchil=
 d--;

    Transition 3, implements the part of wait that picks a child.

 kern_exit.c:886:        parent->p_nstopchild--;

    Transition 2, when zombie process is freed and disassociated from
    its parent.

 kern_exit.c:964:                child->p_pptr->p_nstopchild--;
 kern_exit.c:965:                parent->p_nstopchild++;

    Transition 4, in proc_reparent.

 kern_fork.c:564:                p1->p_nstopchild++;

    Transition 1, implements proc.12345.stopfork to stop on fork (*).

 kern_lwp.c:514:         p->p_pptr->p_nstopchild--;

    Transition 2, restarts stopped lwp.  Not clear to me what the
    original p_stat is, but it is set to SACTIVE, so none of {SSTOP,
    SDEAD, SZOMB}.

 kern_sig.c:1373:                                p->p_pptr->p_nstopchild--;

    Transition 2, restarts stopped process on SIGCONT.

 kern_sig.c:1474:        p->p_pptr->p_nstopchild++;

    Transition 1, stops process on SIGSTOP.

 kern_sig.c:2192:                p->p_pptr->p_nstopchild--;

    Transition 2, restarts stopped process using ptrace.

 (*) It looks like there is some duplicated logic to stop in various
 circumstances for proc.12345.stop{exec,exit,fork}.  The differences
 are worth scrutinizing further, but are not likely to be relevant to
 your unreapable zombie problem.

 The following updates of p_stat currently exist:

 kern_exec.c:1296:               p->p_stat =3D SSTOP;

    Transition 1, proc.12345.stopexec (PR kern/50298), does
    p->p_pptr->p_nstopchild++.

 kern_exec.c:2073:               l->l_proc->p_stat =3D SSTOP;
 kern_exec.c:2144:               l->l_proc->p_stat =3D ostat;

    posix_spawn, no obvious update of l->l_proc->p_pptr->p_nstopchild,
    which looks wrong to me, but it is restored shortly afterward.
    This looks like a sketchy bookkeeping kludge, not actually stopping
    a process.

 kern_exec.c:2552:       p2->p_stat =3D SACTIVE;

    posix_spawn, new process (SIDL), can't be transition 2.

 kern_exit.c:232:                p->p_stat =3D SSTOP;

    Transition 1, proc.12345.stopexit, XXX looks like it's missing
    p->p_pptr->p_nstopchild++ (PR kern/50308).

 kern_exit.c:250:        p->p_stat =3D SDYING;
 kern_exit.c:492:        p->p_stat =3D SDEAD;
 kern_exit.c:556:        p->p_stat =3D SZOMB;

    Transition 1, process exit, does p->p_pptr->p_nstopchild++
    alongside the p->p_stat =3D SDEAD transition.

    XXX I assume it must not be stopped beforehand, so that it is not
    relevant whether p was waited, but I have not proven this
    assumption.

    XXX It does not appear that p->p_stat =3D SDEAD happens with
    p->p_lock held, which looks wrong.  What is the lock order for
    processes p_lock of different processes?  Ancestor first or
    descendant first?

 kern_exit.c:884:        p->p_stat =3D SIDL;               /* not even a zom=
 bie any more */

    Transition 2, proc_free, does p->p_pptr->p_nstopchild--.  Since it
    is always a transition from SZOMB, no need to check whether p is
    waited.

 kern_fork.c:562:                p2->p_stat =3D SSTOP;

    Transition 1, proc.12345.stopfork, does p2->p_pptr->p_nstopchild++.
    Since it is always a new process (SIDL), no need to check whether
    p2 is waited.

 kern_fork.c:570:                p2->p_stat =3D SACTIVE;

    In fork, new process (SIDL), can't be transition 2.

 kern_lwp.c:510: p->p_stat =3D SACTIVE;

    Transition 2, restart stopped lwp, does p->p_pptr->p_nstopchild--
    if p is not waited.

 kern_proc.c:739:        p->p_stat =3D SIDL;                       /* protec=
 t against others */

    Newly allocated process, can't be transition 2.

 kern_sig.c:1370:                        p->p_stat =3D SACTIVE;

    Transition 2, SIGCONT/SIGKILL to continue a stopped process, does
    p->p_pptr->p_nstopchild-- if p is not waited.

 kern_sig.c:1472:        p->p_stat =3D SSTOP;

    Transition 1, stops process on SIGSTOP, does
    p->p_pptr->p_nstopchild++.

 kern_sig.c:2187:        p->p_stat =3D SACTIVE;

    Transition 2, restarts stopped process using ptrace, does
    p->p_pptr->p_nstopchild-- if p is not waited.

 kern_synch.c:988:               p->p_stat =3D SSTOP;

    Transition 1, finally the case you noted here.

 From the above analysis, your patch looks correct to me.  But I think
 we nevertheless ought to write down formal names for all these state
 transitions.

Responsible-Changed-From-To: kern-bug-people->pgoyette
Responsible-Changed-By: pgoyette@NetBSD.org
Responsible-Changed-When: Sun, 11 Oct 2015 23:31:54 +0000
Responsible-Changed-Why:
I have the fix.


From: "Paul Goyette" <pgoyette@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/50318 CVS commit: src/sys/kern
Date: Tue, 13 Oct 2015 00:25:52 +0000

 Module Name:	src
 Committed By:	pgoyette
 Date:		Tue Oct 13 00:25:52 UTC 2015

 Modified Files:
 	src/sys/kern: kern_synch.c

 Log Message:
 When clearing out the scheduler queues during system shutdown, we move
 all processes to the SSTOP state.  Make sure we update each process's
 p_waited and the parents' p_nstopchild counters to maintain consistent
 values.  Should not make any real difference this late in the shutdown
 process, but we should still be consistent just in case.

 Fixes PR kern/50318

 Pullups will be requested for:

        NetBSD-7, -6, -6-0, -6-1, -5, -5-0, -5-1, and -5-2


 To generate a diff of this commit:
 cvs rdiff -u -r1.308 -r1.309 src/sys/kern/kern_synch.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->pending-pullups
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Tue, 13 Oct 2015 01:35:36 +0000
State-Changed-Why:
Committed to head, pending pullups


State-Changed-From-To: pending-pullups->closed
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Sat, 07 Nov 2015 22:17:34 +0000
State-Changed-Why:
Fixed, pullups completed.


>Unformatted:

Home	PR Database Search