[RFC] [PATCH] Don't destroy TCP sockets twice

[RFC] [PATCH] Don't destroy TCP sockets twice

[Andi Kleen]

While working on something else I noticed that tcp_v4/6_destroy_sock()
can get called twice on a socket. This happens because when a reset or
similar happens tcp_done destroys the connection socket state, and 
then eventually when the socket is released it is destroyed again. 

Problem here is that the socket can actually still transmit packets
even after having been destroyed by tcp_done: this happens because
tcp_close is called after tcp_done, tcp_close calls tcp_send_fin which
then sends the FIN at least.

With my local modifications this lead to crashes (I destroyed some
additional state in destroy_sock that is needed for sending any
packets). But I think it's broken even without any changes: tcp_*_destroy
sock removes a lot of state, including MD5 state, congestion control
state and some others.

At least for MD5 it's easy to argue that the the FIN still needs this
state. I haven't tested that, but I think what will happen is that
you'll see FINs which are not MD5 signed after a reset (there is no
crash because zero MD5 state is ignored) There might be other similar
problems with congestion avoidance state and the FIN packet.

Overall it also seems inefficient to destroy twice.

The only thing in tcp_*_destroy sock that I think really needs to be
done is clearing the timers, but tcp_done does this already too.

So this patch changes tcp_done to not call inet_csk_destroy_sock()
and leave the destruction to the later final release or close.

The only drawback is that the buffers could be freed a bit later
for the RST case (e.g. when there is a reference to user space
it will wait for the user close), but that doesn't seem like a big 
issue.

I made this a RFC for now because it needs some more review.

Signed-off-by: Andi Kleen <ak@linux.intel.com>

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 6596b4f..f7cae15 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3171,8 +3171,6 @@ void tcp_done(struct sock *sk)
 
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk->sk_state_change(sk);
-	else
-		inet_csk_destroy_sock(sk);
 }
 EXPORT_SYMBOL_GPL(tcp_done);
 

Re: [RFC] [PATCH] Don't destroy TCP sockets twice

[Herbert Xu]

Andi Kleen <andi@firstfloor.org> wrote:
> 
> While working on something else I noticed that tcp_v4/6_destroy_sock()
> can get called twice on a socket. This happens because when a reset or
> similar happens tcp_done destroys the connection socket state, and 
> then eventually when the socket is released it is destroyed again. 

I'm still having problems understanding why you're getting a call
to send a FIN after tcp_done.  This shouldn't happen at all because
tcp_done moves the socket to the TCP_CLOSE state, where FINs should
not be sent.

Can you clarify on how we can reproduce this problem in the
upstream kernel?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] [PATCH] Don't destroy TCP sockets twice

[Andi Kleen]

On Mon, Aug 09, 2010 at 05:30:02PM -0400, Herbert Xu wrote:
> Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > While working on something else I noticed that tcp_v4/6_destroy_sock()
> > can get called twice on a socket. This happens because when a reset or
> > similar happens tcp_done destroys the connection socket state, and 
> > then eventually when the socket is released it is destroyed again. 
> 
> I'm still having problems understanding why you're getting a call
> to send a FIN after tcp_done.  This shouldn't happen at all because
> tcp_done moves the socket to the TCP_CLOSE state, where FINs should
> not be sent.
> 
> Can you clarify on how we can reproduce this problem in the
>  upstream kernel?

This simple patch demonstrates double destroy. I have patches for showing
the more complicated case too, but they're much more ugly.

-Andi

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index a778ee0..bfbb06b 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -459,6 +459,8 @@ struct tcp_sock {
 	 * contains related tcp_cookie_transactions fields.
 	 */
 	struct tcp_cookie_values  *cookie_values;
+
+	int destroyed;
 };
 
 static inline struct tcp_sock *tcp_sk(const struct sock *sk)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 0207662..25bf80a 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1919,6 +1919,9 @@ void tcp_v4_destroy_sock(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 
+	BUG_ON(tp->destroyed != 0);
+	tp->destroyed = 1;
+
 	tcp_clear_xmit_timers(sk);
 
 	tcp_cleanup_congestion_control(sk);
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFC] [PATCH] Don't destroy TCP sockets twice

[Herbert Xu]

On Tue, Aug 10, 2010 at 10:30:40AM +0200, Andi Kleen wrote:
. 
> This simple patch demonstrates double destroy. I have patches for showing
> the more complicated case too, but they're much more ugly.

Andi, I know you're seeing the problem, but I need to udnerstand
why, and this patch doesn't really answer the why part :)

So did you figure out why was calling it first (I presume you
know who called it the second time since you've got the back
trace)?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] [PATCH] Don't destroy TCP sockets twice

[Andi Kleen]

On Tue, Aug 10, 2010 at 06:24:21AM -0400, Herbert Xu wrote:
> On Tue, Aug 10, 2010 at 10:30:40AM +0200, Andi Kleen wrote:
> . 
> > This simple patch demonstrates double destroy. I have patches for showing
> > the more complicated case too, but they're much more ugly.
> 
> Andi, I know you're seeing the problem, but I need to udnerstand
> why, and this patch doesn't really answer the why part :)
> 
> So did you figure out why was calling it first (I presume you
> know who called it the second time since you've got the back
> trace)?

Yes I stored the backtrace of the first caller in the ugly debug
patches and dumped that on the second destroy. It was tcp_done the 
first time.

Also did the same for tcp_sk() and there it was the fin sending.

I agree that tcp_close() should skip it in theory but I saw
it anyways :/

-Andi

Re: [RFC] [PATCH] Don't destroy TCP sockets twice

[Herbert Xu]

On Tue, Aug 10, 2010 at 12:32:06PM +0200, Andi Kleen wrote:
>
> Yes I stored the backtrace of the first caller in the ugly debug
> patches and dumped that on the second destroy. It was tcp_done the 
> first time.
> 
> Also did the same for tcp_sk() and there it was the fin sending.
> 
> I agree that tcp_close() should skip it in theory but I saw
> it anyways :/

So I presume the second caller was tcp_close? That means we have
a serious bug in our stack, as if the socket is already in the
CLOSE state then tcp_close should have short-circuited.

This means that something is changing the TCP socket state after
going into CLOSE.  Can you try adding your debug backtrace patch to
tcp_set_state to see if anybody is indeed changing the socket
state after going into CLOSE (and more importantly who is changing
it)?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt