[PATCH 1/2] kill_something_info: misc cleanups

[PATCH 1/2] kill_something_info: misc cleanups

[Oleg Nesterov]

On top of
	signal-rewrite-kill_something_info-so-it-uses-newer-helpers.patch

- Factor out sending PIDTYPE_PGID wide signals.

- Use is_init(p) instead of "p->pid > 1". We don't hash idle threads anymore,
  no need to worry about p->pid == 0.

- Use "p != current->group_leader" instead of "p->tgid != current->tgid",
  saves one dereference and kills yet another direct pid_t usage.

- Simplify return value calculation for "pid == -1" case, remove "retval"
  variable.

No functional changes.

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

--- eric-mm1/kernel/signal.c~1_ksi	2006-12-16 20:57:58.000000000 +0300
+++ eric-mm1/kernel/signal.c	2006-12-16 22:08:43.000000000 +0300
@@ -1317,34 +1317,37 @@ EXPORT_SYMBOL_GPL(kill_pid_info_as_uid);
  * POSIX specifies that kill(-1,sig) is unspecified, but what we have
  * is probably wrong.  Should make it like BSD or SYSV.
  */
-
 static int kill_something_info(int sig, struct siginfo *info, int pid)
 {
 	int ret;
-	rcu_read_lock();
-	if (!pid) {
-		ret = kill_pgrp_info(sig, info, task_pgrp(current));
-	} else if (pid == -1) {
-		int retval = 0, count = 0;
-		struct task_struct * p;
-
-		read_lock(&tasklist_lock);
-		for_each_process(p) {
-			if (p->pid > 1 && p->tgid != current->tgid) {
-				int err = group_send_sig_info(sig, info, p);
-				++count;
-				if (err != -EPERM)
-					retval = err;
-			}
-		}
-		read_unlock(&tasklist_lock);
-		ret = count ? retval : -ESRCH;
-	} else if (pid < 0) {
-		ret = kill_pgrp_info(sig, info, find_pid(-pid));
-	} else {
-		ret = kill_pid_info(sig, info, find_pid(pid));
-	}
-	rcu_read_unlock();
+
+	rcu_read_lock();
+	if (pid > 0) {
+		ret = kill_pid_info(sig, info, find_pid(pid));
+	} else if (pid == -1) {
+		struct task_struct *p;
+		int found = 0;
+
+		ret = 0;
+		read_lock(&tasklist_lock);
+		for_each_process(p)
+			if (!is_init(p) && p != current->group_leader) {
+				int err = group_send_sig_info(sig, info, p);
+				if (err != -EPERM)
+					ret = err;
+				found = 1;
+			}
+		read_unlock(&tasklist_lock);
+		if (!found)
+			ret = -ESRCH;
+	} else {
+		struct pid *grp = task_pgrp(current);
+		if (pid != 0)
+			grp = find_pid(-pid);
+		ret = kill_pgrp_info(sig, info, grp);
+	}
+	rcu_read_unlock();
+
 	return ret;
 }
 

Re: [PATCH 1/2] kill_something_info: misc cleanups

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On top of
> 	signal-rewrite-kill_something_info-so-it-uses-newer-helpers.patch
>
> - Factor out sending PIDTYPE_PGID wide signals.
>
> - Use is_init(p) instead of "p->pid > 1". We don't hash idle threads anymore,
>   no need to worry about p->pid == 0.


I do not believe is_init is the proper function here.  In the presence
of multiple pid namespaces the intention is for is_init to catch all of
the special handling (except signal behavior) for the init process.

That way when we have multiple processes with pid == 1 we know which
one we care about.


> - Use "p != current->group_leader" instead of "p->tgid != current->tgid",
>   saves one dereference and kills yet another direct pid_t usage.

Makes sense as you have to be a group_leader to be on the task list.

> - Simplify return value calculation for "pid == -1" case, remove "retval"
>   variable.
>
> No functional changes.

Looks sane.

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

Eric

Re: [PATCH 1/2] kill_something_info: misc cleanups

[Oleg Nesterov]

