[PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets

From: Jerry Chu <redacted>

Crash dump msg looks like this:

<1>[34468.419809] BUG: unable to handle kernel paging request at ffffeb57000dc058
<1>[34468.426770] IP: [<ffffffff80383f9c>] kfree+0x4c/0x2d0
...
<4>[34468.603362] Call Trace:
<4>[34468.605802]  [<ffffffff807542a4>] inet_sock_destruct+0x174/0x1f0
<4>[34468.611786]  [<ffffffff806ced53>] __sk_free+0x23/0x170
<4>[34468.616907]  [<ffffffff806ceec5>] sk_free+0x25/0x30
<4>[34468.621762]  [<ffffffff806d066a>] sk_common_release+0x7a/0x80
<4>[34468.627481]  [<ffffffff80746782>] raw_close+0x22/0x30
<4>[34468.632515]  [<ffffffff80753038>] inet_release+0x58/0x90
<4>[34468.637802]  [<ffffffff806c9dd8>] sock_release+0x28/0x90
<4>[34468.643087]  [<ffffffff806c9f07>] sock_close+0x17/0x30
<4>[34468.648203]  [<ffffffff8039d60a>] fput+0xda/0x210
<4>[34468.652894]  [<ffffffff80398e06>] filp_close+0x66/0x90
<4>[34468.658016]  [<ffffffff802822cd>] put_files_struct+0x9d/0x120
<4>[34468.663743]  [<ffffffff802823fa>] exit_files+0x4a/0x60
<4>[34468.668857]  [<ffffffff802828e4>] do_exit+0x1a4/0x8c0
<4>[34468.673886]  [<ffffffff803697db>] ? do_munmap+0x2ab/0x390
<4>[34468.679256]  [<ffffffff80283384>] do_group_exit+0x44/0xa0
<4>[34468.684632]  [<ffffffff8028341f>] sys_exit_group+0x3f/0x50
<4>[34468.690094]  [<ffffffff807a5730>] sysenter_dispatch+0x7/0x1a

This bug was introduced as part of patch
7ab4551f3b391818e29263279031dca1e26417c6

It turns out the call "socket(PF_INET/PF_INET6, SOCK_RAW, IPPROTO_TCP)"
will cause a raw socket to be created with protocol == IPPROTO_TCP
so checking against the protocol field in sk alone is not sufficient
to guarantee a TCP socket. One must also check type == SOCK_STREAM.

Signed-off-by: H.K. Jerry Chu <redacted>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/af_inet.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 845372b..c9f0ea8 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -140,17 +140,22 @@ void inet_sock_destruct(struct sock *sk)
 
 	sk_mem_reclaim(sk);
 
-	if (sk->sk_type == SOCK_STREAM && sk->sk_state != TCP_CLOSE) {
-		pr_err("Attempt to release TCP socket in state %d %p\n",
-		       sk->sk_state, sk);
-		return;
-	}
 	if (!sock_flag(sk, SOCK_DEAD)) {
 		pr_err("Attempt to release alive inet socket %p\n", sk);
 		return;
 	}
-	if (sk->sk_protocol == IPPROTO_TCP)
-		kfree(inet_csk(sk)->icsk_accept_queue.fastopenq);
+	if (sk->sk_type == SOCK_STREAM) {
+		if (sk->sk_state != TCP_CLOSE) {
+			pr_err("Attempt to release TCP socket in state %d %p\n",
+				sk->sk_state, sk);
+			return;
+		}
+		/* Only TCP sockets (either v4 or v6) may have a valid
+		 * icsk_accept_queue.fastopenq that must be freed.
+		 */
+		if (sk->sk_protocol == IPPROTO_TCP)
+			kfree(inet_csk(sk)->icsk_accept_queue.fastopenq);
+	}
 
 	WARN_ON(atomic_read(&sk->sk_rmem_alloc));
 	WARN_ON(atomic_read(&sk->sk_wmem_alloc));

@@ -216,10 +221,11 @@ int inet_listen(struct socket *sock, int backlog)
 	if (old_state != TCP_LISTEN) {
 		/* Check special setups for testing purpose to enable TFO w/o
 		 * requiring TCP_FASTOPEN sockopt.
-		 * Note that only TCP sockets (SOCK_STREAM) will reach here.
-		 * Also fastopenq may already been allocated because this
-		 * socket was in TCP_LISTEN state previously but was
-		 * shutdown() (rather than close()).
+		 *
+		 * Note that only TCP sockets will reach here. Also fastopenq
+		 * may already been allocated because this socket was in
+		 * TCP_LISTEN state previously but was shutdown() (rather
+		 * than close()).
 		 */
 		if ((sysctl_tcp_fastopen & TFO_SERVER_ENABLE) != 0 &&
 		    inet_csk(sk)->icsk_accept_queue.fastopenq == NULL) {

-- 
1.7.7.3

On Tuesday 18 September 2012 16:35:51 H.K. Jerry Chu wrote:

From: Jerry Chu <redacted>

Crash dump msg looks like this:

<1>[34468.419809] BUG: unable to handle kernel paging request at
ffffeb57000dc058 <1>[34468.426770] IP: [<ffffffff80383f9c>]
kfree+0x4c/0x2d0
...
<4>[34468.603362] Call Trace:
<4>[34468.605802]  [<ffffffff807542a4>] inet_sock_destruct+0x174/0x1f0
<4>[34468.611786]  [<ffffffff806ced53>] __sk_free+0x23/0x170
<4>[34468.616907]  [<ffffffff806ceec5>] sk_free+0x25/0x30
<4>[34468.621762]  [<ffffffff806d066a>] sk_common_release+0x7a/0x80
<4>[34468.627481]  [<ffffffff80746782>] raw_close+0x22/0x30
<4>[34468.632515]  [<ffffffff80753038>] inet_release+0x58/0x90
<4>[34468.637802]  [<ffffffff806c9dd8>] sock_release+0x28/0x90
<4>[34468.643087]  [<ffffffff806c9f07>] sock_close+0x17/0x30
<4>[34468.648203]  [<ffffffff8039d60a>] fput+0xda/0x210
<4>[34468.652894]  [<ffffffff80398e06>] filp_close+0x66/0x90
<4>[34468.658016]  [<ffffffff802822cd>] put_files_struct+0x9d/0x120
<4>[34468.663743]  [<ffffffff802823fa>] exit_files+0x4a/0x60
<4>[34468.668857]  [<ffffffff802828e4>] do_exit+0x1a4/0x8c0
<4>[34468.673886]  [<ffffffff803697db>] ? do_munmap+0x2ab/0x390
<4>[34468.679256]  [<ffffffff80283384>] do_group_exit+0x44/0xa0
<4>[34468.684632]  [<ffffffff8028341f>] sys_exit_group+0x3f/0x50
<4>[34468.690094]  [<ffffffff807a5730>] sysenter_dispatch+0x7/0x1a

This bug was introduced as part of patch
7ab4551f3b391818e29263279031dca1e26417c6

It turns out the call "socket(PF_INET/PF_INET6, SOCK_RAW, IPPROTO_TCP)"
will cause a raw socket to be created with protocol == IPPROTO_TCP
so checking against the protocol field in sk alone is not sufficient
to guarantee a TCP socket. One must also check type == SOCK_STREAM.

Signed-off-by: H.K. Jerry Chu <redacted>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/af_inet.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

Why not moving the TCP-code out of inet_sock_destruct by modifying the sk_destruct
callback when TFO is in use? Like the below (only compile-tested) patch. That
way inet_sock_destruct stays TFO-free.


Cheers,
Christoph

---------

From: Christoph Paasch <redacted>
Date: Wed, 19 Sep 2012 02:06:53 +0200
Subject: [PATCH] Don't add TCP-code in inet_sock_destruct

Signed-off-by: Christoph Paasch <redacted>
---
 include/linux/tcp.h |    4 ++++
 net/ipv4/af_inet.c  |    2 --
 net/ipv4/tcp.c      |    7 +++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index ae46df5..67c789a 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h

@@ -574,6 +574,8 @@ static inline bool fastopen_cookie_present(struct tcp_fastopen_cookie *foc)
 	return foc->len != -1;
 }
 
+extern void tcp_sock_destruct(struct sock *sk);
+
 static inline int fastopen_init_queue(struct sock *sk, int backlog)
 {
 	struct request_sock_queue *queue =

@@ -585,6 +587,8 @@ static inline int fastopen_init_queue(struct sock *sk, int backlog)
 		    sk->sk_allocation);
 		if (queue->fastopenq == NULL)
 			return -ENOMEM;
+
+		sk->sk_destruct = tcp_sock_destruct;
 		spin_lock_init(&queue->fastopenq->lock);
 	}
 	queue->fastopenq->max_qlen = backlog;

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 845372b..766c596 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -149,8 +149,6 @@ void inet_sock_destruct(struct sock *sk)
 		pr_err("Attempt to release alive inet socket %p\n", sk);
 		return;
 	}
