* [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets
@ 2012-09-18 23:35 H.K. Jerry Chu
2012-09-19 0:19 ` Christoph Paasch
0 siblings, 1 reply; 5+ messages in thread
From: H.K. Jerry Chu @ 2012-09-18 23:35 UTC (permalink / raw)
To: davem, netdev; +Cc: ncardwell, edumazet, Jerry Chu
From: Jerry Chu <hkchu@google.com>
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 <hkchu@google.com>
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets
2012-09-18 23:35 [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets H.K. Jerry Chu
@ 2012-09-19 0:19 ` Christoph Paasch
2012-09-19 2:14 ` Jerry Chu
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Christoph Paasch @ 2012-09-19 0:19 UTC (permalink / raw)
To: H.K. Jerry Chu; +Cc: davem, netdev, ncardwell, edumazet
On Tuesday 18 September 2012 16:35:51 H.K. Jerry Chu wrote:
> From: Jerry Chu <hkchu@google.com>
>
> 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 <hkchu@google.com>
> 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 <christoph.paasch@uclouvain.be>
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 <christoph.paasch@uclouvain.be>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets
2012-09-19 0:19 ` Christoph Paasch
@ 2012-09-19 2:14 ` Jerry Chu
2012-09-19 5:12 ` Eric Dumazet
2012-09-20 21:13 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: Jerry Chu @ 2012-09-19 2:14 UTC (permalink / raw)
To: Christoph Paasch; +Cc: davem, netdev, ncardwell, edumazet
On Tue, Sep 18, 2012 at 5:19 PM, Christoph Paasch
<christoph.paasch@uclouvain.be> wrote:
> On Tuesday 18 September 2012 16:35:51 H.K. Jerry Chu wrote:
>> From: Jerry Chu <hkchu@google.com>
>>
>> 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 <hkchu@google.com>
>> 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...)
>
>
> Cheers,
> Christoph
>
> ---------
>
> From: Christoph Paasch <christoph.paasch@uclouvain.be>
> 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 <christoph.paasch@uclouvain.be>
> ---
> 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
>
>
Acked-by: H.K. Jerry Chu <hkchu@google.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets
2012-09-19 0:19 ` Christoph Paasch
2012-09-19 2:14 ` Jerry Chu
@ 2012-09-19 5:12 ` Eric Dumazet
2012-09-20 21:13 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2012-09-19 5:12 UTC (permalink / raw)
To: Christoph Paasch; +Cc: H.K. Jerry Chu, davem, netdev, ncardwell, edumazet
On Wed, 2012-09-19 at 02:19 +0200, Christoph Paasch wrote:
> 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 <christoph.paasch@uclouvain.be>
> 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 <christoph.paasch@uclouvain.be>
> ---
> 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);
Yes, it seems much better, thanks !
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets
2012-09-19 0:19 ` Christoph Paasch
2012-09-19 2:14 ` Jerry Chu
2012-09-19 5:12 ` Eric Dumazet
@ 2012-09-20 21:13 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2012-09-20 21:13 UTC (permalink / raw)
To: christoph.paasch; +Cc: hkchu, netdev, ncardwell, edumazet
From: Christoph Paasch <christoph.paasch@uclouvain.be>
Date: Wed, 19 Sep 2012 02:19:23 +0200
> From: Christoph Paasch <christoph.paasch@uclouvain.be>
> 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 <christoph.paasch@uclouvain.be>
Applied, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-20 21:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-18 23:35 [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets H.K. Jerry Chu
2012-09-19 0:19 ` Christoph Paasch
2012-09-19 2:14 ` Jerry Chu
2012-09-19 5:12 ` Eric Dumazet
2012-09-20 21:13 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).