On 12/16, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > On top of
> > 	signal-rewrite-kill_something_info-so-it-uses-newer-helpers.patch
> >
> > - Factor out sending PIDTYPE_PGID wide signals.
> >
> > - Use is_init(p) instead of "p->pid > 1". We don't hash idle threads anymore,
> >   no need to worry about p->pid == 0.
> 
> 
> I do not believe is_init is the proper function here.

Ok. How about child_reaper() for now? "p->pid == 1" doesn't look good either.

>                                                        In the presence
> of multiple pid namespaces

In that case we should use something else than for_each_process() to filter
out task from different namespaces, no?

>                             the intention is for is_init to catch all of
> the special handling (except signal behavior) for the init process.
>
> That way when we have multiple processes with pid == 1 we know which
> one we care about.

I must admit, I don't understand what is the purpose of pid namespace.
The current implementation looks incomplete. For example, mk_pid()
takes pid_namespace into account, but find_pid() (and thus attach_pid())
does not. Shouldn't pid_hash[] live in the "struct pid_namespace" ?

Oleg.

Re: [PATCH 1/2] kill_something_info: misc cleanups

[Eric W. Biederman]

I'm CC'ing the containers list these namespaces are what it is setup
to talk about.

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 12/16, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@tv-sign.ru> writes:
>> 
>> > On top of
>> > 	signal-rewrite-kill_something_info-so-it-uses-newer-helpers.patch
>> >
>> > - Factor out sending PIDTYPE_PGID wide signals.
>> >
>> > - Use is_init(p) instead of "p->pid > 1". We don't hash idle threads
> anymore,
>> >   no need to worry about p->pid == 0.
>> 
>> 
>> I do not believe is_init is the proper function here.
>
> Ok. How about child_reaper() for now? "p->pid == 1" doesn't look
> good either.

Ok.  It is probably reasonable to introduce a helper at this point so
we can be clear on what is happening.  I called it pspace_leader
in my proof of concept implementation.  pid_leader is probably more
concise and more in line with the rest of the naming though.

>>                                                        In the presence
>> of multiple pid namespaces
>
> In that case we should use something else than for_each_process() to filter
> out task from different namespaces, no?

Probably I haven't quite gotten that far yet.  In my proof of concept
implementation I simply did more filtering on the task list. i.e. I
did this:

> int __kill_pspace_info(int sig, struct siginfo *info, struct pspace *pspace)
> {
>         struct task_struct *p = NULL;
>         int retval = 0, count = 0;
> 
>         for_each_process(p) {
>                 int err;
>                 /* Skip the current pspace leader */
>                 if (current_pspace_leader(p))
>                         continue;
> 
>                 /* Skip the sender of the signal */
>                 if (p->signal == current->signal)
>                         continue;
> 
>                 /* Skip processes outside the target process space */
>                 if (!in_pspace(pspace, p))
>                         continue;
> 
>                 /* Finally it is a good process send the signal. */
>                 err = group_send_sig_info(sig, info, p);
>                 ++count;
>                 if (err != -EPERM)
>                         retval = err;
>         }
>         return count ? retval : -ESRCH;
> }
> 

The difficult part is the recursive nature of the pid namespace.  We
can't really keep a separate process list for each.  So if we wanted
an efficient traversal the only thing I can see is to put the pids
into a radix tree/trie and walk that.

I haven't gotten to this part yet in the production kernel.  I am
close but not quite there yet.  

>>                             the intention is for is_init to catch all of
>> the special handling (except signal behavior) for the init process.
>>
>> That way when we have multiple processes with pid == 1 we know which
>> one we care about.
>
> I must admit, I don't understand what is the purpose of pid namespace.
> The current implementation looks incomplete. For example, mk_pid()
> takes pid_namespace into account, but find_pid() (and thus attach_pid())
> does not. Shouldn't pid_hash[] live in the "struct pid_namespace" ?

It isn't yet.  Just a bit of scaffolding to make it possible.  However must
of the support code is in place.  As I see it the first step is getting
everything using struct pid, pid_nr, and getting rid of must uses of
daemonize() and kernel_thread.

Once I have that actually implementing the pid namespace in a usable form
will be a couple of simple patches.

What I am looking at for basic semantics is that each process in
addition to have a unique pid in it's own pid namespace will have a
unique pid in each pid namespace up to the root of the process tree.
I don't expect this to need to be more than about 3 pids for the first
implementation and then CLONE_NEW_PID_NAMESPACE and then return an out
of resources error. 

