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: <CANn89i+OR9fDEN6w385KP1ZSO4qjcU7TAbO7p0jiKnqjKAH+bg@mail.gmail.com>
Date: Thu, 3 Aug 2023 09:07:10 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>, Florian Westphal <fw@...len.de>
Cc: Kuniyuki Iwashima <kuniyu@...zon.com>, "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, Aug 3, 2023 at 8:59 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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?
>

I think Florian did this, he might recall the details.