Date: Wed, 16 Dec 2015 16:11:37 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com>, FreeBSD Current <freebsd-current@freebsd.org> Subject: Re: fork_findpid() - Fatal trap 12: page fault while in kernel mode Message-ID: <20151216141137.GX3625@kib.kiev.ua> In-Reply-To: <20151216135427.GA19658@dft-labs.eu> References: <20151215174238.2d7cc3bb@fabiankeil.de> <20151216104227.GS3625@kib.kiev.ua> <20151216122116.09e1b27d@fabiankeil.de> <20151216121000.GV3625@kib.kiev.ua> <20151216135427.GA19658@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151216141137.GX3625>