Don't destroy TCP sockets twice

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.

index 6596b4f..f7cae15 100644
@@ -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);
 

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

-Andi

index a778ee0..bfbb06b 100644
@@ -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)

index 0207662..25bf80a 100644
@@ -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);

[email protected] -- Speaking for myself only.

On Tue, Aug 10, 2010 at 10:30:40AM +0200, Andi Kleen wrote:
.

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt