From owner-freebsd-current@freebsd.org Wed Dec 16 13:54:31 2015 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id F2439A499BA for ; Wed, 16 Dec 2015 13:54:31 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 8E0D313B4 for ; Wed, 16 Dec 2015 13:54:31 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x233.google.com with SMTP id l126so39446501wml.0 for ; Wed, 16 Dec 2015 05:54:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=unuevyHsNREHIWS5QTJVWxw3ii732iWPdT6o/bEqofw=; b=p3CrylOXBH42/OIjbVzxzJWMsw8W8h3dEoPc0BeSHpdUxRTplh3wzHuZlCHFUOngOQ JnCYRyWE7nXLkK1sqk/XXBcftTyGdNsBY0BWeGJg0cF6qGhsN8NSpyWbuQYTQn5RxP+M CRZ5tIn4kHoEfIwDUx4qCuxNujTaeqiBlXTfByW20MhhbpLq/tSjEVq3sgYW6AlVN4L2 1xLOEMOimXnqp59enwD0TUDcuyfFpOqfAE8TLxg/P0IQWlcE+T1EQcVdqwU+JNaVeurb PZANG+Lw+wgqeGP8MKU4ax7U9uUWUM01qs8sTqEAOD+NoY2c+ISbInYHfBA2uREH+jxw +0ow== X-Received: by 10.28.60.11 with SMTP id j11mr11452963wma.57.1450274070083; Wed, 16 Dec 2015 05:54:30 -0800 (PST) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id z127sm7568402wmz.19.2015.12.16.05.54.29 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 16 Dec 2015 05:54:29 -0800 (PST) Date: Wed, 16 Dec 2015 14:54:27 +0100 From: Mateusz Guzik To: Konstantin Belousov Cc: FreeBSD Current Subject: Re: fork_findpid() - Fatal trap 12: page fault while in kernel mode Message-ID: <20151216135427.GA19658@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , FreeBSD Current References: <20151215174238.2d7cc3bb@fabiankeil.de> <20151216104227.GS3625@kib.kiev.ua> <20151216122116.09e1b27d@fabiankeil.de> <20151216121000.GV3625@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20151216121000.GV3625@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Dec 2015 13:54:32 -0000 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 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