netdev - Re: [PATCH] tcp: Reset tcp connections in SYN-SENT state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iJPgzWWor7YeGZvVH92gbJmD96Nwi=1pjfbXO8AgsjyNA@mail.gmail.com>
Date:   Wed, 20 Feb 2019 07:47:59 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Devi Sandeep Endluri V V <dendluri@...eaurora.org>
Cc:     netdev <netdev@...r.kernel.org>, subashab@...eaurora.org,
        sharathv@...eaurora.org, ssaha@...eaurora.org,
        strenche@...eaurora.org
Subject: Re: [PATCH] tcp: Reset tcp connections in SYN-SENT state

On Wed, Feb 20, 2019 at 6:28 AM Devi Sandeep Endluri V V
<dendluri@...eaurora.org> wrote:
>
> Userspace sends tcp connection (sock) destroy on network permission
> change. Kernel though doesn't send reset for the connections in
> SYN-SENT state and these connections continue to remain. Even as
> per RFC 793, there is no hard rule to not send RST on ABORT in
> this state. Change to make sure RST are send for connections in
> syn-sent state to avoid lingering connections on network switch.
>
> References from RFC 793
>
> ABORT Call
>
>         SYN-SENT STATE
>
>         All queued SENDs and RECEIVEs should be given "connection reset"
>         notification, delete the TCB, enter CLOSED state, and return.
>
> SEGMENT ARRIVES
>
>         If the state is SYN-SENT then
>         If the RST bit is set
>
>           If the ACK was acceptable then signal the user "error:
>           connection reset", drop the segment, enter CLOSED state,
>           delete TCB, and return.  Otherwise (no ACK) drop the segment
>           and return.
>
> Signed-off-by: Devi Sandeep Endluri V V <dendluri@...eaurora.org>
> ---
>  net/ipv4/tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git net/ipv4/tcp.c net/ipv4/tcp.c
> index cf3c509..8569dc5e 100644
> --- net/ipv4/tcp.c
> +++ net/ipv4/tcp.c
> @@ -2495,7 +2495,7 @@ static inline bool tcp_need_reset(int state)
>  {
>         return (1 << state) &
>                (TCPF_ESTABLISHED | TCPF_CLOSE_WAIT | TCPF_FIN_WAIT1 |
> -               TCPF_FIN_WAIT2 | TCPF_SYN_RECV);
> +               TCPF_FIN_WAIT2 | TCPF_SYN_RECV | TCPF_SYN_SENT);
>  }
>
>  static void tcp_rtx_queue_purge(struct sock *sk)

Hi

1) I do not believe this patch is complete :
   You have not changed tcp_disconnect() which seems to have dead code
if we apply your patch.

2) I am not sure we want to send yet another packet if the prior SYN
packets elect no answer.
  (It is not clear if your patch applies to this case)

3) If we do not RESET, the other side has not seen the 3rd packet and
should eventually exit from SYN_RECV state and die.

4) A RESET can be lost on the network, and nothing will retransmit it.

Can you describe the use case more precisely, because it seems in
contradiction of a feature that we plan to upstream.
(TCP_SILENT_CLOSE : do not send flood of RST/FIN when a gigantic
server process prematurely dies)

Thanks.