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

[Mateusz Guzik]

On Wed, Dec 16, 2015 at 02:10:00PM +0200, Konstantin Belousov wrote:
> 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
> @@ -396,13 +396,12 @@ 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));

While I agree with analysis the patch does not look right. Since the
struct is only assigned and all locks get dropped, there is nothing
preventing another thread from the forking process to change the process
group.

Interestngly very same function assigns the pointer explicitely later:
        p2->p_pgrp = p1->p_pgrp;

As such, I would argue the right solution is to move p_pgrp out of the
copied area. Exit path clears the pointer, so it should be fine to just
do that.

That is (untested):

diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 90effa6..cb94318 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -586,7 +586,6 @@ struct proc {
 	int		p_osrel;	/* (x) osreldate for the
 					       binary (from ELF note, if any) */
 	char		p_comm[MAXCOMLEN + 1];	/* (b) Process name. */
-	struct pgrp	*p_pgrp;	/* (c + e) Pointer to process group. */
 	struct sysentvec *p_sysent;	/* (b) Syscall dispatch info. */
 	struct pargs	*p_args;	/* (c) Process arguments. */
 	rlim_t		p_cpulimit;	/* (c) Current CPU limit in seconds. */
@@ -599,6 +598,7 @@ struct proc {
 	u_int		p_xsig;		/* (c) Stop/kill sig. */
 /* End area that is copied on creation. */
 #define	p_endcopy	p_xsig
+	struct pgrp	*p_pgrp;	/* (c + e) Pointer to process group. */
 	struct knlist	p_klist;	/* (c) Knotes attached to this proc. */
 	int		p_numthreads;	/* (c) Number of threads. */
 	struct mdproc	p_md;		/* Any machine-dependent fields. */

-- 
Mateusz Guzik <mjguzik gmail.com>