The pid namespace is fundamentally recursive because otherwise things
like wait and kill (basic process control) won't work if they don't
have pids to work with.  We need to allow for migration which means
pids can be assigned on another machine which means we have to work
with arbitrary pid values.

I have gotten the worst of them but there are still quite a few pid_t
references that need to be fixed up.  The two that stand out in my
head are pids in the siginfo of signal sending and credentials
on unix domain sockets.

When it is done from inside of the pid namespace you will have your
own separate set of pids.  Essentially what we are building is chroot
on steroids.  Separate instances of the pid namespace, the ipc
namespace, and the mount namespace pretty much will result in the
feeling of having the machine to yourself (when you are inside them).

Eric

Re: [PATCH 1/2] kill_something_info: misc cleanups

[Christoph Hellwig]

On Sat, Dec 16, 2006 at 11:05:10PM +0300, Oleg Nesterov wrote:
>  static int kill_something_info(int sig, struct siginfo *info, int pid)
>  {
>  	int ret;
> +
> +	rcu_read_lock();
> +	if (pid > 0) {
> +		ret = kill_pid_info(sig, info, find_pid(pid));
> +	} else if (pid == -1) {
> +		struct task_struct *p;
> +		int found = 0;
> +
> +		ret = 0;
> +		read_lock(&tasklist_lock);
> +		for_each_process(p)
> +			if (!is_init(p) && p != current->group_leader) {
> +				int err = group_send_sig_info(sig, info, p);
> +				if (err != -EPERM)
> +					ret = err;
> +				found = 1;
> +			}
> +		read_unlock(&tasklist_lock);
> +		if (!found)
> +			ret = -ESRCH;

This branch should probably be factored out into a helper of it's own:

static int kill_this_group_info(int sig, struct siginfo *info)
{
	struct task_struct *p;
	int ret = 0, found = 0;

	read_lock(&tasklist_lock);
	for_each_process(p) {
		if (!is_init(p) && p != current->group_leader) {
			int err = group_send_sig_info(sig, info, p);
			if (err != -EPERM)
				ret = err;
			found = 1;
		}
	}
	read_unlock(&tasklist_lock);
	if (!found)
		return -ESRCH;
	return found ? ret : -ESRCH;
}

> +	} else {
> +		struct pid *grp = task_pgrp(current);
> +		if (pid != 0)
> +			grp = find_pid(-pid);
> +		ret = kill_pgrp_info(sig, info, grp);
> +	}

This also looks rather unreadable, an

	} else if (pid) {
		ret = kill_pgrp_info(sig, info, find_pid(-pid));
	} else {
		ret = kill_pgrp_info(sig, info, task_pgrp(current));
	}

might be slightly more code, but also a lot more readable.

Re: [PATCH 1/2] kill_something_info: misc cleanups

[Eric W. Biederman]

Christoph Hellwig <hch@infradead.org> writes:

> On Sat, Dec 16, 2006 at 11:05:10PM +0300, Oleg Nesterov wrote:
>>  static int kill_something_info(int sig, struct siginfo *info, int pid)
>>  {
>>  	int ret;
>> +
>> +	rcu_read_lock();
>> +	if (pid > 0) {
>> +		ret = kill_pid_info(sig, info, find_pid(pid));
>> +	} else if (pid == -1) {
>> +		struct task_struct *p;
>> +		int found = 0;
>> +
>> +		ret = 0;
>> +		read_lock(&tasklist_lock);
>> +		for_each_process(p)
>> +			if (!is_init(p) && p != current->group_leader) {
>> +				int err = group_send_sig_info(sig, info, p);
>> +				if (err != -EPERM)
>> +					ret = err;
>> +				found = 1;
>> +			}
>> +		read_unlock(&tasklist_lock);
>> +		if (!found)
>> +			ret = -ESRCH;
>
> This branch should probably be factored out into a helper of it's own:

The proper name would be something like kill_all_info().  As we are
talking about the group of all processes.

I am sitting here wondering why we bother to ignore init, as init
is protected from all signals it doesn't explicitly setup a signal
handler for.  It is probably worth taking a quick look at the common
shutdown scripts and sysv init and see if anything actually cares if
we simply remove the is_init check.

The only two signals I know that are commonly handled this way
are kill(-1, SIGTERM) and kill(-1, SIGKILL);

And a very quick look at sysvinit-2.86 shows that it doesn't setup a
handler for SIGTERM.  So I believe we can delete we can delete
the is_init check entirely without changing anything and with a less
surprising if anyone ever cares.

>> +	} else {
>> +		struct pid *grp = task_pgrp(current);
>> +		if (pid != 0)
>> +			grp = find_pid(-pid);
>> +		ret = kill_pgrp_info(sig, info, grp);
>> +	}
>
> This also looks rather unreadable, an
>
> 	} else if (pid) {
> 		ret = kill_pgrp_info(sig, info, find_pid(-pid));
> 	} else {
> 		ret = kill_pgrp_info(sig, info, task_pgrp(current));
> 	}
>
> might be slightly more code, but also a lot more readable.

