Messages in this thread | | | Date | Wed, 21 May 2008 10:48:12 +0900 | From | Atsushi Tsuji <> | Subject | Re: [PATCH] kill_something_info: don't take tasklist_lock for pid==-1 case |
| |
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
| |