Messages in this thread Patch in this message | | | Date | Sat, 13 Mar 1999 18:59:36 +0100 | From | Andi Kleen <> | Subject | Re: No connect timeout? |
| |
On Sat, Mar 13, 1999 at 05:32:29PM +0100, kuznet@ms2.inr.ac.ru wrote: > Hello! > > > David, Alexey, what do you think? > > At first sight, this fix does not change anything essentially. > inet_wait_for_connect cannot sleep on socket with sk->err. > sk->error_report() wakes up it after sk->err set, so that > the event cannot be lost.
What really happens is:
app calls connect() socket is set to SS_CONNECTING -> signal ICMP happens in between connect is restarted sees sk->err returns and clears error but does _not_ set SS_UNCONNECTED application retries connect inet_wait_for_connect (because the socket is still SYN_SENT) -> sleeps forever because no wake up happens and no error is set because it is already cleared.
Here is the better fix which always clears SS_CONNECTING, I'm still waiting for Jason's feedback on this one. Seems the "fix" to move the tcp_set_state(TCP_CLOSE) out of the icmp error handler was worse medicine than the original problem.
I also commented some other problems for later fixes:
Now still to find the pkt_too_big to self and copied < acked problems Jason reported. Alexey, do you have a theory for the pkt_too_big to self?
-Andi
--- linux/net/ipv4/af_inet.c-o Fri Mar 12 22:14:56 1999 +++ linux/net/ipv4/af_inet.c Sat Mar 13 11:11:17 1999 @@ -53,6 +53,7 @@ * David S. Miller : New socket lookup architecture. * Some other random speedups. * Cyrus Durgin : Cleaned up file for kmod hacks. + * Andi Kleen : Fix inet_stream_connect TCP race. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -613,14 +614,17 @@ } if(sock->state == SS_CONNECTING) { + /* Note: tcp_connected contains SYN_RECV, which may cause + bogus results here. -AK */ if(tcp_connected(sk->state)) { sock->state = SS_CONNECTED; return 0; } - if(sk->protocol == IPPROTO_TCP && (flags & O_NONBLOCK)) { - if(sk->err) - return sock_error(sk); - return -EALREADY; + if(sk->protocol == IPPROTO_TCP) { + if (sk->zapped || sk->err) + goto sock_error; + if (flags & O_NONBLOCK) + return -EALREADY; } } else { /* We may need to bind the socket. */ @@ -629,15 +633,17 @@ if (sk->prot->connect == NULL) return(-EOPNOTSUPP); err = sk->prot->connect(sk, uaddr, addr_len); + /* Note: there is a theoretical race here when an wake up + occurred before inet_wait_for_connect is entered. In 2.3 + the wait queue setup should be moved before the low level + connect call. -AK*/ if (err < 0) return(err); sock->state = SS_CONNECTING; } - if (sk->state > TCP_FIN_WAIT2 && sock->state == SS_CONNECTING) { - sock->state = SS_UNCONNECTED; - return sock_error(sk); - } + if (sk->state > TCP_FIN_WAIT2 && sock->state == SS_CONNECTING) + goto sock_error; if (sk->state != TCP_ESTABLISHED && (flags & O_NONBLOCK)) return (-EINPROGRESS); @@ -649,17 +655,19 @@ } sock->state = SS_CONNECTED; - if ((sk->state != TCP_ESTABLISHED) && sk->err) { - /* This is ugly but needed to fix a race in the ICMP error handler */ - if (sk->protocol == IPPROTO_TCP && sk->zapped) { - lock_sock(sk); - tcp_set_state(sk, TCP_CLOSE); - release_sock(sk); - } - sock->state = SS_UNCONNECTED; - return sock_error(sk); - } + if ((sk->state != TCP_ESTABLISHED) && sk->err) + goto sock_error; return(0); + +sock_error: + /* This is ugly but needed to fix a race in the ICMP error handler */ + if (sk->protocol == IPPROTO_TCP && sk->zapped) { + lock_sock(sk); + tcp_set_state(sk, TCP_CLOSE); + release_sock(sk); + } + sock->state = SS_UNCONNECTED; + return sock_error(sk); } /*
-- This is like TV. I don't like TV.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |