Re: [PATCH] posix-timer: fix deletion race

[Oleg Nesterov <oleg@tv-sign.ru>]

On 07/18, Jeremy Katz wrote:
>
> On Wed, 18 Jul 2007, Oleg Nesterov wrote:
> 
> >Jeremy, I agree with Thomas that your patch should not be right, but it
> >does make a difference. Perhaps this is just the timing, but who knows.
> >Could you add some printk's to be sure that lock_timer() actually fails
> >while it never should?
> 
> Agreed.
> 
> Unfortunately, adding any significant output appears to alter the 
> situation to the point where the issue either does not occur, or takes 
> significantly longer to surface.

No, no, I didn't mean any significant output. You changed itimer_delete()

	>  -       spin_lock_irqsave(&timer->it_lock, flags);
	>  +       /* timer already deleted? */
	>  +       if (lock_timer(timer->it_id, &flags) == NULL)
	>  +               return;

This change should not help, lock_timer() should always succeed here.
But since it makes a difference, we can make something like

	if (lock_timer(timer->it_id, &flags) == NULL) {
		printk("Impossible! but it happened.\n");
		return;
	}

The same for posix_timer_fn().


I still can't believe we have a double-free problem, this looks imposiible.
Do you see the

	"idr_remove called for id=%d which is not allocated.\n"

in syslog?


Could you try the patch below? Perhaps we have some wierd problem with
->sigq corruption.

Oleg.

--- t/kernel/posix-timers.c~	2007-06-29 14:45:04.000000000 +0400
+++ t/kernel/posix-timers.c	2007-07-20 18:07:59.000000000 +0400
@@ -443,13 +443,16 @@ static struct k_itimer * alloc_posix_tim
 #define IT_ID_NOT_SET	0
 static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 {
+	sigqueue_free(tmr->sigq);
+	tmr->sigq = NULL;
+
 	if (it_id_set) {
 		unsigned long flags;
 		spin_lock_irqsave(&idr_lock, flags);
 		idr_remove(&posix_timers_id, tmr->it_id);
 		spin_unlock_irqrestore(&idr_lock, flags);
 	}
-	sigqueue_free(tmr->sigq);
+
 	if (unlikely(tmr->it_process) &&
 	    tmr->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))
 		put_task_struct(tmr->it_process);
@@ -615,6 +618,7 @@ static struct k_itimer * lock_timer(time
 	} else
 		spin_unlock_irqrestore(&idr_lock, *flags);
 
+	BUG_ON(timr && !timr->sigq);
 	return timr;
 }