Messages in this thread Patch in this message | | | Date | Tue, 17 Jul 2007 11:39:43 -0700 (PDT) | From | Jeremy Katz <> | Subject | Re: [PATCH] posix-timer: fix deletion race |
| |
On Tue, 17 Jul 2007, Thomas Gleixner wrote:
> Jeremy Katz experienced a posix-timer related bug on 2.6.14. This is > caused by a subtle race, which is there since the original posix timer > commit and persists until today. > > timer_delete does: > lock_timer(); > timer->it_process = NULL; > unlock_timer(); > release_posix_timer(); > > timer->it_process is checked in lock_timer() to prevent access to a > timer, which is on the way to be deleted, but the check happens after > idr_lock is dropped. This allows release_posix_timer() to delete the > timer before the lock code can check the timer: > > CPU 0 CPU 1 > lock_timer(); > timer->it_process = NULL; > unlock_timer(); > lock_timer() > spin_lock(idr_lock); > timer = idr_find(); > spin_lock(timer->lock); > spin_unlock(idr_lock); > release_posix_timer(); > spin_lock(idr_lock); > idr_remove(timer); > spin_unlock(idr_lock); > free_timer(timer); > if (timer->......) > > Change the locking to prevent this.
I tried the patch with my test case, but still see the issue. Here's my explanation of the double free race: CPU 0 CPU 1 sys_timer_delete(): lock_timer(); ... unlock_timer(); itimer_delete() release_posix_timer(): spin_lock_irqsave(timer...) ... ... sigqueue_free() unlock_timer() ... sigqueue_free() BUG_ON(!(q->flags...
the timer fires during deletion: CPU 0 CPU 1 sys_timer_delete(): lock_timer(); ... unlock_timer(); posix_timer_fn() release_posix_timer(): spin_lock_irqsave(timer...) ... ... sigqueue_free() posix_timer_event() ... ... send_sigqueue() BUG_ON(!(q->flags
Currently, this appears to fix the problem. The system survived 19 hours before I rebooted to try other things, while the I usually see at least one of the BUGs within 15 minutes.
--- linux-2.6.14-cgl.original/kernel/posix-timers.c 2007-07-11 16:06:47.000000000 -0700 +++ linux-2.6.14-cgl/kernel/posix-timers.c 2007-07-16 15:25:43.000000000 -0700 @@ -339,7 +339,9 @@ static int posix_timer_fn(void *data) int si_private = 0; int ret = HRTIMER_NORESTART;
- spin_lock_irqsave(&timr->it_lock, flags); + if(lock_timer(timr->it_id, &flags) == NULL) { + return ret; + }
if (timr->it.real.interval.tv64 != 0) si_private = ++timr->it_requeue_pending; @@ -411,8 +413,10 @@ static void release_posix_timer(struct k { if (it_id_set) { unsigned long flags; + /* unmangle timer id */ + int it_id = tmr->it_id & INT_MAX; spin_lock_irqsave(&idr_lock, flags); - idr_remove(&posix_timers_id, tmr->it_id); + idr_remove(&posix_timers_id, it_id); spin_unlock_irqrestore(&idr_lock, flags); } sigqueue_free(tmr->sigq); @@ -545,8 +549,11 @@ sys_timer_create(const clockid_t which_c */
out: - if (error) + if (error) { + /* mangle the timer id for the release */ + new_timer->it_id |= ~INT_MAX; release_posix_timer(new_timer, it_id_set); + }
return error; } @@ -822,6 +829,8 @@ retry_delete: put_task_struct(timer->it_process); timer->it_process = NULL; } + /* mangle the timer ID to prevent anyone else from finding it */ + timer->it_id |= ~INT_MAX; unlock_timer(timer, flags); release_posix_timer(timer, IT_ID_SET); return 0; @@ -835,7 +844,9 @@ static inline void itimer_delete(struct unsigned long flags;
retry_delete: - spin_lock_irqsave(&timer->it_lock, flags); + /* timer already deleted? */ + if (lock_timer(timer->it_id, &flags) == NULL) + return;
if (timer_delete_hook(timer) == TIMER_RETRY) { unlock_timer(timer, flags); @@ -851,6 +862,8 @@ retry_delete: put_task_struct(timer->it_process); timer->it_process = NULL; } + /* mangle the timer ID to prevent anyone else from finding it */ + timer->it_id |= ~INT_MAX; unlock_timer(timer, flags); release_posix_timer(timer, IT_ID_SET); } - 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/
| |