netdev
[Top] [All Lists]

timers in net/ipv6

To: Alexey Kuznetsov <kuznet@xxxxxxxxxxxxx>
Subject: timers in net/ipv6
From: Andrew Morton <andrewm@xxxxxxxxxx>
Date: Tue, 06 Jun 2000 23:07:52 +1000
Cc: "netdev@xxxxxxxxxxx" <netdev@xxxxxxxxxxx>
Sender: owner-netdev@xxxxxxxxxxx
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);
 
<Prev in Thread] Current Thread [Next in Thread>