And that part is basically what we have now, just reshuffled.

Eric

Re: [PATCH 1/2] kill_something_info: misc cleanups

[Oleg Nesterov]

On 12/17, Eric W. Biederman wrote:
>
> I am sitting here wondering why we bother to ignore init, as init
> is protected from all signals it doesn't explicitly setup a signal
> handler for.
> ...
>                       So I believe we can delete we can delete
> the is_init check entirely without changing anything and with a less
> surprising if anyone ever cares.

is_init() is very cheap. But if we send a signal and it is not ignored
we will wake up /sbin/init without good reason, just to complete unneded
do_signal(). Also, we may have a special setup so that this signal really
means something for init (and it has a handler for). In that case the
caller of kill(-1, sig) will be surprised.

Btw, de_thread() already takes care about multithread init, but
get_signal_to_deliver() does not:

	if (current == child_reaper(current))
		continue;

	// handle sig_kernel_stop()/sig_fatal()

This doesn't protect init from SIGKILL if we send it to sub-thread (and
this can happen even if we use kill(1, sig), not tkill). Yes, the main
thread will survive, but still this is not what we want. SIGSTOP will
manage to stop entire group because sub-thread sets ->group_stop_count.

> > Christoph Hellwig <hch@infradead.org> writes:
> >
> > This also looks rather unreadable, an
> >
> >       } else if (pid) {
> >               ret = kill_pgrp_info(sig, info, find_pid(-pid));
> >       } else {
> >               ret = kill_pgrp_info(sig, info, task_pgrp(current));
> >       }
> >
> > might be slightly more code, but also a lot more readable.

I personally disagree, but this is matter of taste.

Ok, it was a cleanup only, let's forget it.

Still I don't like "p->pid > 1" check. And I don't think we need a new
helper (pid_leader or such) now. When we have multiple pid namespaces
we should rework kill(-1, sig) anyway. Right now this check means
"skip init", nothing more.

Oleg.

Re: [PATCH 1/2] kill_something_info: misc cleanups

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 12/17, Eric W. Biederman wrote:
>>
>> I am sitting here wondering why we bother to ignore init, as init
>> is protected from all signals it doesn't explicitly setup a signal
>> handler for.
>> ...
>>                       So I believe we can delete we can delete
>> the is_init check entirely without changing anything and with a less
>> surprising if anyone ever cares.
>
> is_init() is very cheap. But if we send a signal and it is not ignored
> we will wake up /sbin/init without good reason, just to complete unneded
> do_signal(). 

kill(-1, SIGXXX) is sufficiently expensive and rare that waking up an
additional process doesn't matter.

> Also, we may have a special setup so that this signal really
> means something for init (and it has a handler for). In that case the
> caller of kill(-1, sig) will be surprised.

If it exists sure.  My quick glance suggested that nothing in user
space cares.  My thinking about it suggests that have a different
rule for ignoring signals for kill(-1,..) and every other kill
function is confusing.  So we may be doing the wrong thing.

> Btw, de_thread() already takes care about multithread init, but
> get_signal_to_deliver() does not:
>
> 	if (current == child_reaper(current))
> 		continue;

Yep.  That looks like something that needs to be fixed.  Looks like
I missed it when I realized multi-threaded init was a problem.
Probably just: current->group_leader == child_reaper(current).

