[PATCH] Re: [BUG] Race condition with it_real_fn in kernel/itimer.c

Steven Rostedt
2005-06-15T19:34:46Z

On Wed, 2005-06-15 at 14:37 -0400, Steven Rostedt wrote:

>
> I haven't played too much with the itimer, what harm can happen if the
> timer is running while this is deleted? [examines code here] This also
> looks bad. Since the softirq function can be running and then call
> it_real_arm unprotected! And you can see here that it_real_arm is also
> called and they both call add_timer! This would not work, so far the
> first patch seems to handle this.

Ha! There's even another problem that I just noticed. it_real_value
could return zero while the function is running! So you don't get the
protection here either! Take a look here to see what the problem is.

static unsigned long it_real_value(struct signal_struct *sig)
{
unsigned long val = 0;
if (timer_pending(&sig->real_timer)) {
val = sig->real_timer.expires - jiffies;

/* look out for negative/zero itimer.. */
if ((long) val <= 0)
val = 1;
}
return val;
}

and

static inline int timer_pending(const struct timer_list * timer)
{
return timer->base != NULL;
}

and

static inline void __run_timers(tvec_base_t *base)
{
...
timer = list_entry(head->next,struct timer_list,entry);
fn = timer->function;
data = timer->data;

list_del(&timer->entry);
set_running_timer(base, timer);
smp_wmb();
timer->base = NULL;
spin_unlock_irq(&base->lock);
{
u32 preempt_count = preempt_count();
fn(data);
...
}

So, timer_pending tests if timer->base is NULL, but here we see that
timer->base IS NULL before the function is called, and as I have said
earlier, the it_real_arm can be called on two CPUS simultaneously. So
here's another patch that should fix this race condition too.

--- linux-2.6.12-rc6/kernel/itimer.c.orig 2005-06-15 12:14:13.000000000 -0400
+++ linux-2.6.12-rc6/kernel/itimer.c 2005-06-15 15:31:12.000000000 -0400
@@ -156,8 +156,15 @@
spin_lock_irq(&tsk->sighand->siglock);
interval = tsk->signal->it_real_incr;
val = it_real_value(tsk->signal);
- if (val)
- del_timer_sync(&tsk->signal->real_timer);
+ /*
+ * Call del_timer_sync unconditionally, since we don't
+ * know if it is running or not. We also need to unlock
+ * the siglock so that the it_real_fn called by ksoftirqd
+ * doesn't wait for us.
+ */
+ spin_unlock(&tsk->sighand->siglock);
+ del_timer_sync(&tsk->signal->real_timer);
+ spin_lock(&tsk->sighand->siglock);
tsk->signal->it_real_incr =
timeval_to_jiffies(&value->it_interval);
it_real_arm(tsk, timeval_to_jiffies(&value->it_value));

-- Steve