Date: Sun, 14 Jul 2019 23:29:43 -0700 From: =?UTF-8?B?546L5piK54S2?= <msl0000023508@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: bugs@freebsd.org Subject: Re: [Bug 238837] init: Remove P_SYSTEM flag from PID 1 to allow easier debugging of init(8) Message-ID: <CAPge7yeumUb6FwT9O3Cd=gqbaavCD-d2HZPua1chwjXQW_t7hA@mail.gmail.com> In-Reply-To: <20190713190837.E899@besplex.bde.org> References: <bug-238837-227@https.bugs.freebsd.org/bugzilla/> <bug-238837-227-hBlTfh5AxA@https.bugs.freebsd.org/bugzilla/> <20190713190837.E899@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@optusnet.com.au>:
> On Sat, 13 Jul 2019 a bug that doesn't want replies@freebsd.org wrote:
>
>> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=238837
>>
>> --- Comment #3 from WHR <msl0000023508@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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPge7yeumUb6FwT9O3Cd=gqbaavCD-d2HZPua1chwjXQW_t7hA>