Re: [tipc-discussion] [net v1 1/1] tipc: fix duplicate SYN messages under link congestion

[Jon M. <jon...@er...>]

Good observation. However, this re-introduces a problem I discovered and fixed.
When a socket is being shutdown, and may be in DISCONNCTING state, there may still be messages in the write queue, and those must be pushed out before we can delete the socket, otherwise the connection hasn't finished its task.  I noticed this because the benchmark program didn't finish until I had fixed this. 

I think the safest solution is to actually peek the first message in the queue and return if that is a SYN message.
That way, we could also eliminate the corresponding test I do in __tipc_shutdown(), and just call push_backlog() unconditionally there.

///jon





> -----Original Message-----
> From: Tung Nguyen <tun...@de...>
> Sent: 13-Nov-19 07:32
> To: tip...@li...; Jon Maloy <jon...@er...>; ma...@do...;
> yin...@wi...
> Subject: [tipc-discussion] [net v1 1/1] tipc: fix duplicate SYN messages under link congestion
> 
> Scenario:
> 1. A client socket initiates a SYN message to a listening socket.
> 2. The send link is congested, the SYN message is put in the
> send link and a wakeup message is put in wakeup queue.
> 3. The congestion situation is abated, the wakeup message is
> pulled out of the wakeup queue. Function tipc_sk_push_backlog()
> is called to send out delayed messages by Nagle. However,
> the client socket is still in CONNECTING state. So, it sends
> the SYN message in the socket write queue to the listening socket
> again.
> 4. The listening socket receives the first SYN message and creates
> first server socket. The client socket receives ACK- and establishes
> a connection to the first server socket. The client socket closes
> its connection with the first server socket.
> 5. The listening socket receives the second SYN message and creates
> second server socket. The second server socket sends ACK- to the
> client socket, but it has been closed. It results in connection
> refuse error when reading from the server socket in user space.
> 
> Solution: return from function tipc_sk_push_backlog() immediately
> if the client socket state is not ESTABLISHED.
> 
> Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
> Signed-off-by: Tung Nguyen <tun...@de...>
> ---
>  net/tipc/socket.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 5d7859a..61f9da4 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1246,13 +1246,18 @@ void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
>  static void tipc_sk_push_backlog(struct tipc_sock *tsk)
>  {
>  	struct sk_buff_head *txq = &tsk->sk.sk_write_queue;
> -	struct net *net = sock_net(&tsk->sk);
> +	struct sock *sk = &tsk->sk;
> +	struct net *net = sock_net(sk);
>  	u32 dnode = tsk_peer_node(tsk);
>  	int rc;
> 
>  	if (skb_queue_empty(txq) || tsk->cong_link_cnt)
>  		return;
> 
> +	/* Do not send SYN again after congestion */
> +	if (sk->sk_state != TIPC_ESTABLISHED)
> +		return;
> +
>  	tsk->snt_unacked += tsk->snd_backlog;
>  	tsk->snd_backlog = 0;
>  	tsk->expect_ack = true;
> --
> 2.1.4