linux-kernel - Re: [PATCH] signals: do_tkill: don't use 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:	Fri, 7 Mar 2008 23:13:46 +0300
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Roland McGrath <roland@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] signals: do_tkill: don't use tasklist_lock

On 03/07, Roland McGrath wrote:
> 
> > Note that we don't return an error if lock_task_sighand() fails, we
> > pretend the task dies after receiving the signal. Otherwise, we should
> > fight with the nasty races with mt-exec without having any advantage.
> 
> To clarify, this is not a change from the existing behavior.
> So your change is fine regardless of this issue.
> 
> The case you have in mind is that p was the old group_leader
> being replaced by another thread that exec'd, right?

Yes.

> It is the most obscure of nits, but I think it can be wrong to drop a
> signal in this case.  If it's a fatal signal (especially SIGKILL),
> then either the thread group should be killed or the call should
> return an error.

Yes, we are not consistent here with or without this patch.

Btw, a question: we are buggy or just "not perfect" ? After all, the
main thread actually exits although this is just linux's implementation
detail.

> For the exec case, if p->sighand is cleared that means the
> release_task(leader) call at the end of de_thread started.  So by
> now, the pid has been transferred to the exec'ing thread.  If we just
> restart the lookup, it will find the new thread (or not, and we can
> return -ESRCH).

Yep. kill_pid_info() does exactly this. Initially I was going to repeat
this logic,

> I'm inclined to do that, but it certainly should be a second patch
> after this one.

but actually it doesn't buy much here.

Suppose that the main thread is already dead (dequeued SIGKILL), but
not yet released. This window is not that small. In that window (before
de_thread() switches pids) any private signal (even SIGKILL) sent to the
main thread will be silently lost.

To do a proper fix, we should change de_thread() so that the new leader
also "inherits" old_leader->pending signals.

This is certainly possible as a separate change.

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/