[PATCH v3 net-next 0/2] tcp: Disable header prediction for MD5.

[PATCH v3 net-next 0/2] tcp: Disable header prediction for MD5.

[Kuniyuki Iwashima]

The 1st patch disable header prediction for MD5 flow and the 2nd
patch updates the stale comment in tcp_parse_options().


Changes:
  v3:
    * Disable header prediction instead of enabling
    * Add the 2nd patch

  v2: https://lore.kernel.org/netdev/20230803042214.38309-1-kuniyu@amazon.com/
    * Update function graph

  v1: https://lore.kernel.org/all/20230803011658.17086-1-kuniyu@amazon.com/


Kuniyuki Iwashima (2):
  tcp: Disable header prediction for MD5 flow.
  tcp: Update stale comment for MD5 in tcp_parse_options().

 net/ipv4/tcp_input.c     | 5 ++---
 net/ipv4/tcp_minisocks.c | 2 --
 net/ipv4/tcp_output.c    | 5 -----
 3 files changed, 2 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH v3 net-next 1/2] tcp: Disable header prediction for MD5 flow.

[Kuniyuki Iwashima]

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.  We
add TCPOLEN_MD5SIG_ALIGNED to tcp_header_len in two paths to 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().

These two cases will have the wrong value in pred_flags and never go
into the fast path.

We could update tcp_header_len in tcp_rcv_synsent_state_process(), but
a test with slightly modified netperf which uses MD5 for each flow shows
that the slow path is actually a bit faster than the fast path.

  On c5.4xlarge EC2 instance (16 vCPU, 32 GiB mem)

  $ for i in {1..10}; do
  ./super_netperf $(nproc) -H localhost -l 10 -- -m 256 -M 256;
  done

  Avg of 10
  * 36e68eadd303  : 10.376 Gbps
  * all fast path : 10.374 Gbps (patch v2, See Link)
  * all slow path : 10.394 Gbps

The header prediction is not worth adding complexity for MD5, so let's
disable it for MD5.

Link: https://lore.kernel.org/netdev/20230803042214.38309-1-kuniyu@amazon.com/
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
v3:
  * Disable header prediction

v2: https://lore.kernel.org/netdev/20230803042214.38309-1-kuniyu@amazon.com/
  * Update function graph

v1: https://lore.kernel.org/netdev/20230803011658.17086-1-kuniyu@amazon.com/
---
 net/ipv4/tcp_minisocks.c | 2 --
 net/ipv4/tcp_output.c    | 5 -----
 2 files changed, 7 deletions(-)

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index c8f2aa003387..ddba3b67f7c8 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -570,8 +570,6 @@ struct sock *tcp_create_openreq_child(const struct sock *sk,
 	newtp->tsoffset = treq->ts_off;
 #ifdef CONFIG_TCP_MD5SIG
 	newtp->md5sig_info = NULL;	/*XXX*/
-	if (treq->af_specific->req_md5_lookup(sk, req_to_sk(req)))
-		newtp->tcp_header_len += TCPOLEN_MD5SIG_ALIGNED;
 #endif
 	if (skb->len >= TCP_MSS_DEFAULT + newtp->tcp_header_len)
 		newicsk->icsk_ack.last_seg_size = skb->len - newtp->tcp_header_len;
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 51d8638d4b4c..c5412ee77fc8 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3741,11 +3741,6 @@ static void tcp_connect_init(struct sock *sk)
 	if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_timestamps))
 		tp->tcp_header_len += TCPOLEN_TSTAMP_ALIGNED;
 
-#ifdef CONFIG_TCP_MD5SIG
-	if (tp->af_specific->md5_lookup(sk, sk))
-		tp->tcp_header_len += TCPOLEN_MD5SIG_ALIGNED;
-#endif
-
 	/* If user gave his TCP_MAXSEG, record it to clamp */
 	if (tp->rx_opt.user_mss)
 		tp->rx_opt.mss_clamp = tp->rx_opt.user_mss;
-- 
2.30.2

Re: [PATCH v3 net-next 1/2] tcp: Disable header prediction for MD5 flow.

[Eric Dumazet]

On Fri, Aug 4, 2023 at 12:46 AM Kuniyuki Iwashima <kuniyu@amazon.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.  We
> add TCPOLEN_MD5SIG_ALIGNED to tcp_header_len in two paths to 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().
>
> These two cases will have the wrong value in pred_flags and never go
> into the fast path.
>
> We could update tcp_header_len in tcp_rcv_synsent_state_process(), but
> a test with slightly modified netperf which uses MD5 for each flow shows
> that the slow path is actually a bit faster than the fast path.
>
>   On c5.4xlarge EC2 instance (16 vCPU, 32 GiB mem)
>
>   $ for i in {1..10}; do
>   ./super_netperf $(nproc) -H localhost -l 10 -- -m 256 -M 256;
>   done
>
>   Avg of 10
>   * 36e68eadd303  : 10.376 Gbps
>   * all fast path : 10.374 Gbps (patch v2, See Link)
>   * all slow path : 10.394 Gbps
>
> The header prediction is not worth adding complexity for MD5, so let's
> disable it for MD5.
>
> Link: https://lore.kernel.org/netdev/20230803042214.38309-1-kuniyu@amazon.com/
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

[PATCH v3 net-next 2/2] tcp: Update stale comment for MD5 in tcp_parse_options().

[Kuniyuki Iwashima]

Since commit 9ea88a153001 ("tcp: md5: check md5 signature without socket
lock"), the MD5 option is checked in tcp_v[46]_rcv().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/tcp_input.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 670c3dab24f2..2995802126e4 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4126,9 +4126,8 @@ void tcp_parse_options(const struct net *net,
 				break;
 #ifdef CONFIG_TCP_MD5SIG
 			case TCPOPT_MD5SIG:
-				/*
-				 * The MD5 Hash has already been
-				 * checked (see tcp_v{4,6}_do_rcv()).
+				/* The MD5 Hash has already been
+				 * checked (see tcp_v{4,6}_rcv()).
 				 */
 				break;
 #endif
-- 
2.30.2

Re: [PATCH v3 net-next 2/2] tcp: Update stale comment for MD5 in tcp_parse_options().

[Eric Dumazet]

On Fri, Aug 4, 2023 at 12:47 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Since commit 9ea88a153001 ("tcp: md5: check md5 signature without socket
> lock"), the MD5 option is checked in tcp_v[46]_rcv().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v3 net-next 0/2] tcp: Disable header prediction for MD5.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 3 Aug 2023 15:45:50 -0700 you wrote:
> The 1st patch disable header prediction for MD5 flow and the 2nd
> patch updates the stale comment in tcp_parse_options().
> 
> 
> Changes:
>   v3:
>     * Disable header prediction instead of enabling
>     * Add the 2nd patch
> 
> [...]

Here is the summary with links:
  - [v3,net-next,1/2] tcp: Disable header prediction for MD5 flow.
    https://git.kernel.org/netdev/net-next/c/d0f2b7a9ca0a
  - [v3,net-next,2/2] tcp: Update stale comment for MD5 in tcp_parse_options().
    https://git.kernel.org/netdev/net-next/c/b20515368932

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html