linux-kernel - Re: [PATCH] kill_something_info: don't take tasklist

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m18wy45nej.fsf@frodo.ebiederm.org>
Date:	Tue, 20 May 2008 20:47:16 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Oleg Nesterov <oleg@...sign.ru>
Cc:	Atsushi Tsuji <a-tsuji@...jp.nec.com>,
	linux-kernel@...r.kernel.org, Roland McGrath <roland@...hat.com>
Subject: Re: [PATCH] kill_something_info: don't take tasklist_lock for pid==-1 case

Sorry for the very delayed response.  I have been moving and overwhelmed
with everything.

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

> 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.
>> 

> Hmm. Yes, group_send_sig_info() doesn't need tasklist_lock. But we
> take tasklist_lock to "freeze" the tasks list, so that we can't miss
> a new forked process.
>
> Same for __kill_pgrp_info(), we take tasklist to kill the whole group
> "atomically".
>
>
> However. Is it really needed? copy_process() returns -ERESTARTNOINTR
> if signal_pending(), and the new task is always placed at the tail
> of the list. Looks like nobody can escape the signal, at least fatal
> or SIGSTOP.

Call me paranoid but I don't think there is any guarantee without a lock
that we will hit the -ERESTARTNOITR check for new processes.  I think we
have a slight race where the fork process may not have received the signal
(because it is near the tail of the list) but the new process would be
added to the list immediately after we read it's pointer.

> Note also that copy_process() does list_add_tail_rcu(p->tasks) under
> ->siglock, this means kill_something_info() must see the new childs
> after group_send_sig_info() drops ->siglock.

That is subtle.  Switching to the per task siglock for protection.

> 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.

Actually we do sent the signal to init but we shouldn't, if we want
our semantics straight.  And we drop the signal early enough we might
not see it.

> I think this is the only problem with this change.
>
> Eric, Roland?

That is a very subtle idea.  I wish I could convince myself it would
work and be maintainable.

> (Unfortunately, attach_pid() adds the task to the head of hlist, this
>  means we can't avoid tasklist for __kill_pgrp_info).

Probably.  If there wasn't a chance of sending a signal twice we
could rescan the list if the head changed.

What we might be able to do is to switch the tasklist_lock into a seq_lock.
like was done for the dcache.  The challenge is how to avoid resending
a signal when we retry.  Store the sequence number in the sighand_struct?

If we had a magic place that children could check. To see if they belonged
to a group of processes that was exiting that might help.

Grr.  I just can't see any solution that is cheaper and that I can
verify will be correct.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/