Merge branch 'Fix-reconnection-latency-caused-by-FIN-ACK-handling-race'

SeongJae Park says:

====================
Fix reconnection latency caused by FIN/ACK handling race

The first patch fixes the problem by adjusting the first resend delay of
the SYN in the case.  The second one adds a user space test to reproduce
this problem.

From v2
(https://lore.kernel.org/linux-kselftest/20200201071859.4231-1-sj38.park@gmail.com/)
 - Use TCP_TIMEOUT_MIN as reduced delay (Neal Cardwall)
 - Add Reviewed-by and Signed-off-by from Eric Dumazet

From v1
(https://lore.kernel.org/linux-kselftest/20200131122421.23286-1-sjpark@amazon.com/)
 - Drop the trivial comment fix patch (Eric Dumazet)
 - Limit the delay adjustment to only the first SYN resend (Eric Dumazet)
 - selftest: Avoid use of hard-coded port number (Eric Dumazet)
 - Explain RST/ACK and FIN/ACK has no big difference (Neal Cardwell)
====================
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

net/ipv4/tcp_input.c

@@ -5908,8 +5908,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 		 *        the segment and return)"
 		 */
 		if (!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_una) ||
-		    after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt))
+		    after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) {
+			/* Previous FIN/ACK or RST/ACK might be ignored. */
+			if (icsk->icsk_retransmits == 0)
+				inet_csk_reset_xmit_timer(sk,
+						ICSK_TIME_RETRANS,
+						TCP_TIMEOUT_MIN, TCP_RTO_MAX);
 			goto reset_and_undo;
+		}
 
 		if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr &&
 		    !between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp,

tools/testing/selftests/net/.gitignore

@@ -22,3 +22,4 @@ ipv6_flowlabel_mgr
 so_txtime
 tcp_fastopen_backup_key
 nettest
+fin_ack_lat

tools/testing/selftests/net/Makefile

@@ -11,6 +11,7 @@ TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
 TEST_PROGS += udpgro_bench.sh udpgro.sh test_vxlan_under_vrf.sh reuseport_addr_any.sh
 TEST_PROGS += test_vxlan_fdb_changelink.sh so_txtime.sh ipv6_flowlabel.sh
 TEST_PROGS += tcp_fastopen_backup_key.sh fcnal-test.sh l2tp.sh traceroute.sh
+TEST_PROGS += fin_ack_lat.sh
 TEST_PROGS_EXTENDED := in_netns.sh
 TEST_GEN_FILES =  socket nettest
 TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any
@@ -18,6 +19,7 @@ TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite
 TEST_GEN_FILES += udpgso udpgso_bench_tx udpgso_bench_rx ip_defrag
 TEST_GEN_FILES += so_txtime ipv6_flowlabel ipv6_flowlabel_mgr
 TEST_GEN_FILES += tcp_fastopen_backup_key
+TEST_GEN_FILES += fin_ack_lat
 TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
 TEST_GEN_PROGS += reuseport_dualstack reuseaddr_conflict tls
 

tools/testing/selftests/net/fin_ack_lat.c

