linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [net-next PATCH v3] tcp: Add logic to check for SYN w/ data in tcp_simple_retransmit
@ 2020-12-12 20:31 Alexander Duyck
  2020-12-14 17:42 ` Eric Dumazet
  2020-12-15  3:40 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 4+ messages in thread
From: Alexander Duyck @ 2020-12-12 20:31 UTC (permalink / raw)
  To: edumazet, davem, kuba, ycheng
  Cc: kuznet, yoshfuji, netdev, linux-kernel, kafai, kernel-team

From: Alexander Duyck <alexanderduyck@fb.com>

There are cases where a fastopen SYN may trigger either a ICMP_TOOBIG
message in the case of IPv6 or a fragmentation request in the case of
IPv4. This results in the socket stalling for a second or more as it does
not respond to the message by retransmitting the SYN frame.

Normally a SYN frame should not be able to trigger a ICMP_TOOBIG or
ICMP_FRAG_NEEDED however in the case of fastopen we can have a frame that
makes use of the entire MSS. In the case of fastopen it does, and an
additional complication is that the retransmit queue doesn't contain the
original frames. As a result when tcp_simple_retransmit is called and
walks the list of frames in the queue it may not mark the frames as lost
because both the SYN and the data packet each individually are smaller than
the MSS size after the adjustment. This results in the socket being stalled
until the retransmit timer kicks in and forces the SYN frame out again
without the data attached.

In order to resolve this we can reduce the MSS the packets are compared
to in tcp_simple_retransmit to -1 for cases where we are still in the
TCP_SYN_SENT state for a fastopen socket. Doing this we will mark all of
the packets related to the fastopen SYN as lost.

Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---

v2: Changed logic to invalidate all retransmit queue frames if fastopen SYN
v3: Updated commit message to reflect actual solution in 3rd paragraph

 net/ipv4/tcp_input.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9e8a6c1aa019..e44327a39a1f 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2688,7 +2688,22 @@ void tcp_simple_retransmit(struct sock *sk)
 	const struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct sk_buff *skb;
-	unsigned int mss = tcp_current_mss(sk);
+	int mss;
+
+	/* A fastopen SYN request is stored as two separate packets within
+	 * the retransmit queue, this is done by tcp_send_syn_data().
+	 * As a result simply checking the MSS of the frames in the queue
+	 * will not work for the SYN packet.
+	 *
+	 * Us being here is an indication of a path MTU issue so we can
+	 * assume that the fastopen SYN was lost and just mark all the
+	 * frames in the retransmit queue as lost. We will use an MSS of
+	 * -1 to mark all frames as lost, otherwise compute the current MSS.
+	 */
+	if (tp->syn_data && sk->sk_state == TCP_SYN_SENT)
+		mss = -1;
+	else
+		mss = tcp_current_mss(sk);
 
 	skb_rbtree_walk(skb, &sk->tcp_rtx_queue) {
 		if (tcp_skb_seglen(skb) > mss)



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [net-next PATCH v3] tcp: Add logic to check for SYN w/ data in tcp_simple_retransmit
  2020-12-12 20:31 [net-next PATCH v3] tcp: Add logic to check for SYN w/ data in tcp_simple_retransmit Alexander Duyck
@ 2020-12-14 17:42 ` Eric Dumazet
  2020-12-14 18:52   ` Yuchung Cheng
  2020-12-15  3:40 ` patchwork-bot+netdevbpf
  1 sibling, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2020-12-14 17:42 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: David Miller, Jakub Kicinski, Yuchung Cheng, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, netdev, LKML, Martin KaFai Lau, kernel-team

On Sat, Dec 12, 2020 at 9:31 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> From: Alexander Duyck <alexanderduyck@fb.com>
>
> There are cases where a fastopen SYN may trigger either a ICMP_TOOBIG
> message in the case of IPv6 or a fragmentation request in the case of
> IPv4. This results in the socket stalling for a second or more as it does
> not respond to the message by retransmitting the SYN frame.
>
> Normally a SYN frame should not be able to trigger a ICMP_TOOBIG or
> ICMP_FRAG_NEEDED however in the case of fastopen we can have a frame that
> makes use of the entire MSS. In the case of fastopen it does, and an
> additional complication is that the retransmit queue doesn't contain the
> original frames. As a result when tcp_simple_retransmit is called and
> walks the list of frames in the queue it may not mark the frames as lost
> because both the SYN and the data packet each individually are smaller than
> the MSS size after the adjustment. This results in the socket being stalled
> until the retransmit timer kicks in and forces the SYN frame out again
> without the data attached.
>
> In order to resolve this we can reduce the MSS the packets are compared
> to in tcp_simple_retransmit to -1 for cases where we are still in the
> TCP_SYN_SENT state for a fastopen socket. Doing this we will mark all of
> the packets related to the fastopen SYN as lost.
>
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> ---
>

