netdev - Re: [PATCH v2 net] tcp: Enable header prediction for active open connections with MD5.

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iJbn2+KADkS_PJYvsm_hkSuxrp_TpYHcMDcdq71=VCSZQ@mail.gmail.com>
Date: Thu, 3 Aug 2023 08:44:14 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>, 
	YOSHIFUJI Hideaki <yoshfuji@...ux-ipv6.org>, Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net] tcp: Enable header prediction for active open
 connections with MD5.

On Thu, Aug 3, 2023 at 6:22 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> TCP socket saves the minimum required header length in tcp_header_len
> of struct tcp_sock, and later the value is used in __tcp_fast_path_on()
> to generate a part of TCP header in tcp_sock(sk)->pred_flags.
>
> In tcp_rcv_established(), if the incoming packet has the same pattern
> with pred_flags, we enter the fast path and skip full option parsing.
>
> The MD5 option is parsed in tcp_v[46]_rcv(), so we need not parse it
> again later in tcp_rcv_established() unless other options exist.  Thus,
> MD5 should add TCPOLEN_MD5SIG_ALIGNED to tcp_header_len and avoid the
> slow path.
>
> For passive open connections with MD5, we add TCPOLEN_MD5SIG_ALIGNED
> to tcp_header_len in tcp_create_openreq_child() after 3WHS.
>
> On the other hand, we do it in tcp_connect_init() for active open
> connections.  However, the value is overwritten while processing
> SYN+ACK or crossed SYN in tcp_rcv_synsent_state_process().
>
>   1) SYN+ACK
>
>     tcp_rcv_synsent_state_process
>       tp->tcp_header_len = sizeof(struct tcphdr) or
>                            sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED
>       tcp_finish_connect
>         __tcp_fast_path_on
>       tcp_send_ack
>
>   2) Crossed SYN and the following ACK
>
>     tcp_rcv_synsent_state_process
>       tp->tcp_header_len = sizeof(struct tcphdr) or
>                            sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED
>       tcp_set_state(sk, TCP_SYN_RECV)
>       tcp_send_synack
>
>     -- ACK received --
>     tcp_v4_rcv
>       tcp_v4_do_rcv
>         tcp_rcv_state_process
>           tcp_fast_path_on
>             __tcp_fast_path_on
>
> So these two cases will have the wrong value in pred_flags and never
> go into the fast path.
>
> Let's add TCPOLEN_MD5SIG_ALIGNED in tcp_rcv_synsent_state_process()
> to enable header prediction for active open connections.

I do not think we want to slow down fast path (no MD5), for 'header
prediction' of MD5 flows,
considering how slow MD5 is anyway (no GSO/GRO), and add yet another
ugly #ifdef CONFIG_TCP_MD5SIG
in already convoluted code base.

The case of cross-syn is kind of hilarious, if you ask me.

>
> Fixes: cfb6eeb4c860 ("[TCP]: MD5 Signature Option (RFC2385) support.")

This would be net-next material anyway, unless you show a huge
improvement after this patch,
which I doubt very much.
Or maybe the real intent is not fully expressed in your changelog ?