Re: Instability in kernel version 2.6.12.5

["David S. Miller" <davem@davemloft.net>]

From: Linus Torvalds <torvalds@osdl.org>
Date: Thu, 6 Oct 2005 11:24:25 -0700 (PDT)

> For example, maybe some networking timer just changes its "expires" field 
> _while_ a timer is active directly rather than using "mod_timer()", which 
> can screw up the sorting - and that can affect other timers.

We actually investigated a possible case of this recently.

It was thought that perhaps it was possible for the ARP
generic neighbour cache to double-add a timer, so we added
a guard by converting it to mod_timer() from add_timer()
and making sure it always returns "0" (timer not active).

static inline void neigh_add_timer(struct neighbour *n, unsigned long when)
{
	if (unlikely(mod_timer(&n->timer, when))) {
		printk("NEIGH: BUG, double timer add, state is %x\n",
		       n->nud_state);
	}
}

But this new debugging hasn't triggered for anyone yet :-)

In general the networking tends to use mod_timer() exclusively, for
the simple reason that this makes refcounting on the object so much
simpler.  For example, all of the socket timer helpers do stuff like
this:

void sk_reset_timer(struct sock *sk, struct timer_list* timer,
		    unsigned long expires)
{
	if (!mod_timer(timer, expires))
		sock_hold(sk);
}

void sk_stop_timer(struct sock *sk, struct timer_list* timer)
{
	if (timer_pending(timer) && del_timer(timer))
		__sock_put(sk);
}

I have no idea what the situation is in the netfilter bits, but
something similar is likely.

This brings me to a topic I'd like addressed.  add_timer() no longer
checks whether it is adding a timer twice or not.

This debugging functionality got lost when add_timer() was changed
to be implemented in terms of __mod_timer().  I really think adding
back a "BUG_ON(timer_pending(timer)" would be a very good idea.
I believe Andrew Morton even added this bug check into his -mm tree
last time I brought this issue up.

Finally, it could be argued that add_timer() is not really a necessary
interface and that one can do whatever they need to purely using
mod_timer().  mod_timer() is kind of like a NAND gate I suppose :-)