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]

Date:	Sat, 31 May 2008 20:55:25 +0400
From:	Oleg Nesterov <oleg@...sign.ru>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
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, sorry for the delay,

On 05/20, Eric W. Biederman wrote:
> 
> 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.

Hmm. could you clarify? I tend to always trust you, just can't understand
the text above...

However, I think this patch adds another subtle race which I missed before.

Let's suppose that the task has two threads, A (== main thread) and B. A has
already exited, B does exec.

In that case it is possible that (without tasklist_lock) kill_something_info()
sends the signal to the old leader (A), but before group_send_sig_info(A)
takes ->siglock B switches the leader and does release_task(A). In that
group_send_sig_info()->lock_task_sighand() fails and we miss the process.

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

Note the (broken) "p->pid > 1" check, kill_something_info() skips init.
Not that it matters though.

Oleg.

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