[PATCH 1/3] posix-timers: don't switch to ->group_leader if ->it_process dies

[PATCH 1/3] posix-timers: don't switch to ->group_leader if ->it_process dies

[Oleg Nesterov]

(textually depends on posix-timers-fix-posix_timer_event-vs-dequeue_signal-race.patch)

posix_timer_event() drops SIGEV_THREAD_ID and switches to ->group_leader if
send_sigqueue() fails.

This is not very useful and doesn't work reliably. send_sigqueue() can only
fail if ->it_process is dead. But it can die before it dequeues the SI_TIMER
signal, in that case the timer stops anyway.

Remove this code. I guess it was needed a long ago to ensure that the timer
is not destroyed when when its creator thread dies.

Q: perhaps it makes sense to change sys_timer_settime() to return an error
if ->it_process is dead?

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 26-rc2/kernel/posix-timers.c~1_DONT_RESEND	2008-07-23 20:24:05.000000000 +0400
+++ 26-rc2/kernel/posix-timers.c	2008-08-03 20:25:41.000000000 +0400
@@ -298,6 +298,7 @@ void do_schedule_next_timer(struct sigin
 
 int posix_timer_event(struct k_itimer *timr, int si_private)
 {
+	int shared, ret;
 	/*
 	 * FIXME: if ->sigq is queued we can race with
 	 * dequeue_signal()->do_schedule_next_timer().
@@ -316,20 +317,10 @@ int posix_timer_event(struct k_itimer *t
 	timr->sigq->info.si_tid = timr->it_id;
 	timr->sigq->info.si_value = timr->it_sigev_value;
 
-	if (timr->it_sigev_notify & SIGEV_THREAD_ID) {
-		struct task_struct *leader;
-		int ret = send_sigqueue(timr->sigq, timr->it_process, 0);
-
-		if (likely(ret >= 0))
-			return ret;
-
-		timr->it_sigev_notify = SIGEV_SIGNAL;
-		leader = timr->it_process->group_leader;
-		put_task_struct(timr->it_process);
-		timr->it_process = leader;
-	}
-
-	return send_sigqueue(timr->sigq, timr->it_process, 1);
+	shared = !(timr->it_sigev_notify & SIGEV_THREAD_ID);
+	ret = send_sigqueue(timr->sigq, timr->it_process, shared);
+	/* If we failed to send the signal the timer stops. */
+	return ret > 0;
 }
 EXPORT_SYMBOL_GPL(posix_timer_event);
 

Re: [PATCH 1/3] posix-timers: don't switch to ->group_leader if ->it_process dies

[Roland McGrath]

The key thing that the old code does it to drop the ref to the old task as
soon as it notices.  It may be worthwhile to do that in the case where
send_sigqueue fails.  I realize it's already not perfect, since it will
keep the old ref around for a long time if the timer does not fire soon.


Thanks,
Roland

Re: [PATCH 1/3] posix-timers: don't switch to ->group_leader if ->it_process dies

[Oleg Nesterov]

On 08/03, Roland McGrath wrote:
>
> The key thing that the old code does it to drop the ref to the old task as
> soon as it notices.

I thought about this too. But, again, this doesn't actually work. And it is
very easy to pin the dead task_struct. The user can just create the
SIGEV_THREAD_ID timer and kill the sub-thread (->it_process). Without
calling timer_settime() the timer can't notice the death.

(just in case, please note that the next patch doesn't make things worse,
 when ->group_leader dies the timer should be destroyed soon).

> It may be worthwhile to do that in the case where
> send_sigqueue fails.

I think it is better to convert the posix timers to use "struct pid*" instead
of "struct task_struct*". This change looks more or less straightforward, and
it should fix the problem.

Oleg.