tcp/dccp: fix timewait races in timer handling

When creating a timewait socket, we need to arm the timer before
allowing other cpus to find it. The signal allowing cpus to find
the socket is setting tw_refcnt to non zero value.

As we set tw_refcnt in __inet_twsk_hashdance(), we therefore need to
call inet_twsk_schedule() first.

This also means we need to remove tw_refcnt changes from
inet_twsk_schedule() and let the caller handle it.

Note that because we use mod_timer_pinned(), we have the guarantee
the timer wont expire before we set tw_refcnt as we run in BH context.

To make things more readable I introduced inet_twsk_reschedule() helper.

When rearming the timer, we can use mod_timer_pending() to make sure
we do not rearm a canceled timer.

Note: This bug can possibly trigger if packets of a flow can hit
multiple cpus. This does not normally happen, unless flow steering
is broken somehow. This explains this bug was spotted ~5 months after
its introduction.

A similar fix is needed for SYN_RECV sockets in reqsk_queue_hash_req(),
but will be provided in a separate patch for proper tracking.

Fixes: 789f558cfb36 ("tcp/dccp: get rid of central timewait timer")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Ying Cai <ycai@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/inet_timewait_sock.h b/include/net/inet_timewait_sock.h
index 879d6e5a973b4ae1af54d6b0c6103c02ee774991..186f3a1e1b1f6ddd898d0f5871cb46222ae6b80d 100644 (file)
--- a/include/net/inet_timewait_sock.h
+++ b/include/net/inet_timewait_sock.h
@@ -110,7 +110,19 @@ struct inet_timewait_sock *inet_twsk_alloc(const struct sock *sk,
 void __inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk,
                           struct inet_hashinfo *hashinfo);
 
-void inet_twsk_schedule(struct inet_timewait_sock *tw, const int timeo);
+void __inet_twsk_schedule(struct inet_timewait_sock *tw, int timeo,
+                         bool rearm);
+
+static void inline inet_twsk_schedule(struct inet_timewait_sock *tw, int timeo)
+{
+       __inet_twsk_schedule(tw, timeo, false);
+}
+
+static void inline inet_twsk_reschedule(struct inet_timewait_sock *tw, int timeo)
+{
+       __inet_twsk_schedule(tw, timeo, true);
+}
+
 void inet_twsk_deschedule_put(struct inet_timewait_sock *tw);
 
 void inet_twsk_purge(struct inet_hashinfo *hashinfo,

diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c
index 30addee2dd037f9686c5585243e503632dcdd21a..838f524cf11a177b473ca83e3d7f597ee2904d21 100644 (file)
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -48,8 +48,6 @@ void dccp_time_wait(struct sock *sk, int state, int timeo)
                        tw->tw_ipv6only = sk->sk_ipv6only;
                }
 #endif
-               /* Linkage updates. */
-               __inet_twsk_hashdance(tw, sk, &dccp_hashinfo);
 
                /* Get the TIME_WAIT timeout firing. */
                if (timeo < rto)
@@ -60,6 +58,8 @@ void dccp_time_wait(struct sock *sk, int state, int timeo)
                        timeo = DCCP_TIMEWAIT_LEN;
 
                inet_twsk_schedule(tw, timeo);
+               /* Linkage updates. */
+               __inet_twsk_hashdance(tw, sk, &dccp_hashinfo);
                inet_twsk_put(tw);
        } else {
                /* Sorry, if we're out of memory, just CLOSE this

diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index ae22cc24fbe89b32be1f2142450c198e78026851..c67f9bd7699c5a1d210f214fd54aeea6944ccecb 100644 (file)
--- a/net/ipv4/inet_timewait_sock.c
+++ b/net/ipv4/inet_timewait_sock.c
@@ -123,13 +123,15 @@ void __inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk,
        /*
         * Step 2: Hash TW into tcp ehash chain.
         * Notes :
-        * - tw_refcnt is set to 3 because :
+        * - tw_refcnt is set to 4 because :
         * - We have one reference from bhash chain.
         * - We have one reference from ehash chain.
+        * - We have one reference from timer.
+        * - One reference for ourself (our caller will release it).
         * We can use atomic_set() because prior spin_lock()/spin_unlock()
         * committed into memory all tw fields.
         */
-       atomic_set(&tw->tw_refcnt, 1 + 1 + 1);
+       atomic_set(&tw->tw_refcnt, 4);
        inet_twsk_add_node_rcu(tw, &ehead->chain);
 
        /* Step 3: Remove SK from hash chain */
@@ -217,7 +219,7 @@ void inet_twsk_deschedule_put(struct inet_timewait_sock *tw)
 }
 EXPORT_SYMBOL(inet_twsk_deschedule_put);
 