SGTM, thanks !

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

> v2: Changed logic to invalidate all retransmit queue frames if fastopen SYN
> v3: Updated commit message to reflect actual solution in 3rd paragraph
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [net-next PATCH v3] tcp: Add logic to check for SYN w/ data in tcp_simple_retransmit
  2020-12-14 17:42 ` Eric Dumazet
@ 2020-12-14 18:52   ` Yuchung Cheng
  0 siblings, 0 replies; 4+ messages in thread
From: Yuchung Cheng @ 2020-12-14 18:52 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Alexander Duyck, David Miller, Jakub Kicinski, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, netdev, LKML, Martin KaFai Lau, kernel-team

On Mon, Dec 14, 2020 at 9:42 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Sat, Dec 12, 2020 at 9:31 PM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > From: Alexander Duyck <alexanderduyck@fb.com>
> >
> > There are cases where a fastopen SYN may trigger either a ICMP_TOOBIG
> > message in the case of IPv6 or a fragmentation request in the case of
> > IPv4. This results in the socket stalling for a second or more as it does
> > not respond to the message by retransmitting the SYN frame.
> >
> > Normally a SYN frame should not be able to trigger a ICMP_TOOBIG or
> > ICMP_FRAG_NEEDED however in the case of fastopen we can have a frame that
> > makes use of the entire MSS. In the case of fastopen it does, and an
> > additional complication is that the retransmit queue doesn't contain the
> > original frames. As a result when tcp_simple_retransmit is called and
> > walks the list of frames in the queue it may not mark the frames as lost
> > because both the SYN and the data packet each individually are smaller than
> > the MSS size after the adjustment. This results in the socket being stalled
> > until the retransmit timer kicks in and forces the SYN frame out again
> > without the data attached.
> >
> > In order to resolve this we can reduce the MSS the packets are compared
> > to in tcp_simple_retransmit to -1 for cases where we are still in the
> > TCP_SYN_SENT state for a fastopen socket. Doing this we will mark all of
> > the packets related to the fastopen SYN as lost.
> >
> > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> > ---
> >
>
> SGTM, thanks !
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Nice work. I tested and verified it works with our packetdrill

Signed-off-by: Yuchung Cheng <ycheng@google.com>

>
> > v2: Changed logic to invalidate all retransmit queue frames if fastopen SYN
> > v3: Updated commit message to reflect actual solution in 3rd paragraph
> >

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [net-next PATCH v3] tcp: Add logic to check for SYN w/ data in tcp_simple_retransmit
  2020-12-12 20:31 [net-next PATCH v3] tcp: Add logic to check for SYN w/ data in tcp_simple_retransmit Alexander Duyck
  2020-12-14 17:42 ` Eric Dumazet
@ 2020-12-15  3:40 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2020-12-15  3:40 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: edumazet, davem, kuba, ycheng, kuznet, yoshfuji, netdev,
	linux-kernel, kafai, kernel-team

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Sat, 12 Dec 2020 12:31:24 -0800 you wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> There are cases where a fastopen SYN may trigger either a ICMP_TOOBIG
> message in the case of IPv6 or a fragmentation request in the case of
> IPv4. This results in the socket stalling for a second or more as it does
> not respond to the message by retransmitting the SYN frame.
> 
> [...]

Here is the summary with links:
  - [net-next,v3] tcp: Add logic to check for SYN w/ data in tcp_simple_retransmit
    https://git.kernel.org/netdev/net-next/c/c31b70c9968f

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-12-15  3:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-12 20:31 [net-next PATCH v3] tcp: Add logic to check for SYN w/ data in tcp_simple_retransmit Alexander Duyck
2020-12-14 17:42 ` Eric Dumazet
2020-12-14 18:52   ` Yuchung Cheng
2020-12-15  3:40 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).