Hi, Alexey.
You're the first cab off the rank :)
Here are the results of reviewing net/ipv4/* for timer deletion safety.
I believe there are some races in there. I have marked these with the
string REVIEWME. Additional commentary is in ipv4.txt
I have attached a proposed patch in which I have made _all_ timer
handlers use del_timer_async, so this patch is a no-op.
(I'm really getting sick of staring at timer handlers)
--
-akpm- --- linux-2.4.0test1-ac5/net/ipv4/igmp.c Wed Apr 26 22:52:03 2000
+++ linux-akpm/net/ipv4/igmp.c Tue May 30 21:22:23 2000
@@ -142,7 +142,7 @@
static __inline__ void igmp_stop_timer(struct ip_mc_list *im)
{
spin_lock_bh(&im->lock);
- if (del_timer(&im->timer))
+ if (del_timer_async(&im->timer))
atomic_dec(&im->refcnt);
im->tm_running=0;
im->reporter = 0;
@@ -165,7 +165,7 @@
{
spin_lock_bh(&im->lock);
im->unsolicit_count = 0;
- if (del_timer(&im->timer)) {
+ if (del_timer_async(&im->timer)) {
if ((long)(im->timer.expires-jiffies) < max_delay) {
add_timer(&im->timer);
im->tm_running=1;
--- linux-2.4.0test1-ac5/net/ipv4/ip_fragment.c Sat Apr 29 21:22:41 2000
+++ linux-akpm/net/ipv4/ip_fragment.c Tue May 30 21:24:29 2000
@@ -150,7 +150,7 @@
qp->iph->saddr == saddr &&
qp->iph->daddr == daddr &&
qp->iph->protocol == protocol) {
- del_timer(&qp->timer);
+ del_timer_async(&qp->timer);
break;
}
}
@@ -170,7 +170,7 @@
struct ipfrag *fp;
/* Stop the timer for this entry. */
- del_timer(&qp->timer);
+ del_timer_async(&qp->timer);
/* Remove this entry from the "incomplete datagrams" queue. */
if(qp->next)
--- linux-2.4.0test1-ac5/net/ipv4/ipmr.c Mon May 15 21:24:15 2000
+++ linux-akpm/net/ipv4/ipmr.c Tue May 30 21:29:45 2000
@@ -756,10 +756,15 @@
uc->mfc_mcastgrp == c->mfc_mcastgrp) {
*cp = uc->next;
if (atomic_dec_and_test(&cache_resolve_queue_len))
- del_timer(&ipmr_expire_timer);
+ del_timer_async(&ipmr_expire_timer);
break;
}
}
+ /*
+ * REVIEWME: this function can be called from process context, yet I
+ * did a del_timer_async. Can we use del_timer_sync? Can the handler
+ * and this code fight each other?
+ */
spin_unlock_bh(&mfc_unres_lock);
if (uc) {
--- linux-2.4.0test1-ac5/net/ipv4/route.c Sat Apr 29 21:22:41 2000
+++ linux-akpm/net/ipv4/route.c Tue May 30 21:34:47 2000
@@ -395,7 +395,14 @@
spin_lock_bh(&rt_flush_lock);
- if (del_timer(&rt_flush_timer) && delay > 0 && rt_deadline) {
+/*
+ * REVIEWME: rt_cache_flush() can be called from process context. The above
+ * spinlock won't help us - rt_check_expire() could be running now (and
doesn't use
+ * the lock anyway
+ * Should we use del_timer_sync() here?
+ */
+
+ if (del_timer_async(&rt_flush_timer) && delay > 0 && rt_deadline) {
long tmo = (long)(rt_deadline - now);
/* If flush timer is already running
--- linux-2.4.0test1-ac5/net/ipv4/tcp_timer.c Mon May 15 21:24:15 2000
+++ linux-akpm/net/ipv4/tcp_timer.c Tue May 30 21:59:17 2000
@@ -81,7 +81,7 @@
* The delayed ack timer can be set if we are changing the
* retransmit timer when removing acked frames.
*/
- if (timer_pending(&tp->probe_timer) &&
del_timer(&tp->probe_timer))
+ if (timer_pending(&tp->probe_timer) &&
del_timer_async(&tp->probe_timer))
__sock_put(sk);
if (when > TCP_RTO_MAX) {
printk(KERN_DEBUG "reset_xmit_timer sk=%p when=0x%lx,
caller=%p\n", sk, when, NET_CALLER(sk));
@@ -110,14 +110,17 @@
{
struct tcp_opt *tp = &sk->tp_pinfo.af_tcp;
- if(timer_pending(&tp->retransmit_timer) &&
del_timer(&tp->retransmit_timer))
+/*
+ * REVIEWME: this function can be called from process context. See ipv4.txt
+ */
+ if(timer_pending(&tp->retransmit_timer) &&
del_timer_async(&tp->retransmit_timer))
__sock_put(sk);
- if(timer_pending(&tp->delack_timer) && del_timer(&tp->delack_timer))
+ if(timer_pending(&tp->delack_timer) &&
del_timer_async(&tp->delack_timer))
__sock_put(sk);
tp->ack.blocked = 0;
- if(timer_pending(&tp->probe_timer) && del_timer(&tp->probe_timer))
+ if(timer_pending(&tp->probe_timer) && del_timer_async(&tp->probe_timer))
__sock_put(sk);
- if(timer_pending(&sk->timer) && del_timer(&sk->timer))
+ if(timer_pending(&sk->timer) && del_timer_async(&sk->timer))
__sock_put(sk);
}
@@ -368,8 +371,12 @@
tw->pprev_death = NULL;
tcp_tw_put(tw);
if (--tcp_tw_count == 0)
- del_timer(&tcp_tw_timer);
+ del_timer_async(&tcp_tw_timer);
}
+/*
+ * REVIEWME: this function can be called from process context. Can it race
with tcp_twkill()?
+ * I think it can, because tcp_twkill could be spinning on tw_death_lock() on
another CPU right now!
+ */
spin_unlock(&tw_death_lock);
}
@@ -508,7 +515,7 @@
out:
if ((tcp_tw_count -= killed) == 0)
- del_timer(&tcp_tw_timer);
+ del_timer_async(&tcp_tw_timer);
net_statistics[smp_processor_id()*2].TimeWaitKilled += killed;
spin_unlock(&tw_death_lock);
}
@@ -710,7 +717,13 @@
void tcp_delete_keepalive_timer (struct sock *sk)
{
- if (timer_pending(&sk->timer) && del_timer (&sk->timer))
+/*
+ * REVIEWME. tcp_keepalive_timer can be called from process context. Can it
race
+ * against the timer handler?
+ *
+ * Also, do we need timer_pending here? It'll usually return true, perhaps?
+ */
+ if (timer_pending(&sk->timer) && del_timer_async(&sk->timer))
__sock_put(sk);
}
--- linux-2.4.0test1-ac5/include/net/tcp.h Mon May 15 21:24:15 2000
+++ linux-akpm/include/net/tcp.h Tue May 30 22:05:45 2000
@@ -1491,7 +1491,7 @@
return;
};
- if (timer_pending(timer) && del_timer(timer))
+ if (timer_pending(timer) && del_timer_async(timer))
__sock_put(sk);
}
igmp.c
======
igmp_start_timer()
OK as long as always called from BH context.
igmp_mod_timer()
OK as long as always called from BH context.
ip_fragment.c
=============
ip_find()
It says "We are always in BH context", so let's trust that.
ip_free()
Also called only from BH context.
ipmr.c
======
ipbr_mfc_add()
ALERT! Can be called from process context! Added REVIEWME
route.c
=======
ALERT! rt_cache_flush() is called from process context and can
apparently race wrt rt_check_expire(). Added a REVIEWME comment.
tcp_timer.c
===========
tcp_reset_xmit_timer()
OK, always called from timer handlers.
** Why bother calling timer_pending in here? The common case will
be that the timer _is_ pending, so just call del_timer_async???
tcp_clear_xmit_timers()
Called from tcp_disconnect()
Called from tcp_listen_stop()
Called from tcp_close()
ALERT! tcp_clear_xmit_timers() is called from process
context. The retransmit_timer, delack_timer, probe_timer and
sk->timer could still be running!
I used del_timer_async(), but this needs reviewing.
tcp_tw_deschedule()
Called from tcp_timewait_state_process()
Called from tcp_v4_rcv().
OK, BH context.
Called from tcp_v4_check_established()
Called from tcp_v4_hash_connecting()
Called from tcp_connect()
ALERT! tcp_tw_deschedule() called from process context!
Added a REVIEWME, used del_timer_async().
tcp_twcal_tick()
OK, it's a timer handler.
tcp_delete_keepalive_timer()
Called from tcp_listen_stop()
Called from tcp_close()
ALERT! tcp_delete_keepalive_timer() called from process context!
Can it race with the handler?
Added a REVIEWME.
include/net/tcp_timer.h
=======================
tcp_clear_xmit_timer():
Called from tcp_ack_probe()
Called from tcp_ack()
OK, BH only, methinks.
Called from tcp_ack()
OK, BH only.
** IS the timer_pending needed?
|