Hi, Alexey.
Attached is my shot at tightening the timer handling in net/ipv6. Andi
has provided some comments (Thanks!).
The fib6_run_gc() alterations are not pleasant.
There is some code which I believe is safe, but I wasn't 100% sure, so I
left some commentary in there.
Hope this helps... --- linux-2.4.0-test1-ac8/net/ipv6/addrconf.c Wed May 3 18:48:03 2000
+++ linux-akpm/net/ipv6/addrconf.c Mon Jun 5 23:11:11 2000
@@ -180,7 +180,7 @@
static void addrconf_del_timer(struct inet6_ifaddr *ifp)
{
- if (del_timer(&ifp->timer))
+ if (del_timer_async(&ifp->timer))
__in6_ifa_put(ifp);
}
@@ -195,7 +195,7 @@
enum addrconf_timer_t what,
unsigned long when)
{
- if (!del_timer(&ifp->timer))
+ if (!del_timer_async(&ifp->timer))
in6_ifa_hold(ifp);
switch (what) {
@@ -316,7 +316,11 @@
in6_dev_put(ifp->idev);
- 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);
if (!ifp->dead) {
@@ -1660,6 +1664,7 @@
}
mod_timer(&addr_chk_timer, jiffies + ADDR_CHECK_FREQUENCY);
+ timer_exit(&addr_chk_timer);
}
#ifdef CONFIG_RTNETLINK
@@ -2081,7 +2086,7 @@
}
write_unlock_bh(&addrconf_hash_lock);
- del_timer(&addr_chk_timer);
+ del_timer_sync(&addr_chk_timer);
rtnl_unlock();
--- linux-2.4.0-test1-ac8/net/ipv6/ip6_fib.c Wed May 3 18:48:04 2000
+++ linux-akpm/net/ipv6/ip6_fib.c Tue Jun 6 21:51:06 2000
@@ -87,7 +87,8 @@
static __u32 rt_sernum = 0;
-static struct timer_list ip6_fib_timer = { function: fib6_run_gc };
+static void fib6_run_gc_t(unsigned long dummy);
+static struct timer_list ip6_fib_timer = { function: fib6_run_gc_t };
static struct fib6_walker_t fib6_walker_list = {
&fib6_walker_list, &fib6_walker_list,
@@ -1169,7 +1170,7 @@
static spinlock_t fib6_gc_lock = SPIN_LOCK_UNLOCKED;
-void fib6_run_gc(unsigned long dummy)
+void fib6_run_gc(unsigned long dummy, int from_timer)
{
if (dummy != ~0UL) {
spin_lock_bh(&fib6_gc_lock);
@@ -1193,10 +1194,19 @@
if (gc_args.more)
mod_timer(&ip6_fib_timer, jiffies + ip6_rt_gc_interval);
else {
- del_timer(&ip6_fib_timer);
+ del_timer_async(&ip6_fib_timer); /* Redundant? */
ip6_fib_timer.expires = 0;
}
spin_unlock_bh(&fib6_gc_lock);
+ if (from_timer) {
+ timer_exit(&ip6_fib_timer);
+ }
+}
+
+/* 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);
}
#endif
--- linux-2.4.0-test1-ac8/net/ipv6/ip6_flowlabel.c Fri Oct 29 07:34:44 1999
+++ linux-akpm/net/ipv6/ip6_flowlabel.c Mon Jun 5 23:12:27 2000
@@ -103,7 +103,14 @@
fl->opt = NULL;
kfree(opt);
}
- if (!del_timer(&ip6_fl_gc_timer) ||
+/*
+ * 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
+ */
+ if (!del_timer_async(&ip6_fl_gc_timer) ||
(long)(ip6_fl_gc_timer.expires - ttd) > 0)
ip6_fl_gc_timer.expires = ttd;
add_timer(&ip6_fl_gc_timer);
@@ -146,6 +153,7 @@
add_timer(&ip6_fl_gc_timer);
}
write_unlock(&ip6_fl_lock);
+ timer_exit(&ip6_fl_gc_timer);
}
static int fl_intern(struct ip6_flowlabel *fl, __u32 label)
@@ -615,7 +623,7 @@
void ip6_flowlabel_cleanup()
{
- del_timer(&ip6_fl_gc_timer);
+ del_timer_sync(&ip6_fl_gc_timer);
#ifdef CONFIG_PROC_FS
remove_proc_entry("net/ip6_flowlabel", 0);
#endif
--- linux-2.4.0-test1-ac8/net/ipv6/mcast.c Wed Feb 9 13:35:27 2000
+++ linux-akpm/net/ipv6/mcast.c Tue Jun 6 21:54:20 2000
@@ -363,7 +363,7 @@
if
(ipv6_addr_type(&ma->mca_addr)&(IPV6_ADDR_LINKLOCAL|IPV6_ADDR_LOOPBACK))
return;
- if (del_timer(&ma->mca_timer))
+ if (del_timer_async(&ma->mca_timer)) /* REVIEWME: see below */
delay = ma->mca_timer.expires - jiffies;
if (delay >= resptime) {
@@ -404,6 +404,10 @@
if (idev == NULL)
return 0;
+/*
+ * 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()
+ */
read_lock(&idev->lock);
if (ipv6_addr_any(addrp)) {
for (ma = idev->mc_list; ma; ma=ma->next)
@@ -450,11 +454,15 @@
* Cancel the timer for this group
*/
+/* 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
del_timer_sync().
+ * Andi says "Should not matter. it is a independent idev."
+ */
read_lock(&idev->lock);
for (ma = idev->mc_list; ma; ma=ma->next) {
if (ipv6_addr_cmp(&ma->mca_addr, addrp) == 0) {
if (ma->mca_flags & MAF_TIMER_RUNNING) {
- del_timer(&ma->mca_timer);
+ del_timer_async(&ma->mca_timer);
ma->mca_flags &= ~MAF_TIMER_RUNNING;
}
@@ -552,7 +560,7 @@
igmp6_send(&ma->mca_addr, ma->idev->dev, ICMPV6_MGM_REPORT);
delay = net_random() % IGMP6_UNSOLICITED_IVAL;
- if (del_timer(&ma->mca_timer))
+ if (del_timer_async(&ma->mca_timer))
delay = ma->mca_timer.expires - jiffies;
ma->mca_timer.expires = jiffies + delay;
@@ -573,8 +581,14 @@
if (ma->mca_flags & MAF_LAST_REPORTER)
igmp6_send(&ma->mca_addr, ma->idev->dev, ICMPV6_MGM_REDUCTION);
+/*
+ * REVIEWME: igmp6_timer_handler() could be running now. As soon as we return
+ * from igmp6_leave_group(), 'ma' is kfree'ed. Could igmp6_send() end up
+ * touching kfree'ed memory?
+ * Andi thinks it should be del_timer_sync here.
+ */
if (ma->mca_flags & MAF_TIMER_RUNNING)
- del_timer(&ma->mca_timer);
+ del_timer_async(&ma->mca_timer);
}
void igmp6_timer_handler(unsigned long data)
--- linux-2.4.0-test1-ac8/net/ipv6/reassembly.c Wed May 3 18:48:04 2000
+++ linux-akpm/net/ipv6/reassembly.c Tue Jun 6 22:03:06 2000
@@ -197,7 +197,13 @@
{
struct ipv6_frag *fp, *back;
- del_timer(&fq->timer);
+/* 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);
for (fp = fq->fragments; fp; ) {
frag_kfree_skb(fp->skb);
@@ -495,7 +501,10 @@
frag_kfree_s(back, sizeof(*back));
}
- del_timer(&fq->timer);
+/* REVIEWME: ip6_frag_lock is held now, so del_timer_sync() will deadlock.
+ * I think this code is safe anyway - the list is consistent when the lock is
released.
+ */
+ del_timer_async(&fq->timer);
fq->prev->next = fq->next;
fq->next->prev = fq->prev;
fq->prev = fq->next = NULL;
--- linux-2.4.0-test1-ac8/net/ipv6/route.c Sun Jan 23 06:54:58 2000
+++ linux-akpm/net/ipv6/route.c Tue Jun 6 21:47:14 2000
@@ -589,7 +589,7 @@
goto out;
expire++;
- fib6_run_gc(expire);
+ fib6_run_gc(expire, 0);
last_gc = now;
if (atomic_read(&ip6_dst_ops.entries) < ip6_dst_ops.gc_thresh)
expire = ip6_rt_gc_timeout>>1;
@@ -1882,7 +1882,7 @@
proc_dointvec(ctl, write, filp, buffer, lenp);
if (flush_delay < 0)
flush_delay = 0;
- fib6_run_gc((unsigned long)flush_delay);
+ fib6_run_gc((unsigned long)flush_delay, 0);
return 0;
} else
return -EINVAL;
--- linux-2.4.0-test1-ac8/include/net/ip6_fib.h Tue Aug 24 03:01:02 1999
+++ linux-akpm/include/net/ip6_fib.h Tue Jun 6 21:48:24 2000
@@ -174,7 +174,7 @@
extern void inet6_rt_notify(int event, struct rt6_info *rt);
-extern void fib6_run_gc(unsigned long dummy);
+extern void fib6_run_gc(unsigned long dummy, int
from_timer);
extern void fib6_gc_cleanup(void);
|