diff mbox series

[5.10,099/167] tcp: disable TFO blackhole logic by default

Message ID 20210726153842.719316961@linuxfoundation.org
State New
Headers show
Series None | expand

Commit Message

Greg Kroah-Hartman July 26, 2021, 3:38 p.m. UTC 
From: Wei Wang <weiwan@google.com>

[ Upstream commit 213ad73d06073b197a02476db3a4998e219ddb06 ]

Multiple complaints have been raised from the TFO users on the internet
stating that the TFO blackhole logic is too aggressive and gets falsely
triggered too often.
(e.g. https://blog.apnic.net/2021/07/05/tcp-fast-open-not-so-fast/)
Considering that most middleboxes no longer drop TFO packets, we decide
to disable the blackhole logic by setting
/proc/sys/net/ipv4/tcp_fastopen_blackhole_timeout_set to 0 by default.

Fixes: cf1ef3f0719b4 ("net/tcp_fastopen: Disable active side TFO in certain scenarios")
Signed-off-by: Wei Wang <weiwan@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 Documentation/networking/ip-sysctl.rst | 2 +-
 net/ipv4/tcp_fastopen.c                | 9 ++++++++-
 net/ipv4/tcp_ipv4.c                    | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

Comments

Pavel Machek July 28, 2021, 10:12 a.m. UTC | #1 
Hi!

> [ Upstream commit 213ad73d06073b197a02476db3a4998e219ddb06 ]

> 

> Multiple complaints have been raised from the TFO users on the internet

> stating that the TFO blackhole logic is too aggressive and gets falsely

> triggered too often.

> (e.g. https://blog.apnic.net/2021/07/05/tcp-fast-open-not-so-fast/)

> Considering that most middleboxes no longer drop TFO packets, we decide

> to disable the blackhole logic by setting

> /proc/sys/net/ipv4/tcp_fastopen_blackhole_timeout_set to 0 by

> default.


I understand this makes sense for mainline, but should we have this in
stable? Somebody may still be using broken middlebox with their
"stable" server.

Best regards,
								Pavel

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Yuchung Cheng July 28, 2021, 4:32 p.m. UTC | #2 
On Wed, Jul 28, 2021 at 3:12 AM Pavel Machek <pavel@denx.de> wrote:
>

> Hi!

>

> > [ Upstream commit 213ad73d06073b197a02476db3a4998e219ddb06 ]

> >

> > Multiple complaints have been raised from the TFO users on the internet

> > stating that the TFO blackhole logic is too aggressive and gets falsely

> > triggered too often.

> > (e.g. https://blog.apnic.net/2021/07/05/tcp-fast-open-not-so-fast/)

> > Considering that most middleboxes no longer drop TFO packets, we decide

> > to disable the blackhole logic by setting

> > /proc/sys/net/ipv4/tcp_fastopen_blackhole_timeout_set to 0 by

> > default.

>

> I understand this makes sense for mainline, but should we have this in

> stable? Somebody may still be using broken middlebox with their

> "stable" server.

Thank you Pavel for raising this issue. You made a good point.

The enabled-by-default policy has caused disruptions to applications.
We have received quite a few others over the years beside the cited
report. Other major TFO implementations (e.g. iOS, Windows) do not
have such mechanisms and seem to work fine.

On the other hand maybe we do not hear middlebox issues because this
mechanism is working. So I am okay to avoid applying to stable and
keep in net-next to test this new policy.

>

> Best regards,

>                                                                 Pavel

>

> --

> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk

> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
diff mbox series

Patch

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 4abcfff15e38..4822a058a81d 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -751,7 +751,7 @@  tcp_fastopen_blackhole_timeout_sec - INTEGER
 	initial value when the blackhole issue goes away.
 	0 to disable the blackhole detection.
 
-	By default, it is set to 1hr.
+	By default, it is set to 0 (feature is disabled).
 
 tcp_fastopen_key - list of comma separated 32-digit hexadecimal INTEGERs
 	The list consists of a primary key and an optional backup key. The
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 08548ff23d83..d49709ba8e16 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -507,6 +507,9 @@  void tcp_fastopen_active_disable(struct sock *sk)
 {
 	struct net *net = sock_net(sk);
 
+	if (!sock_net(sk)->ipv4.sysctl_tcp_fastopen_blackhole_timeout)
+		return;
+
 	/* Paired with READ_ONCE() in tcp_fastopen_active_should_disable() */
 	WRITE_ONCE(net->ipv4.tfo_active_disable_stamp, jiffies);
 
@@ -526,10 +529,14 @@  void tcp_fastopen_active_disable(struct sock *sk)
 bool tcp_fastopen_active_should_disable(struct sock *sk)
 {
 	unsigned int tfo_bh_timeout = sock_net(sk)->ipv4.sysctl_tcp_fastopen_blackhole_timeout;
-	int tfo_da_times = atomic_read(&sock_net(sk)->ipv4.tfo_active_disable_times);
 	unsigned long timeout;
+	int tfo_da_times;
 	int multiplier;
 
+	if (!tfo_bh_timeout)
+		return false;
+
+	tfo_da_times = atomic_read(&sock_net(sk)->ipv4.tfo_active_disable_times);
 	if (!tfo_da_times)
 		return false;
 
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 5212db9ea157..04e259a04443 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2913,7 +2913,7 @@  static int __net_init tcp_sk_init(struct net *net)
 	net->ipv4.sysctl_tcp_comp_sack_nr = 44;
 	net->ipv4.sysctl_tcp_fastopen = TFO_CLIENT_ENABLE;
 	spin_lock_init(&net->ipv4.tcp_fastopen_ctx_lock);
-	net->ipv4.sysctl_tcp_fastopen_blackhole_timeout = 60 * 60;
+	net->ipv4.sysctl_tcp_fastopen_blackhole_timeout = 0;
 	atomic_set(&net->ipv4.tfo_active_disable_times, 0);
 
 	/* Reno is always built in */