Hmm.  If the code already has a child_reaper(current) tests in the
signal delivery that is probably the idiom we should use to start
with to detect init for signal handling purposes.

Hmm.  I don't like that test there at all.  I think this is quite
likely something I got wrong the first time through.

The desired action is: kill(1, SIGKILL) is ignored, kill(75, SIGKILL)
is delivered, even if both of those refer to the same process
the difference being in perspective which pid namespace we are
coming from.

Can we do the ignore in send_signal?

I definitely need to look at signal deliver more.

> 	// handle sig_kernel_stop()/sig_fatal()
>
> This doesn't protect init from SIGKILL if we send it to sub-thread (and
> this can happen even if we use kill(1, sig), not tkill). Yes, the main
> thread will survive, but still this is not what we want. SIGSTOP will
> manage to stop entire group because sub-thread sets ->group_stop_count.

Yep.  We need to fix this.  It doesn't happen today because we don't
have a multi-threaded init.  But as soon as untrusted users can have
their own init this becomes we need to handle everything properly.

>> > Christoph Hellwig <hch@infradead.org> writes:
>> >
>> > This also looks rather unreadable, an
>> >
>> >       } else if (pid) {
>> >               ret = kill_pgrp_info(sig, info, find_pid(-pid));
>> >       } else {
>> >               ret = kill_pgrp_info(sig, info, task_pgrp(current));
>> >       }
>> >
>> > might be slightly more code, but also a lot more readable.
>
> I personally disagree, but this is matter of taste.
>
> Ok, it was a cleanup only, let's forget it.
>
> Still I don't like "p->pid > 1" check. And I don't think we need a new
> helper (pid_leader or such) now. When we have multiple pid namespaces
> we should rework kill(-1, sig) anyway. Right now this check means
> "skip init", nothing more.

We need two specific helpers.
1) To detect the real machine init and the special things we have to
   do for it.
2) To detect the init of a pid namespace for user visible semantic
   purposes like signal handling (is there anything else).

I really don't care much about them so long as they are separate
functions.  The reason I care is that we are in the middle of
implementing the pid namespace.

Eric

Re: [PATCH 1/2] kill_something_info: misc cleanups

[Oleg Nesterov]

On 12/18, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > Btw, de_thread() already takes care about multithread init, but
> > get_signal_to_deliver() does not:
> >
> > 	if (current == child_reaper(current))
> > 		continue;
> 
> Probably just: current->group_leader == child_reaper(current).

No. deadlock on exec.

> Can we do the ignore in send_signal?

No. It is not possible immediately with the current implemantation,
but the main reason is that we want to retain the "init is protected
from all signals it doesn't explicitly setup a signal handler for"
property. Consider init doing

	signal(SIGTERM, handler);
	...
	signal(SIGTERM, SIG_DFL);	<----- SIGTERM comes

so we should do something for init on receive-signal path. Yes, yes,
we _can_ solve this in other way (say, change sys_rt_sigaction), but
this is nasty and doesn't solve other problems.

> > This doesn't protect init from SIGKILL if we send it to sub-thread (and
> > this can happen even if we use kill(1, sig), not tkill). Yes, the main
> > thread will survive, but still this is not what we want. SIGSTOP will
> > manage to stop entire group because sub-thread sets ->group_stop_count.
> 
> Yep.  We need to fix this.  It doesn't happen today because we don't
> have a multi-threaded init.  But as soon as untrusted users can have
> their own init this becomes we need to handle everything properly.

This is a longstanding problem, an it is connected to other longstanding
problems (say, fatal signal may be lost on exec). I wish I had a time to
at least try to find a solution.

Probably I'll find some time at the beginning of January, I have some
vague ideas. Hmm... at least I had :)

> We need two specific helpers.
> 1) To detect the real machine init and the special things we have to
>    do for it.
> 2) To detect the init of a pid namespace for user visible semantic
>    purposes like signal handling (is there anything else).

Yes, I understand. My intent was to push this cleanup before multiple
pid namespaces will change kill_something_inf(). But as I said it is
minor, and since you and Christoph don't like it (for whatever reason)
please forget it.

Oleg.