-	if (sk->sk_protocol == IPPROTO_TCP)
-		kfree(inet_csk(sk)->icsk_accept_queue.fastopenq);
 
 	WARN_ON(atomic_read(&sk->sk_rmem_alloc));
 	WARN_ON(atomic_read(&sk->sk_wmem_alloc));

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index df83d74..7b1e940 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -2325,6 +2325,13 @@ int tcp_disconnect(struct sock *sk, int flags)
 }
 EXPORT_SYMBOL(tcp_disconnect);
 
+void tcp_sock_destruct(struct sock *sk)
+{
+	inet_sock_destruct(sk);
+
+	kfree(inet_csk(sk)->icsk_accept_queue.fastopenq);
+}
+
 static inline bool tcp_can_repair_sock(const struct sock *sk)
 {
 	return capable(CAP_NET_ADMIN) &&

-- 
1.7.9.5

On Tue, Sep 18, 2012 at 5:19 PM, Christoph Paasch
[off-list ref] wrote:

On Tuesday 18 September 2012 16:35:51 H.K. Jerry Chu wrote:

quoted

From: Jerry Chu <redacted>

Crash dump msg looks like this:

<1>[34468.419809] BUG: unable to handle kernel paging request at
ffffeb57000dc058 <1>[34468.426770] IP: [<ffffffff80383f9c>]
kfree+0x4c/0x2d0
...
<4>[34468.603362] Call Trace:
<4>[34468.605802]  [<ffffffff807542a4>] inet_sock_destruct+0x174/0x1f0
<4>[34468.611786]  [<ffffffff806ced53>] __sk_free+0x23/0x170
<4>[34468.616907]  [<ffffffff806ceec5>] sk_free+0x25/0x30
<4>[34468.621762]  [<ffffffff806d066a>] sk_common_release+0x7a/0x80
<4>[34468.627481]  [<ffffffff80746782>] raw_close+0x22/0x30
<4>[34468.632515]  [<ffffffff80753038>] inet_release+0x58/0x90
<4>[34468.637802]  [<ffffffff806c9dd8>] sock_release+0x28/0x90
<4>[34468.643087]  [<ffffffff806c9f07>] sock_close+0x17/0x30
<4>[34468.648203]  [<ffffffff8039d60a>] fput+0xda/0x210
<4>[34468.652894]  [<ffffffff80398e06>] filp_close+0x66/0x90
<4>[34468.658016]  [<ffffffff802822cd>] put_files_struct+0x9d/0x120
<4>[34468.663743]  [<ffffffff802823fa>] exit_files+0x4a/0x60
<4>[34468.668857]  [<ffffffff802828e4>] do_exit+0x1a4/0x8c0
<4>[34468.673886]  [<ffffffff803697db>] ? do_munmap+0x2ab/0x390
<4>[34468.679256]  [<ffffffff80283384>] do_group_exit+0x44/0xa0
<4>[34468.684632]  [<ffffffff8028341f>] sys_exit_group+0x3f/0x50
<4>[34468.690094]  [<ffffffff807a5730>] sysenter_dispatch+0x7/0x1a

This bug was introduced as part of patch
7ab4551f3b391818e29263279031dca1e26417c6

It turns out the call "socket(PF_INET/PF_INET6, SOCK_RAW, IPPROTO_TCP)"
will cause a raw socket to be created with protocol == IPPROTO_TCP
so checking against the protocol field in sk alone is not sufficient
to guarantee a TCP socket. One must also check type == SOCK_STREAM.

Signed-off-by: H.K. Jerry Chu <redacted>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/af_inet.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

Why not moving the TCP-code out of inet_sock_destruct by modifying the sk_destruct
callback when TFO is in use? Like the below (only compile-tested) patch. That
way inet_sock_destruct stays TFO-free.

That will work too. (I briefly thought about this but was distracted by
the two pr_err() returns...)

Acked-by: H.K. Jerry Chu <redacted>

On Wed, 2012-09-19 at 02:19 +0200, Christoph Paasch wrote:

Yes, it seems much better, thanks !

Acked-by: Eric Dumazet <edumazet@google.com>

From: Christoph Paasch <redacted>
Date: Wed, 19 Sep 2012 02:19:23 +0200

From: Christoph Paasch <redacted>
Date: Wed, 19 Sep 2012 02:06:53 +0200
Subject: [PATCH] Don't add TCP-code in inet_sock_destruct

Signed-off-by: Christoph Paasch <redacted>

Applied, thanks.