Re: fork_findpid() - Fatal trap 12: page fault while in kernel mode

On Wed, Dec 16, 2015 at 12:21:16PM +0100, Fabian Keil wrote:
> Konstantin Belousov <kostikbel_at_gmail.com> wrote:
> > It is the values of *p and *(p->p_pgrp) that are needed, from the frame 8.
> 
> Unfortunately it's not available and apparently I removed the attempts
> to get it from the previous output.

> allproc is available and the first one matches lastpid and has an invalid
> p_pgrp, but due to trypid being optimized out as well, it's not obvious
> (to me) that it's the right process.

p_suspcount = 0, p_xthread = 0xfffff801162819a0, p_boundary_count = 0, p_pendingcnt = 0, p_itimers = 0x0, p_procdesc = 0x0, p_treeflag = 0, p_magic = 3203398350, p_osrel = 1100090, 
>   p_comm = 0xfffff800304df3c4 "privoxy",
p_pgrp = 0x618b0080,

> I've changed p's declaration to static so hopefully its value will
> be available the next time the panic occurs, but it may take a while
> until that happens.

>From the state of the process you provided, it is a new (zigote) of the
forking process, which was already linked into allproc list.  Also,
it seems that bzero part of the forking procedure was finished, but bcopy
was not yet.  The p_pgrp cannot be a pointer, it is not yet initialized.

There, we have at least one issue, since zigote is linked before the
p_pgrp is initialized, and the proctree/allproc locks are dropped.
As result, fork_findpid() accesses memory with undefined content.

It seems that the least morbid solution is to slightly extend the scope
of the allproc lock in do_fork(), to prevent fork_findpid() from working
while we did not finished copying data from old to new process.

diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 1b556be..ae04c9f 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
_at__at_ -396,13 +396,12 _at__at_ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2,
 	PROC_LOCK(p2);
 	PROC_LOCK(p1);
 
-	sx_xunlock(&allproc_lock);
-
 	bcopy(&p1->p_startcopy, &p2->p_startcopy,
 	    __rangeof(struct proc, p_startcopy, p_endcopy));
 	pargs_hold(p2->p_args);
 
 	PROC_UNLOCK(p1);
+	sx_xunlock(&allproc_lock);
 
 	bzero(&p2->p_startzero,
 	    __rangeof(struct proc, p_startzero, p_endzero));