> - if (del_timer(&ifp->timer))
> + * REVIEWME: I assume the del_timer_async() here is safe
> + * due to the below check (timer shouldn't be scheduled).
> + */
> + if (del_timer_async(&ifp->timer))
> printk("Timer is still running, when freeing ifa=%p\n", ifp);
This is debugging check. We have already discussed this.
> +/* Timer handler */
> +static void fib6_run_gc_t(unsigned long dummy)
> + fib6_run_gc(dummy, 1);
> void __init fib6_init(void)
> @@ -1211,7 +1221,7 @@
> #ifdef MODULE
> void fib6_gc_cleanup(void)
> - del_timer(&ip6_fib_timer);
> + del_timer_sync(&ip6_fib_timer);
Modular IPv6 is so buggy, that it is useless to fix these issues
separately of big picture, though.
> + * REVIEWME: we may have a race here.
> + * ip6_fl_gc() could be running now, and after fl_release() returns,
> + * this ip6_flowlabel is kfree()ed. Is there a possibility that
> + * the still-running ip6_fl_gc() could end up accessing kfree'ed
> + * memory? If so, I think a del_timer_sync() is safe here
Yes, the same as in fib6 gc.
> + * REVIEWME: igmp6_timer_handler() could be running right now on another CPU.
> + * Is this safe? If not, we need del_timer_sync() in igmp6_group_queried()
> + */
del_timer_sync() does not work in this case, certainly.
It is not evident, because spinlocks are forgotten too. 8)
Note, that igmp6 case is more complicated than igmp, it makes
more work from softirqs. I hoped to move addrconf to process context
until 2.4.0, and, certainly, forgot. 8)
OK, we have to make it working on softirq yet.
> +/* REVIEWME: igmp6_timer_handler() could be running now. Is this safe?
> Could we
> + * do the in6_dev_put() right in the middle of igmp6_send()? If so, we need
> + * Andi says "Should not matter. it is a independent idev."
> + */
We grab refcnt, when we use idev.
> +/* REVIEWME: frag_expire() could be running on another CPU now, (if this
> + * function is called from reasm_frag()). frag_expire() will be spinning
> + * on ip6_frag_lock. Once this function returns to reasm_frag() and
> + * reasm_frag releases the lock, frag_expire() will run and will
> + * again call fq_free. Probably safe, but needs an expert eye :)
> + */
> + del_timer_async(&fq->timer);
We have already discussed this. It is not safe, certainly,
and needs refcounting.