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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
