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]

Date: Thu, 03 Aug 2023 08:59:32 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Eric Dumazet <edumazet@...gle.com>, Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>,  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, 2023-08-03 at 08:44 +0200, Eric Dumazet wrote:
> 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.

Somewhat related, do you know how much header prediction makes a
difference for plain TCP? IIRC quite some time ago there was the idea
to remove header prediction completely to simplify the code overall -
then reverted because indeed caused regression in RR test-case. Do you
know if that is still true? would it make sense to re-evaluate that
thing (HP removal) again?

Thanks!

Paolo