Re: [fixed] [patch] Re: [bug] stuck localhost TCP connections, v2.6.26-rc3+

[Joe Perches <joe@perches.com>]

On Wed, 2008-06-04 at 02:22 +0300, Ilpo Järvinen wrote:
> But here's somewhat more likely explanation... Only compile tested...
> It probably needs some commenting from people who understand locking 
> variants & details (I don't).
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index c9454f0..d21d2b9 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4562,6 +4562,7 @@ static int tcp_defer_accept_check(struct sock *sk)
>  	struct tcp_sock *tp = tcp_sk(sk);
>  
>  	if (tp->defer_tcp_accept.request) {
> +		struct sock *listen_sk = tp->defer_tcp_accept.listen_sk;

Not commenting on the locking, but I think the code
would be clearer if tp->defer_tcp_accept was used in
a temporary instead.

Perhaps:

 net/ipv4/tcp_input.c |   33 ++++++++++++++++-----------------
 1 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index b54d9d3..f846e11 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4535,18 +4535,18 @@ static void tcp_urg(struct sock *sk, struct sk_buff *skb, struct tcphdr *th)
 static int tcp_defer_accept_check(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-
-	if (tp->defer_tcp_accept.request) {
-		int queued_data =  tp->rcv_nxt - tp->copied_seq;
-		int hasfin =  !skb_queue_empty(&sk->sk_receive_queue) ?
+	struct tcp_deferred_accept_info *tdai = &tp->defer_tcp_accept;
+	if (tdai->request) {
+		int queued_data = tp->rcv_nxt - tp->copied_seq;
+		int hasfin = !skb_queue_empty(&sk->sk_receive_queue) ?
 			tcp_hdr((struct sk_buff *)
 				sk->sk_receive_queue.prev)->fin : 0;
 
 		if (queued_data && hasfin)
 			queued_data--;
 
-		if (queued_data &&
-		    tp->defer_tcp_accept.listen_sk->sk_state == TCP_LISTEN) {
+		bh_lock_sock(tdai->listen_sk);
+		if (queued_data && tdai->listen_sk->sk_state == TCP_LISTEN) {
 			if (sock_flag(sk, SOCK_KEEPOPEN)) {
 				inet_csk_reset_keepalive_timer(sk,
 							       keepalive_time_when(tp));
@@ -4554,23 +4554,22 @@ static int tcp_defer_accept_check(struct sock *sk)
 				inet_csk_delete_keepalive_timer(sk);
 			}
 
-			inet_csk_reqsk_queue_add(
-				tp->defer_tcp_accept.listen_sk,
-				tp->defer_tcp_accept.request,
-				sk);
+			inet_csk_reqsk_queue_add(tdai->listen_sk,
+						 tdai->request,
+						 sk);
 
-			tp->defer_tcp_accept.listen_sk->sk_data_ready(
-				tp->defer_tcp_accept.listen_sk, 0);
+			tdai->listen_sk->sk_data_ready(tdai->listen_sk, 0);
 
-			sock_put(tp->defer_tcp_accept.listen_sk);
+			sock_put(tdai->listen_sk);
 			sock_put(sk);
-			tp->defer_tcp_accept.listen_sk = NULL;
-			tp->defer_tcp_accept.request = NULL;
-		} else if (hasfin ||
-			   tp->defer_tcp_accept.listen_sk->sk_state != TCP_LISTEN) {
+			tdai->listen_sk = NULL;
+			tdai->request = NULL;
+		} else if (hasfin || tdai->listen_sk->sk_state != TCP_LISTEN) {
+			bh_unlock_sock(tdai->listen_sk);
 			tcp_reset(sk);
 			return -1;
 		}
+		bh_unlock_sock(tdai->listen_sk);
 	}
 	return 0;
 }