[PATCH v2 0/4] tcp: fix problems when tcp_fin_timeout is greater than 60

[PATCH v2 0/4] tcp: fix problems when tcp_fin_timeout is greater than 60

[Toshiaki Makita]

This is resubmission of the patch set that fixes problems when
tcp_fin_timeout is greater than 60.

- fix too short FIN_WAIT2 timer when tcp_fin_timeout is between 60
  and 120.
- fix FIN_WAIT2 timer expression in /proc/net/tcp and inet_diag when
  tcp_fin_timeout is greater than 60.

v2:
 - Separated changes of indentation to another patch.
 - Revised the comment of the patch modifying the expression of
   /proc/net/tcp to explain the reason for change.
 - Modified inet_diag.

Toshiaki Makita (4):
  tcp: fix too short FIN_WAIT2 time out
  tcp: fix FIN_WAIT2 timer expression in /proc/net/tcp
  tcp: fix the indentation in timer calculation of /proc/net/tcp
  inet_diag: fix FIN_WAIT2 timer expression in inet_diag

 include/linux/inet_diag.h |    2 ++
 net/dccp/diag.c           |   15 +++++++++++++++
 net/ipv4/inet_diag.c      |   20 --------------------
 net/ipv4/tcp_diag.c       |   27 +++++++++++++++++++++++++++
 net/ipv4/tcp_ipv4.c       |   23 +++++++++++++++--------
 net/ipv4/tcp_timer.c      |    8 ++------
 net/ipv6/tcp_ipv6.c       |   23 +++++++++++++++--------
 7 files changed, 76 insertions(+), 42 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/4] tcp: fix too short FIN_WAIT2 time out

[Toshiaki Makita]

