lkml.org 
[lkml]   [2007]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] posix-timer: fix deletion race
On 07/17, 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->......)

This is funny. I do remember this bug was discussed a couple of years ago,
and the conclusion was "we can fix it later" :)

> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -605,13 +605,14 @@ static struct k_itimer * lock_timer(timer_t timer_id, unsigned long *flags)
> timr = (struct k_itimer *) idr_find(&posix_timers_id, (int) timer_id);
> if (timr) {
> spin_lock(&timr->it_lock);
> - spin_unlock(&idr_lock);
>
> if ((timr->it_id != timer_id) || !(timr->it_process) ||
> timr->it_process->tgid != current->tgid) {
> - unlock_timer(timr, *flags);
> + spin_unlock(&timr->it_lock);
> + spin_unlock_irqrestore(&idr_lock, *flags);
> timr = NULL;
> - }
> + } else
> + spin_unlock(&idr_lock);
> } else
> spin_unlock_irqrestore(&idr_lock, *flags);

I think we can make a simpler patch,

--- posix-timers.c~ 2007-06-29 14:45:04.000000000 +0400
+++ posix-timers.c 2007-07-17 16:59:45.000000000 +0400
@@ -449,6 +449,9 @@ static void release_posix_timer(struct k
idr_remove(&posix_timers_id, tmr->it_id);
spin_unlock_irqrestore(&idr_lock, flags);
}
+
+ spin_unlock_wait(tmr->it_lock);
+
sigqueue_free(tmr->sigq);
if (unlikely(tmr->it_process) &&
tmr->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))

What do you think? Instead of complicating the lock_timer(), release_posix_timer()
just makes sure that nobody can "use" tmr.

(Actually, I am not sure this is my idea, perhaps something like above was suggested
by somebody else, I forgot the discussion completely).

Oleg.

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

\
 
 \ /
  Last update: 2007-07-17 15:09    [W:0.087 / U:0.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site