Messages in this thread Patch in this message | | | Date | Fri, 20 Jul 2007 18:15:29 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH] posix-timer: fix deletion race |
| |
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; } - 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/
| |