+// SPDX-License-Identifier: GPL-2.0
+
+#include <arpa/inet.h>
+#include <errno.h>
+#include <error.h>
+#include <netinet/in.h>
+#include <netinet/tcp.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/socket.h>
+#include <sys/time.h>
+#include <unistd.h>
+
+static int child_pid;
+
+static unsigned long timediff(struct timeval s, struct timeval e)
+{
+	unsigned long s_us, e_us;
+
+	s_us = s.tv_sec * 1000000 + s.tv_usec;
+	e_us = e.tv_sec * 1000000 + e.tv_usec;
+	if (s_us > e_us)
+		return 0;
+	return e_us - s_us;
+}
+
+static void client(int port)
+{
+	int sock = 0;
+	struct sockaddr_in addr, laddr;
+	socklen_t len = sizeof(laddr);
+	struct linger sl;
+	int flag = 1;
+	int buffer;
+	struct timeval start, end;
+	unsigned long lat, sum_lat = 0, nr_lat = 0;
+
+	while (1) {
+		gettimeofday(&start, NULL);
+
+		sock = socket(AF_INET, SOCK_STREAM, 0);
+		if (sock < 0)
+			error(-1, errno, "socket creation");
+
+		sl.l_onoff = 1;
+		sl.l_linger = 0;
+		if (setsockopt(sock, SOL_SOCKET, SO_LINGER, &sl, sizeof(sl)))
+			error(-1, errno, "setsockopt(linger)");
+
+		if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
+					&flag, sizeof(flag)))
+			error(-1, errno, "setsockopt(nodelay)");
+
+		addr.sin_family = AF_INET;
+		addr.sin_port = htons(port);
+
+		if (inet_pton(AF_INET, "127.0.0.1", &addr.sin_addr) <= 0)
+			error(-1, errno, "inet_pton");
+
+		if (connect(sock, (struct sockaddr *)&addr, sizeof(addr)) < 0)
+			error(-1, errno, "connect");
+
+		send(sock, &buffer, sizeof(buffer), 0);
+		if (read(sock, &buffer, sizeof(buffer)) == -1)
+			error(-1, errno, "waiting read");
+
+		gettimeofday(&end, NULL);
+		lat = timediff(start, end);
+		sum_lat += lat;
+		nr_lat++;
+		if (lat < 100000)
+			goto close;
+
+		if (getsockname(sock, (struct sockaddr *)&laddr, &len) == -1)
+			error(-1, errno, "getsockname");
+		printf("port: %d, lat: %lu, avg: %lu, nr: %lu\n",
+				ntohs(laddr.sin_port), lat,
+				sum_lat / nr_lat, nr_lat);
+close:
+		fflush(stdout);
+		close(sock);
+	}
+}
+
+static void server(int sock, struct sockaddr_in address)
+{
+	int accepted;
+	int addrlen = sizeof(address);
+	int buffer;
+
+	while (1) {
+		accepted = accept(sock, (struct sockaddr *)&address,
+				(socklen_t *)&addrlen);
+		if (accepted < 0)
+			error(-1, errno, "accept");
+
+		if (read(accepted, &buffer, sizeof(buffer)) == -1)
+			error(-1, errno, "read");
+		close(accepted);
+	}
+}
+
+static void sig_handler(int signum)
+{
+	kill(SIGTERM, child_pid);
+	exit(0);
+}
+
+int main(int argc, char const *argv[])
+{
+	int sock;
+	int opt = 1;
+	struct sockaddr_in address;
+	struct sockaddr_in laddr;
+	socklen_t len = sizeof(laddr);
+
+	if (signal(SIGTERM, sig_handler) == SIG_ERR)
+		error(-1, errno, "signal");
+
+	sock = socket(AF_INET, SOCK_STREAM, 0);
+	if (sock < 0)
+		error(-1, errno, "socket");
+
+	if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR | SO_REUSEPORT,
+				&opt, sizeof(opt)) == -1)
+		error(-1, errno, "setsockopt");
+
+	address.sin_family = AF_INET;
+	address.sin_addr.s_addr = INADDR_ANY;
+	/* dynamically allocate unused port */
+	address.sin_port = 0;
+
+	if (bind(sock, (struct sockaddr *)&address, sizeof(address)) < 0)
+		error(-1, errno, "bind");
+
+	if (listen(sock, 3) < 0)
+		error(-1, errno, "listen");
+
+	if (getsockname(sock, (struct sockaddr *)&laddr, &len) == -1)
+		error(-1, errno, "getsockname");
+
+	fprintf(stderr, "server port: %d\n", ntohs(laddr.sin_port));
+	child_pid = fork();
+	if (!child_pid)
+		client(ntohs(laddr.sin_port));
+	else
+		server(sock, laddr);
+
+	return 0;
+}

tools/testing/selftests/net/fin_ack_lat.sh

+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test latency spikes caused by FIN/ACK handling race.
+
+set +x
+set -e
+
+tmpfile=$(mktemp /tmp/fin_ack_latency.XXXX.log)
+
+cleanup() {
+	kill $(pidof fin_ack_lat)
+	rm -f $tmpfile
+}
+
+trap cleanup EXIT
+
+do_test() {
+	RUNTIME=$1
+
+	./fin_ack_lat | tee $tmpfile &
+	PID=$!
+
+	sleep $RUNTIME
+	NR_SPIKES=$(wc -l $tmpfile | awk '{print $1}')
+	if [ $NR_SPIKES -gt 0 ]
+	then
+		echo "FAIL: $NR_SPIKES spikes detected"
+		return 1
+	fi
+	return 0
+}
+
+do_test "30"
+echo "test done"