When tcp_fin_timeout is between 60 and 120, a FIN_WAIT2 socket
disappears in (tcp_fin_timeout - 60) * 2 sec, which is shorter
than expected.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/ipv4/tcp_timer.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index b78aac3..c20e474 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -576,12 +576,8 @@ static void tcp_keepalive_timer (unsigned long data)
 
 	if (sk->sk_state == TCP_FIN_WAIT2 && sock_flag(sk, SOCK_DEAD)) {
 		if (tp->linger2 >= 0) {
-			const int tmo = tcp_fin_time(sk) - TCP_TIMEWAIT_LEN;
-
-			if (tmo > 0) {
-				tcp_time_wait(sk, TCP_FIN_WAIT2, tmo);
-				goto out;
-			}
+			tcp_time_wait(sk, TCP_FIN_WAIT2, TCP_TIMEWAIT_LEN);
+			goto out;
 		}
 		tcp_send_active_reset(sk, GFP_ATOMIC);
 		goto death;
-- 
1.7.10.4

[PATCH v2 2/4] tcp: fix FIN_WAIT2 timer expression in /proc/net/tcp

[Toshiaki Makita]

When tcp_fin_timeout is greater than 60 and a socket is half-closed,
/proc/net/tcp shows a FIN_WAIT2 keepalive timer. This is confusing
because keepalive probes will never be sent in this case.
Keepalive probes will really be sent with a FIN_WAIT2 keepalive timer
shown when a socket is not closed but shutdown and SO_KEEPALIVE is set.

Even without this change, a keepalive timer for an orphaned FIN_WAIT2
socket will eventually turn to a timewait timer, and by default,
or when tcp_fin_timeout is 60, there will be no keepalive timer for
orphaned FIN_WAIT2 sockets. Therefore, this change will hardly affect
a bad influence to userspace.

This patch changes the expression to show only timewait timer for
orphaned FIN_WAIT2 sockets.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/ipv4/tcp_ipv4.c |   11 +++++++++--
 net/ipv6/tcp_ipv6.c |   11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 4a8ec45..86a0685 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2666,8 +2666,15 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 		timer_active	= 4;
 		timer_expires	= icsk->icsk_timeout;
 	} else if (timer_pending(&sk->sk_timer)) {
-		timer_active	= 2;
-		timer_expires	= sk->sk_timer.expires;
+		if (sk->sk_state == TCP_FIN_WAIT2 && sock_flag(sk, SOCK_DEAD)) {
+			timer_active	= 3;
+			timer_expires	= sk->sk_timer.expires +
+					  (tp->linger2 >= 0 ?
+					   TCP_TIMEWAIT_LEN : 0);
+		} else {
+			timer_active	= 2;
+			timer_expires	= sk->sk_timer.expires;
+		}
 	} else {
 		timer_active	= 0;
 		timer_expires = jiffies;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 9b64600..b74b133 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1811,8 +1811,15 @@ static void get_tcp6_sock(struct seq_file *seq, struct sock *sp, int i)
 		timer_active	= 4;
 		timer_expires	= icsk->icsk_timeout;
 	} else if (timer_pending(&sp->sk_timer)) {
-		timer_active	= 2;
-		timer_expires	= sp->sk_timer.expires;
+		if (sp->sk_state == TCP_FIN_WAIT2 && sock_flag(sp, SOCK_DEAD)) {
+			timer_active	= 3;
+			timer_expires	= sp->sk_timer.expires +
+					  (tp->linger2 >= 0 ?
+					   TCP_TIMEWAIT_LEN : 0);
+		} else {
+			timer_active	= 2;
+			timer_expires	= sp->sk_timer.expires;
+		}
 	} else {
 		timer_active	= 0;
 		timer_expires = jiffies;
-- 
1.7.10.4

[PATCH v2 3/4] tcp: fix the indentation in timer calculation of /proc/net/tcp

[Toshiaki Makita]

Fix the indentation in get_tcp4_sock and get_tcp6_sock.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/ipv4/tcp_ipv4.c |   12 ++++++------
 net/ipv6/tcp_ipv6.c |   12 ++++++------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 86a0685..6f0ae53 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2660,11 +2660,11 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 	int rx_queue;
 
 	if (icsk->icsk_pending == ICSK_TIME_RETRANS) {
-		timer_active	= 1;
-		timer_expires	= icsk->icsk_timeout;
+		timer_active		= 1;
+		timer_expires		= icsk->icsk_timeout;
 	} else if (icsk->icsk_pending == ICSK_TIME_PROBE0) {
-		timer_active	= 4;
-		timer_expires	= icsk->icsk_timeout;
+		timer_active		= 4;
+		timer_expires		= icsk->icsk_timeout;
 	} else if (timer_pending(&sk->sk_timer)) {
 		if (sk->sk_state == TCP_FIN_WAIT2 && sock_flag(sk, SOCK_DEAD)) {
 			timer_active	= 3;
@@ -2676,8 +2676,8 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 			timer_expires	= sk->sk_timer.expires;
 		}
 	} else {
-		timer_active	= 0;
-		timer_expires = jiffies;
+		timer_active		= 0;
+		timer_expires		= jiffies;
 	}
 
 	if (sk->sk_state == TCP_LISTEN)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index b74b133..78cdb81 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1805,11 +1805,11 @@ static void get_tcp6_sock(struct seq_file *seq, struct sock *sp, int i)
 	srcp  = ntohs(inet->inet_sport);
 
 	if (icsk->icsk_pending == ICSK_TIME_RETRANS) {
-		timer_active	= 1;
-		timer_expires	= icsk->icsk_timeout;
+		timer_active		= 1;
+		timer_expires		= icsk->icsk_timeout;
 	} else if (icsk->icsk_pending == ICSK_TIME_PROBE0) {
-		timer_active	= 4;
-		timer_expires	= icsk->icsk_timeout;
+		timer_active		= 4;
+		timer_expires		= icsk->icsk_timeout;
 	} else if (timer_pending(&sp->sk_timer)) {
 		if (sp->sk_state == TCP_FIN_WAIT2 && sock_flag(sp, SOCK_DEAD)) {
 			timer_active	= 3;
@@ -1821,8 +1821,8 @@ static void get_tcp6_sock(struct seq_file *seq, struct sock *sp, int i)
 			timer_expires	= sp->sk_timer.expires;
 		}
 	} else {
-		timer_active	= 0;
-		timer_expires = jiffies;
+		timer_active		= 0;
+		timer_expires		= jiffies;
 	}
 
 	seq_printf(seq,
-- 
1.7.10.4

[PATCH v2 4/4] inet_diag: fix FIN_WAIT2 timer expression in inet_diag

[Toshiaki Makita]

This is a change similar to the patch for /proc/net/tcp.
FIN_WAIT2 timers for orphaned sockets are also shown in inet_diag.
There will be no keepalive timer for orphand FIN_WAIT2 sockets by
this change.
Besides, timer calculation of an inet_connection_sock is splitted
into two pieces because few common codes remain between timer
calculation of a tcp_sock and a dccp_sock: Special handling with
FIN_WAIT2 is needed for a tcp_sock, and consideration of
ICSK_TIME_PROBE0 is unnecessary for a dccp_sock.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 include/linux/inet_diag.h |    2 ++
 net/dccp/diag.c           |   15 +++++++++++++++
 net/ipv4/inet_diag.c      |   20 --------------------
 net/ipv4/tcp_diag.c       |   27 +++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
index 46da024..f7f359e 100644
--- a/include/linux/inet_diag.h
+++ b/include/linux/inet_diag.h
@@ -1,6 +1,7 @@
 #ifndef _INET_DIAG_H_
 #define _INET_DIAG_H_ 1
 
+#include <linux/kernel.h>
 #include <uapi/linux/inet_diag.h>
 
 struct sock;
@@ -27,6 +28,7 @@ struct inet_diag_handler {
 };
 
 struct inet_connection_sock;
+#define EXPIRES_IN_MS(tmo)  DIV_ROUND_UP((tmo - jiffies) * 1000, HZ)
 int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
 			      struct sk_buff *skb, struct inet_diag_req_v2 *req,
 			      struct user_namespace *user_ns,
diff --git a/net/dccp/diag.c b/net/dccp/diag.c
index 028fc43..b979a5b 100644
--- a/net/dccp/diag.c
+++ b/net/dccp/diag.c
@@ -42,6 +42,21 @@ static void dccp_get_info(struct sock *sk, struct tcp_info *info)
 static void dccp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
 			       void *_info)
 {
+	const struct inet_connection_sock *icsk = inet_csk(sk);
+
+	if (icsk->icsk_pending == ICSK_TIME_RETRANS) {
+		r->idiag_timer = 1;
+		r->idiag_retrans = icsk->icsk_retransmits;
+		r->idiag_expires = EXPIRES_IN_MS(icsk->icsk_timeout);
+	} else if (timer_pending(&sk->sk_timer)) {
+		r->idiag_timer = 2;
+		r->idiag_retrans = icsk->icsk_probes_out;
+		r->idiag_expires = EXPIRES_IN_MS(sk->sk_timer.expires);
+	} else {
+		r->idiag_timer = 0;
+		r->idiag_expires = 0;
+	}
+
 	r->idiag_rqueue = r->idiag_wqueue = 0;
 
 	if (_info != NULL)
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 7afa2c3..32c22a7 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -156,26 +156,6 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
 		goto out;
 	}
 
-#define EXPIRES_IN_MS(tmo)  DIV_ROUND_UP((tmo - jiffies) * 1000, HZ)
-
-	if (icsk->icsk_pending == ICSK_TIME_RETRANS) {
-		r->idiag_timer = 1;
-		r->idiag_retrans = icsk->icsk_retransmits;
-		r->idiag_expires = EXPIRES_IN_MS(icsk->icsk_timeout);
-	} else if (icsk->icsk_pending == ICSK_TIME_PROBE0) {
-		r->idiag_timer = 4;
-		r->idiag_retrans = icsk->icsk_probes_out;
-		r->idiag_expires = EXPIRES_IN_MS(icsk->icsk_timeout);
-	} else if (timer_pending(&sk->sk_timer)) {
-		r->idiag_timer = 2;
-		r->idiag_retrans = icsk->icsk_probes_out;
-		r->idiag_expires = EXPIRES_IN_MS(sk->sk_timer.expires);
-	} else {
-		r->idiag_timer = 0;
-		r->idiag_expires = 0;
-	}
-#undef EXPIRES_IN_MS
-
 	if (ext & (1 << (INET_DIAG_INFO - 1))) {
 		attr = nla_reserve(skb, INET_DIAG_INFO,
 				   sizeof(struct tcp_info));
diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index ed3f2ad..120a4250 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -20,9 +20,36 @@
 static void tcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
 			      void *_info)
 {
+	const struct inet_connection_sock *icsk = inet_csk(sk);
 	const struct tcp_sock *tp = tcp_sk(sk);
 	struct tcp_info *info = _info;
 
+	if (icsk->icsk_pending == ICSK_TIME_RETRANS) {
+		r->idiag_timer = 1;
+		r->idiag_retrans = icsk->icsk_retransmits;
+		r->idiag_expires = EXPIRES_IN_MS(icsk->icsk_timeout);
+	} else if (icsk->icsk_pending == ICSK_TIME_PROBE0) {
+		r->idiag_timer = 4;
+		r->idiag_retrans = icsk->icsk_probes_out;
+		r->idiag_expires = EXPIRES_IN_MS(icsk->icsk_timeout);
+	} else if (timer_pending(&sk->sk_timer)) {
+		if (sk->sk_state == TCP_FIN_WAIT2 && sock_flag(sk, SOCK_DEAD)) {
+			const unsigned long timer_expires =
+				sk->sk_timer.expires +
+				(tp->linger2 >= 0 ? TCP_TIMEWAIT_LEN : 0);
+
+			r->idiag_timer = 3;
+			r->idiag_expires = EXPIRES_IN_MS(timer_expires);
+		} else {
+			r->idiag_timer = 2;
+			r->idiag_retrans = icsk->icsk_probes_out;
+			r->idiag_expires = EXPIRES_IN_MS(sk->sk_timer.expires);
+		}
+	} else {
+		r->idiag_timer = 0;
+		r->idiag_expires = 0;
+	}
+
 	if (sk->sk_state == TCP_LISTEN) {
 		r->idiag_rqueue = sk->sk_ack_backlog;
 		r->idiag_wqueue = sk->sk_max_ack_backlog;
-- 
1.7.10.4

Re: [PATCH v2 1/4] tcp: fix too short FIN_WAIT2 time out

[David Miller]

From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Mon, 18 Mar 2013 21:39:04 +0900

>  		if (tp->linger2 >= 0) {
> -			const int tmo = tcp_fin_time(sk) - TCP_TIMEWAIT_LEN;
> -
> -			if (tmo > 0) {
> -				tcp_time_wait(sk, TCP_FIN_WAIT2, tmo);
> -				goto out;
> -			}
> +			tcp_time_wait(sk, TCP_FIN_WAIT2, TCP_TIMEWAIT_LEN);
> +			goto out;
>  		}

Well, now you're completely ignoring the user's linger setting.

I really can't take these patches seriously, and will not apply them,
sorry.

Re: [PATCH v2 1/4] tcp: fix too short FIN_WAIT2 time out

[Toshiaki Makita]

On Tue, 2013-03-19 at 09:32 -0400, David Miller wrote:
> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Date: Mon, 18 Mar 2013 21:39:04 +0900
> 
> >               if (tp->linger2 >= 0) {
> > -                     const int tmo = tcp_fin_time(sk) - TCP_TIMEWAIT_LEN;
> > -
> > -                     if (tmo > 0) {
> > -                             tcp_time_wait(sk, TCP_FIN_WAIT2, tmo);
> > -                             goto out;
> > -                     }
> > +                     tcp_time_wait(sk, TCP_FIN_WAIT2, TCP_TIMEWAIT_LEN);
> > +                     goto out;
> >               }
> 
> Well, now you're completely ignoring the user's linger setting.

If you mention TCP_LINGER2, I don't think I'm ignoring it.
It is taken into account in tcp_rcv_state_process() or tcp_close().
If I'm misunderstanding, I'd be glad if you could point out it.

> 
> I really can't take these patches seriously, and will not apply them,
> sorry.

I think, at least, too short timeout is harmful.
If tcp_fin_timeout is set to 61, it will expire in 2 seconds, which
cause peer to receive unexpected reset by sending fin.
Don't you think there is a problem?

Toshiaki Makita