-void inet_twsk_schedule(struct inet_timewait_sock *tw, const int timeo)
+void __inet_twsk_schedule(struct inet_timewait_sock *tw, int timeo, bool rearm)
 {
        /* timeout := RTO * 3.5
         *
@@ -245,12 +247,14 @@ void inet_twsk_schedule(struct inet_timewait_sock *tw, const int timeo)
         */
 
        tw->tw_kill = timeo <= 4*HZ;
-       if (!mod_timer_pinned(&tw->tw_timer, jiffies + timeo)) {
-               atomic_inc(&tw->tw_refcnt);
+       if (!rearm) {
+               BUG_ON(mod_timer_pinned(&tw->tw_timer, jiffies + timeo));
                atomic_inc(&tw->tw_dr->tw_count);
+       } else {
+               mod_timer_pending(&tw->tw_timer, jiffies + timeo);
        }
 }
-EXPORT_SYMBOL_GPL(inet_twsk_schedule);
+EXPORT_SYMBOL_GPL(__inet_twsk_schedule);
 
 void inet_twsk_purge(struct inet_hashinfo *hashinfo,
                     struct inet_timewait_death_row *twdr, int family)

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 6d8795b066aca708df47de3c9211f36bee5eb1d4..def765911ff85dfefd2c73c41b6585b33c77bcea 100644 (file)
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -162,9 +162,9 @@ kill_with_rst:
                if (tcp_death_row.sysctl_tw_recycle &&
                    tcptw->tw_ts_recent_stamp &&
                    tcp_tw_remember_stamp(tw))
-                       inet_twsk_schedule(tw, tw->tw_timeout);
+                       inet_twsk_reschedule(tw, tw->tw_timeout);
                else
-                       inet_twsk_schedule(tw, TCP_TIMEWAIT_LEN);
+                       inet_twsk_reschedule(tw, TCP_TIMEWAIT_LEN);
                return TCP_TW_ACK;
        }
 
@@ -201,7 +201,7 @@ kill:
                                return TCP_TW_SUCCESS;
                        }
                }
-               inet_twsk_schedule(tw, TCP_TIMEWAIT_LEN);
+               inet_twsk_reschedule(tw, TCP_TIMEWAIT_LEN);
 
                if (tmp_opt.saw_tstamp) {
                        tcptw->tw_ts_recent       = tmp_opt.rcv_tsval;
@@ -251,7 +251,7 @@ kill:
                 * Do not reschedule in the last case.
                 */
                if (paws_reject || th->ack)
-                       inet_twsk_schedule(tw, TCP_TIMEWAIT_LEN);
+                       inet_twsk_reschedule(tw, TCP_TIMEWAIT_LEN);
 
                return tcp_timewait_check_oow_rate_limit(
                        tw, skb, LINUX_MIB_TCPACKSKIPPEDTIMEWAIT);
@@ -322,9 +322,6 @@ void tcp_time_wait(struct sock *sk, int state, int timeo)
                } while (0);
 #endif
 
-               /* Linkage updates. */
-               __inet_twsk_hashdance(tw, sk, &tcp_hashinfo);
-
                /* Get the TIME_WAIT timeout firing. */
                if (timeo < rto)
                        timeo = rto;
@@ -338,6 +335,8 @@ void tcp_time_wait(struct sock *sk, int state, int timeo)
                }
 
                inet_twsk_schedule(tw, timeo);
+               /* Linkage updates. */
+               __inet_twsk_hashdance(tw, sk, &tcp_hashinfo);
                inet_twsk_put(tw);
        } else {
                /* Sorry, if we're out of memory, just CLOSE this