LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 5/5] ptrace: it is fun to strace /sbin/init
@ 2008-03-16 15:54 Oleg Nesterov
  2008-03-16 22:31 ` Roland McGrath
  2008-03-20 16:27 ` Pavel Machek
  0 siblings, 2 replies; 7+ messages in thread
From: Oleg Nesterov @ 2008-03-16 15:54 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Davide Libenzi, Eric W. Biederman, Ingo Molnar, Laurent Riffard,
	Pavel Emelyanov, Roland McGrath, linux-kernel

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init
  2008-03-16 15:54 [PATCH 5/5] ptrace: it is fun to strace /sbin/init Oleg Nesterov
@ 2008-03-16 22:31 ` Roland McGrath
  2008-03-16 23:17   ` Oleg Nesterov
  2008-03-20 16:27 ` Pavel Machek
  1 sibling, 1 reply; 7+ messages in thread
From: Roland McGrath @ 2008-03-16 22:31 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Davide Libenzi, Eric W. Biederman, Ingo Molnar,
	Laurent Riffard, Pavel Emelyanov, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init
  2008-03-16 22:31 ` Roland McGrath
@ 2008-03-16 23:17   ` Oleg Nesterov
  0 siblings, 0 replies; 7+ messages in thread
From: Oleg Nesterov @ 2008-03-16 23:17 UTC (permalink / raw)
  To: Roland McGrath
  Cc: Andrew Morton, Davide Libenzi, Eric W. Biederman, Ingo Molnar,
	Laurent Riffard, Pavel Emelyanov, linux-kernel

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init
  2008-03-16 15:54 [PATCH 5/5] ptrace: it is fun to strace /sbin/init Oleg Nesterov
  2008-03-16 22:31 ` Roland McGrath
@ 2008-03-20 16:27 ` Pavel Machek
  2008-03-20 16:57   ` Oleg Nesterov
  1 sibling, 1 reply; 7+ messages in thread
From: Pavel Machek @ 2008-03-20 16:27 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Davide Libenzi, Eric W. Biederman, Ingo Molnar,
	Laurent Riffard, Pavel Emelyanov, Roland McGrath, linux-kernel


> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init
  2008-03-20 16:27 ` Pavel Machek
@ 2008-03-20 16:57   ` Oleg Nesterov
  2008-03-20 23:25     ` Pavel Machek
  0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2008-03-20 16:57 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Andrew Morton, Davide Libenzi, Eric W. Biederman, Ingo Molnar,
	Laurent Riffard, Pavel Emelyanov, Roland McGrath, linux-kernel

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init
  2008-03-20 16:57   ` Oleg Nesterov
@ 2008-03-20 23:25     ` Pavel Machek
  2008-03-21  1:15       ` Oleg Nesterov
  0 siblings, 1 reply; 7+ messages in thread
From: Pavel Machek @ 2008-03-20 23:25 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Davide Libenzi, Eric W. Biederman, Ingo Molnar,
	Laurent Riffard, Pavel Emelyanov, Roland McGrath, linux-kernel

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/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 5/5] ptrace: it is fun to strace /sbin/init
  2008-03-20 23:25     ` Pavel Machek
@ 2008-03-21  1:15       ` Oleg Nesterov
  0 siblings, 0 replies; 7+ messages in thread
From: Oleg Nesterov @ 2008-03-21  1:15 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Andrew Morton, Davide Libenzi, Eric W. Biederman, Ingo Molnar,
	Laurent Riffard, Pavel Emelyanov, Roland McGrath, linux-kernel

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2008-03-21  1:17 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-16 15:54 [PATCH 5/5] ptrace: it is fun to strace /sbin/init Oleg Nesterov
2008-03-16 22:31 ` Roland McGrath
2008-03-16 23:17   ` Oleg Nesterov
2008-03-20 16:27 ` Pavel Machek
2008-03-20 16:57   ` Oleg Nesterov
2008-03-20 23:25     ` Pavel Machek
2008-03-21  1:15       ` Oleg Nesterov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).