Re: [tipc-discussion] [net] tipc: fix successful connect() but timed out

[Jon M. <jm...@re...>]

Acked-by: Jon Maloy <jm...@re...>


On 2/4/20 10:43 PM, Tuong Lien wrote:
> In commit 74cdc9035b82 ("tipc: fix wrong connect() return code"), we
> fixed the issue with the 'connect()' that returns zero even though the
> connecting has failed by waiting for the connection to be 'ESTABLISHED'
> really. However, the approach has one drawback in conjunction with our
> 'lightweight' connection setup mechanism that the following scenario
> can happen:
>
>            (server)                        (client)
>
>     +- accept()|                      |             wait_for_conn()
>     |          |                      |connect() -------+
>     |          |<-------[SYN]---------|                 > sleeping
>     |          |                      *CONNECTING       |
>     |--------->*ESTABLISHED           |                 |
>                |--------[ACK]-------->*ESTABLISHED      > wakeup()
>          send()|--------[DATA]------->|\                > wakeup()
>          send()|--------[DATA]------->| |               > wakeup()
>            .   .          .           . |-> recvq       .
>            .   .          .           . |               .
>          send()|--------[DATA]------->|/                > wakeup()
>         close()|--------[FIN]-------->*DISCONNECTING    |
>                *DISCONNECTING         |                 |
>                |                      ~~~~~~~~~~~~~~~~~~> schedule()
>                                                         | wait again
>                                                         .
>                                                         .
>                                                         | ETIMEDOUT
>
> Upon the receipt of the server 'ACK', the client becomes 'ESTABLISHED'
> and the 'wait_for_conn()' process is woken up but not run. Meanwhile,
> the server starts to send a number of data following by a 'close()'
> shortly without waiting any response from the client, which then forces
> the client socket to be 'DISCONNECTING' immediately. When the wait
> process is switched to be running, it continues to wait until the timer
> expires because of the unexpected socket state. The client 'connect()'
> will finally get ‘-ETIMEDOUT’ and force to release the socket whereas
> there remains the messages in its receive queue.
>
> Obviously the issue would not happen if the server had some delay prior
> to its 'close()' (or the number of 'DATA' messages is large enough),
> but any kind of delay would make the connection setup/shutdown "heavy".
> We solve this by simply allowing the 'connect()' returns zero in this
> particular case. The socket is already 'DISCONNECTING', so any further
> write will get '-EPIPE' but the socket is still able to read the
> messages existing in its receive queue.
>
> Note: This solution doesn't break the previous one as it deals with a
> different situation that the socket state is 'DISCONNECTING' but has no
> error (i.e. sk->sk_err = 0).
>
> Fixes: 74cdc9035b82 ("tipc: fix wrong connect() return code")
> Signed-off-by: Tuong Lien <tuo...@de...>
> ---
>   net/tipc/socket.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index f9b4fb92c0b1..693e8902161e 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2441,6 +2441,8 @@ static int tipc_wait_for_connect(struct socket *sock, long *timeo_p)
>   			return -ETIMEDOUT;
>   		if (signal_pending(current))
>   			return sock_intr_errno(*timeo_p);
> +		if (sk->sk_state == TIPC_DISCONNECTING)
> +			break;
>   
>   		add_wait_queue(sk_sleep(sk), &wait);
>   		done = sk_wait_event(sk, timeo_p, tipc_sk_connected(sk),

--