MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
From: Roland McGrath <roland@redhat.com>
To: Oleg Nesterov <oleg@tv-sign.ru>
Cc: Andrew Morton <akpm@linux-foundation.org>,
       "Eric W. Biederman" <ebiederm@xmission.com>,
       Ingo Molnar <mingo@elte.hu>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] signals: do_tkill: don't use tasklist_lock
In-Reply-To: Oleg Nesterov's message of  Friday, 7 March 2008 12:58:13 +0300 <20080307095813.GA8894@tv-sign.ru>
References: <20080307095813.GA8894@tv-sign.ru>
Message-Id: <20080307193159.1AFF127010F@magilla.localdomain>
Date: Fri,  7 Mar 2008 11:31:59 -0800 (PST)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1412
Lines: 37

> Convert do_tkill() to use rcu_read_lock() + lock_task_sighand() to avoid
> taking tasklist lock.

That part looks good.

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

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.  

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

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


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