From owner-freebsd-current@freebsd.org Wed Dec 16 14:11:43 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 8610AA484BD for ; Wed, 16 Dec 2015 14:11:43 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id B11D119FB for ; Wed, 16 Dec 2015 14:11:42 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id tBGEBbEp082921 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Wed, 16 Dec 2015 16:11:38 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua tBGEBbEp082921 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id tBGEBbgX082920; Wed, 16 Dec 2015 16:11:37 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 16 Dec 2015 16:11:37 +0200 From: Konstantin Belousov To: Mateusz Guzik , FreeBSD Current Subject: Re: fork_findpid() - Fatal trap 12: page fault while in kernel mode Message-ID: <20151216141137.GX3625@kib.kiev.ua> References: <20151215174238.2d7cc3bb@fabiankeil.de> <20151216104227.GS3625@kib.kiev.ua> <20151216122116.09e1b27d@fabiankeil.de> <20151216121000.GV3625@kib.kiev.ua> <20151216135427.GA19658@dft-labs.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151216135427.GA19658@dft-labs.eu> User-Agent: Mutt/1.5.24 (2015-08-30) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home 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 14:11:43 -0000 On Wed, Dec 16, 2015 at 02:54:27PM +0100, Mateusz Guzik wrote: > 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. For reused struct proc it would be enough, but not for the new allocation. Neither init nor ctr for the proc zone do not initialize p_pgrp, so you would end up with the garbage in the pointer. I think that your patch should add explcit zeroing of the member into proc_init(). > > 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