[PATCH 5/5] ptrace: it is fun to strace /sbin/init

[PATCH 5/5] ptrace: it is fun to strace /sbin/init

[Oleg Nesterov]

Ptracing of /sbin/init is not allowed. Of course, this is very dangerous, but
may be useful. Introduce the kernel boot parameter to allow this.

Note that it is really dangerous, also because init can lose SIGNAL_UNKILLABLE
flag. But this is because we are not careful enough with setting signal->flags,
this should be cleanuped anyway.

Unless I missed something, ptrace_get_task_struct() is pointless. It does not
need to check "pid == 1", ptrace_attach() does this. It doesn't need tasklist.
It should be replaced with the generic find_get_task_by_vpid() which does not
exist yet.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 25/kernel/ptrace.c~5_INIT_PTRACE	2008-03-16 17:22:04.000000000 +0300
+++ 25/kernel/ptrace.c	2008-03-16 18:33:02.000000000 +0300
@@ -160,6 +160,15 @@ int ptrace_may_attach(struct task_struct
 	return !err;
 }
 
+static int allow_ptrace_init;
+
+static int __init __allow_ptrace_init(char *str)
+{
+	allow_ptrace_init = 1;
+	return 1;
+}
+__setup("init_ptrace", __allow_ptrace_init);
+
 int ptrace_attach(struct task_struct *task)
 {
 	int retval;
@@ -168,7 +177,7 @@ int ptrace_attach(struct task_struct *ta
 	audit_ptrace(task);
 
 	retval = -EPERM;
-	if (task->pid <= 1)
+	if (unlikely(is_global_init(task)) && likely(!allow_ptrace_init))
 		goto out;
 	if (same_thread_group(task, current))
 		goto out;
@@ -518,12 +527,6 @@ struct task_struct *ptrace_get_task_stru
 {
 	struct task_struct *child;
 
-	/*
-	 * Tracing init is not allowed.
-	 */
-	if (pid == 1)
-		return ERR_PTR(-EPERM);
-
 	read_lock(&tasklist_lock);
 	child = find_task_by_vpid(pid);
 	if (child)
--- 25/Documentation/kernel-parameters.txt~5_INIT_PTRACE	2008-02-15 16:58:12.000000000 +0300
+++ 25/Documentation/kernel-parameters.txt	2008-03-16 18:30:28.000000000 +0300
@@ -803,6 +803,8 @@ and is between 256 and 4096 characters. 
 			Run specified binary instead of /sbin/init as init
 			process.
 
+	init_ptrace	[KNL] Allows to ptrace init. Very dangerous. Don't use.
+
 	initcall_debug	[KNL] Trace initcalls as they are executed.  Useful
 			for working out where the kernel is dying during
 			startup.

Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init

[Roland McGrath]

> Ptracing of /sbin/init is not allowed. Of course, this is very dangerous, but
> may be useful. Introduce the kernel boot parameter to allow this.

Personally I wouldn't mind losing all the ptrace/signals special cases for
init.  (Just don't have a buggy init and expect not to crash, don't be root
and kill init, etc.)  So this is fine by me.  The conservative route of
changing it only with a boot option is the wise thing to do.

> Unless I missed something, ptrace_get_task_struct() is pointless. It does not
> need to check "pid == 1", ptrace_attach() does this. It doesn't need tasklist.

Agreed.  It's a hold-over from when there was more hair in there.

> It should be replaced with the generic find_get_task_by_vpid() which does not
> exist yet.

I didn't see enough other uses to really warrant it.  Most
find_task_by_vpid calls don't actually do get_task_struct.
Those that do want to do some other check inside rcu_read_lock
before deciding to bother with get_task_struct anyway.
So there is nothing wrong with ptrace just open-coding:

	rcu_read_lock();
	child = find_task_by_vpid(pid);
	if (child)
		get_task_struct(child);
	rcu_read_unlock();

We are on the way soon to having no arch callers of ptrace_get_task_struct
left, so only the two kernel/ptrace.c uses will survive.  (x86 and ia64
switchovers to arch_ptrace/compat_arch_ptrace are already in the pipeline,
and maybe s390 too.)  So let's worry about the cleanup removing this
function once those wither away.


Thanks,
Roland

Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init

[Oleg Nesterov]

On 03/16, Roland McGrath wrote:
>
> > Ptracing of /sbin/init is not allowed. Of course, this is very dangerous, but
> > may be useful. Introduce the kernel boot parameter to allow this.
> 
> Personally I wouldn't mind losing all the ptrace/signals special cases for
> init.  (Just don't have a buggy init and expect not to crash, don't be root
> and kill init, etc.)  So this is fine by me.  The conservative route of
> changing it only with a boot option is the wise thing to do.

Great.

> > Unless I missed something, ptrace_get_task_struct() is pointless. It does not
> > need to check "pid == 1", ptrace_attach() does this. It doesn't need tasklist.
> 
> Agreed.  It's a hold-over from when there was more hair in there.
> 
> > It should be replaced with the generic find_get_task_by_vpid() which does not
> > exist yet.
> 
> I didn't see enough other uses to really warrant it.  Most
> find_task_by_vpid calls don't actually do get_task_struct.
> Those that do want to do some other check inside rcu_read_lock
> before deciding to bother with get_task_struct anyway.
> So there is nothing wrong with ptrace just open-coding:
> 
> 	rcu_read_lock();
> 	child = find_task_by_vpid(pid);
> 	if (child)
> 		get_task_struct(child);
> 	rcu_read_unlock();

proc_task_lookup(), fill_pid(), attach_task_by_pid(), can use the new helper.

But yes sure, we can open-code this.

Oleg.

Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init

[Pavel Machek]

> Ptracing of /sbin/init is not allowed. Of course, this is very dangerous, but
> may be useful. Introduce the kernel boot parameter to allow this.
...
> @@ -803,6 +803,8 @@ and is between 256 and 4096 characters. 
>  			Run specified binary instead of /sbin/init as init
>  			process.
>  
> +	init_ptrace	[KNL] Allows to ptrace init. Very dangerous. Don't use.
> +

I don't know what ptracing init is good for, and I believe people
wanting to do this kind of special stuff can patch their own kernel...


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init

[Oleg Nesterov]

On 03/20, Pavel Machek wrote:
> 
> > Ptracing of /sbin/init is not allowed. Of course, this is very dangerous, but
> > may be useful. Introduce the kernel boot parameter to allow this.
> ...
> > @@ -803,6 +803,8 @@ and is between 256 and 4096 characters. 
> >  			Run specified binary instead of /sbin/init as init
> >  			process.
> >  
> > +	init_ptrace	[KNL] Allows to ptrace init. Very dangerous. Don't use.
> > +
> 
> I don't know what ptracing init is good for, and I believe people
> wanting to do this kind of special stuff can patch their own kernel...

Yes sure. But could you explain why this can be bad given that ptracing
init needs the explicit boot parameter? IOW, could you explain why you
don't like this small and trivial change which adds a minimal impact ?

Most of the lkml readers can easily implement most of debugging options,
do you mean there are useless? I personally like the possibility to trace
init without re-compiling the kernel, because I'd like to know why /usr/bin/top
sometimes shows init at the top.

Oleg.

Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init

[Pavel Machek]

On Thu 2008-03-20 19:57:56, Oleg Nesterov wrote:
> On 03/20, Pavel Machek wrote:
> > 
> > > Ptracing of /sbin/init is not allowed. Of course, this is very dangerous, but
> > > may be useful. Introduce the kernel boot parameter to allow this.
> > ...
> > > @@ -803,6 +803,8 @@ and is between 256 and 4096 characters. 
> > >  			Run specified binary instead of /sbin/init as init
> > >  			process.
> > >  
> > > +	init_ptrace	[KNL] Allows to ptrace init. Very dangerous. Don't use.
> > > +
> > 
> > I don't know what ptracing init is good for, and I believe people
> > wanting to do this kind of special stuff can patch their own kernel...
> 
> Yes sure. But could you explain why this can be bad given that ptracing
> init needs the explicit boot parameter? IOW, could you explain why you
> don't like this small and trivial change which adds a minimal impact?

"It can't be bad, its optional".

It is bad exactly _because_ it is optional. Anything that adds boot
parameter is *not* trivial... 

Why not add

please_randomly_corrupt_memory boot parameter? It may be useful for
something...
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init

[Oleg Nesterov]

On 03/21, Pavel Machek wrote:
>
> On Thu 2008-03-20 19:57:56, Oleg Nesterov wrote:
> > On 03/20, Pavel Machek wrote:
> > > 
> > > > Ptracing of /sbin/init is not allowed. Of course, this is very dangerous, but
> > > > may be useful. Introduce the kernel boot parameter to allow this.
> > > ...
> > > > @@ -803,6 +803,8 @@ and is between 256 and 4096 characters. 
> > > >  			Run specified binary instead of /sbin/init as init
> > > >  			process.
> > > >  
> > > > +	init_ptrace	[KNL] Allows to ptrace init. Very dangerous. Don't use.
> > > > +
> > > 
> > > I don't know what ptracing init is good for, and I believe people
> > > wanting to do this kind of special stuff can patch their own kernel...
> > 
> > Yes sure. But could you explain why this can be bad given that ptracing
> > init needs the explicit boot parameter? IOW, could you explain why you
> > don't like this small and trivial change which adds a minimal impact?
> 
> "It can't be bad, its optional".
> 
> It is bad exactly _because_ it is optional. Anything that adds boot
> parameter is *not* trivial... 

You are right. I'd prefer to make /sbin/init ptraceable unconditionally,
the root should know what it does. But this will change the historical
behaviour.

> Why not add
> 
> please_randomly_corrupt_memory boot parameter? It may be useful for
> something...
> 								Pavel

Nice argument.

Oleg.