[Bug 238837] init: Remove P_SYSTEM flag from PID 1 to allow easier debugging of init(8)

[王昊然]

Thanks for share this. The followings are my suggestions.


> But debugging also
> requires normal signal handling, perhaps including core dumps, so
> my replacements for the bogus pid tests may be too strong.
I think this could be fixed by allowing default actions be taken during
debugging of init(8), in kern/kern_sig.c:

> Don't take default actions on system processes.
Change to something like:

/*
 * Don't take default actions on system processes.
 */
if ((p == initproc && !(p->p_flag & P_TRACED)) ||
    p->p_flag & P_SYSTEM) {



> - kern_proc.c:stop_all_proc(): to disallow stopping system processes.
>    This seems right for init, and the change breaks it.
Add an additional check for init(8) seems needed, along with the 'P_SYSTEM'
checking.


2019-07-13 3:10 GMT-07:00, Bruce Evans <brde at optusnet.com.au>:
> On Sat, 13 Jul 2019 a bug that doesn't want replies at freebsd.org wrote:
>
>> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=238837
>>
>> --- Comment #3 from WHR <msl0000023508 at gmail.com> ---
>> (In reply to Conrad Meyer from comment #2)
>>
>> I knwon that ptrace(2) can't be used to debug any kernel processes,
>> allowing
>> that would hang or panic the kernel; but process 1 (init(8)) isn't a
>> kernel,
>> but an user process, right?
>>
>>> Have you tried using gdb on it with this patch?
>> Yes, I has been used a 10.3-RELEASE-p29 kernel with this patch applied,
>> for a
>> long time; I can debug init(8) in gdb(1) any time, without any problem.
>
> I removed P_SYSTEM for init 15-20 years ago in my version.  IIRC, I wanted
> this more for ktracing init than for ptrace on it.
>
> This patch also fixes some hard-coding of init's pid.  This is incomplete.
> It keeps too many restrictions on sending signals to init.  p_cansignal()
> doesn't understand the special restrictions on sending signals to init or
> even to P_SYSTEM processes.
>
> XX Index: init_main.c
> XX ===================================================================
> XX RCS file: /home/ncvs/src/sys/kern/init_main.c,v
> XX retrieving revision 1.243
> XX diff -u -2 -r1.243 init_main.c
> XX --- init_main.c	16 Jun 2004 00:26:29 -0000	1.243
> XX +++ init_main.c	22 Jul 2010 15:28:09 -0000
> XX @@ -681,5 +671,5 @@
> XX
> XX  /*
> XX - * Like kthread_create(), but runs in it's own address space.
> XX + * Like kthread_create(), but runs in its own address space.
> XX   * We do this early to reserve pid 1.
> XX   *
>
> Unrelated style fix.  Init's pid also shouldn't be hard-coded in
> comments...
>
> XX @@ -697,8 +687,8 @@
> XX  		panic("cannot fork init: %d\n", error);
> XX  	KASSERT(initproc->p_pid == 1, ("create_init: initproc->p_pid != 1"));
>
> ... but since there is this KASSERT(), the comment is not wrong.
>
> XX -	/* divorce init's credentials from the kernel's */
> XX +
> XX +	/* Divorce init's credentials from the kernel's. */
> XX  	newcred = crget();
> XX  	PROC_LOCK(initproc);
> XX -	initproc->p_flag |= P_SYSTEM;
>
> The only part that actully removes the flag.  I'm not sure if this is
> complete enough -- see the notes on the pid tests.
>
> XX  	oldcred = initproc->p_ucred;
> XX  	crcopy(newcred, oldcred);
> XX Index: kern_sig.c
> XX ===================================================================
> XX RCS file: /home/ncvs/src/sys/kern/kern_sig.c,v
> XX retrieving revision 1.281
> XX diff -u -2 -r1.281 kern_sig.c
> XX --- kern_sig.c	11 Jun 2004 11:16:23 -0000	1.281
> XX +++ kern_sig.c	15 Feb 2005 08:40:46 -0000
> XX @@ -1312,5 +1307,5 @@
> XX  		LIST_FOREACH(p, &allproc, p_list) {
> XX  			PROC_LOCK(p);
> XX -			if (p->p_pid <= 1 || p->p_flag & P_SYSTEM ||
> XX +			if (p == initproc || p->p_flag & P_SYSTEM ||
> XX  			    p == td->td_proc) {
> XX  				PROC_UNLOCK(p);
>
> All the pid tests for signals are bogus, but I kept them for init.  This
> one prevents broadcasting signals to pids <= 1 and to P_SYSTEM processes.
> When P_SYSTEM is set for init, the pid test has no effect for init.
> P_SYSTEM is still set for proc0, so the pid test has no effect when the
> pid is 0.  There are now many kernel processes with pid > 1, and the
> pid check much more obviously has no effect for these.  So P_SYSTEM should
> be set for these processes, and I think it is (kthread_create() sets it
> and I think it is never cleared).
>
> XX @@ -1343,5 +1338,5 @@
> XX  		LIST_FOREACH(p, &pgrp->pg_members, p_pglist) {
> XX  			PROC_LOCK(p);
> XX -			if (p->p_pid <= 1 || p->p_flag & P_SYSTEM) {
> XX +			if (p == initproc || p->p_flag & P_SYSTEM) {
> XX  				PROC_UNLOCK(p);
> XX  				continue;
> XX @@ -2127,5 +2170,5 @@
> XX  			 * Don't take default actions on system processes.
> XX  			 */
> XX -			if (p->p_pid <= 1) {
> XX +			if (p == initproc || p->p_flag & P_SYSTEM) {
> XX  #ifdef DIAGNOSTIC
> XX  				/*
>
> More bogus pid tests.  The last one is most interesting.  It prevents
> taking default actions for P_SYSTEM processes.  I think this prevents
> the foot shooting of killing init with -9 and the more useful behaviour
> of killing init with a signal that makes it dump core for debugging
> later.
>
> This shows the danger of simply removing the flag.  I like allowing foot
> shooting, but don't want to change the policy for it here.
>
> P_SYSTEM was and is checked in kern/*.c without a pid test in:
> - kern_switch.c:choosethread: to allow system processes to run during
>    panic().  Without this change, init was allowed to run then.  This seems
>    wrong.
> - sys_process.c:kern_ptrace(): to disallow debugging system processes.
>    This is wrong for init, and the change fixes it.  But debugging also
>    requires normal signal handling, perhaps including core dumps, so
>    my replacements for the bogus pid tests may be too strong.
>
> P_SYSTEM is now also checked in kern/*.c without a pid test in:
> - kern_proc.c:sysctl_kern_proc_args(): to disallow seeing args for
>    system processes.  This is wrong for init.
> - kern_proc.c:sysctl_kern_proc_env(): similarly for the environment.
> - kern_proc.c:sysctl_kern_proc_auxv(): similarly for the ELF aux vector.
> - kern_proc.c:stop_all_proc(): to disallow stopping system processes.
>    This seems right for init, and the change breaks it.
> - kern_procctl.c:protect_setchild(): seems to be to disallow procctl on
>    init.  This seems to need tests like the ones for signals, if any.
> - kern_racct.c:racct_proc_throttle(): to disallow throttling of system
>    processes.  The comment on this conflates system processes with kernel
>    processes, and the code is misformatted using a blank line to displace
>    half of the code covered by the comment.
>
> I said that I wanted this more for ktrace than for debugging, but ktrace
> doesn't seem to check P_SYSTEM.  It checks p_candebug().  p_candebug()
> seems to be correct for ktracing but not for debugging, since debugging
> needs ptrace which needs P_SYSTEM, but p_candebug() doesn't know that
> it needs P_SYSTEM.  Thus gdb on init is currently permitted, but fails
> in ptace().
>
> Bruce
>