Re: arca-24 [Re: new arca-23 released]

Finn Arne Gangstad (finnag@guardian.no)
Fri, 20 Nov 1998 14:23:13 +0100 (MET)


On Fri, 20 Nov 1998, Andrea Arcangeli wrote:

> o Check that the timer is just pending before add it. If it' s just
> pending do nothing.
> I think this is the cleaner/more efficient way to fix the
> getitimer race and I think that make the timer code more robust
> too. So doing a add_timer(timer);add_timer(timer); will not cause
> one problem.
>[...]

(a slightly compressed version of the relevant patch section):
> void add_timer(struct timer_list *timer)
> {
> unsigned long flags;
>
> spin_lock_irqsave(&timerlist_lock, flags);
> - internal_add_timer(timer);
> + if (!timer_pending(timer))
> + internal_add_timer(timer);
> spin_unlock_irqrestore(&timerlist_lock, flags);
> }

That is _VERY_ dangerous. The code that adds timers usually looks like
this:

timer->expires = whatever;
add_timer(timer);

If the timer was already pending, you have just DESTROYED its expires
value. This means you have no control over when it will expire at all,
since the timer needs to be in the correct timer vector to expire when it
should. It can also mess up cascade_timers slightly (this can and should
be fixed though).

The solution is to use mod_timer instead, it does everything necessary, so
if there is a race anywhere, replace

"timer->expires = x; add_timer(x);" with "mod_timer(timer, x);"

Another change, this one in del_timer:

> spin_lock_irqsave(&timerlist_lock, flags);
> ret = detach_timer(timer);
> - timer->next = timer->prev = 0;
> spin_unlock_irqrestore(&timerlist_lock, flags);
> + timer->next = timer->prev = 0;
> return ret;
> }

What's the rationale for that change? Seems buggy to me, you risk having a
timer with a non-zero prev that is NOT attached, which can confuse the
hell out of the timer functions.

- Finn Arne

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/