Re: [PATCH] kill_something_info: don't take tasklist_lock for pid==-1 case

[Atsushi Tsuji <a-tsuji@bk.jp.nec.com>]

Oleg Nesterov wrote:
> On 03/25, Atsushi Tsuji wrote:
>> This patch avoid taking tasklist_lock in kill_something_info() when
>> the pid is -1. It can convert to rcu_read_lock() for this case because
>> group_send_sig_info() doesn't need tasklist_lock.
>>
>> This patch is for 2.6.25-rc5-mm1.
>>
>> Signed-off-by: Atsushi Tsuji <a-tsuji@bk.jp.nec.com>
>
> Except: We don't send the signal to /sbin/init. This means that (say)
> kill(-1, SIGKILL) can miss the task forked by init. Note that this
> task could be forked even before we start kill_something_info(), but
> without tasklist there is no guarantee we will see it on the ->tasks
> list.
> 
> I think this is the only problem with this change.

Sorry for late reply and thank you for your comment. I understood the
mechanism that kill(-1, SIGKILL) can miss the tasks forked by init
(and the thread group of the current process, because we don't also
send the signal to them). If kill(-1, SIGKILL) finish before the
forking init process does list_add_tail_rcu(p->tasks) in
copy_process(), the process forked by init appears on the ->tasks list
after that.  Is that right?

If so, I think this problem can happen without my patch.
Because even if kill(-1, SIGKILL) take read_lock(&tasklist_lock) in
kill_something_info(), it can finish before init process take
write_lock(&tasklist_lock) in copy_process(). So the forked process
appears after that, too.

Now, I noticed the important problem. I found the tasklist lock in
kill_something_info() can cause stall when some processes execute
kill(-1,SIGCONT) concurrently. It can happen even if a system has
only 4 CPUs (and even if a user is not privileged (not root)).  This is
because the writer cannot take the tasklist lock when a lot of readers
exist and keep holding it.

This allows a local DoS. So we have to avoid that stall. The
conversion from the tasklist lock to rcu_read_lock() can solve this
problem. I think my patch doesn't make the new problem because the
problem that kill can miss the tasks have originally occurred without
my one. If there is no problem, could you ack it?

Thanks,
-Atsushi Tsuji