[PATCH net 0/5] net: netdevsim: fix inconsistent carrier state after link/unlink

[PATCH net 0/5] net: netdevsim: fix inconsistent carrier state after link/unlink

[yk]

From: Yohei Kojima <yk@y-koj.net>

This series fixes netdevsim's inconsistent behavior between carrier
and link/unlink state.

More specifically, this fixes a bug that the carrier goes DOWN although
two netdevsim were peered, depending on the order of peering and ifup.
Especially in a NetworkManager-enabled environment, netdevsim test fails
because of this.

The first patch fixes the bug itself in netdevsim/bus.c by adding
netif_carrier_on() into a proper function. The second and third patches
clean up netdevsim test and add a regression test for this bug.

The fourth and fifth patches improve TCP Fast Open (TFO) test, which
depends on netdevsim. In a NetworkManager-enabled environment, although
TFO test times out because of this bug, the test exits with 0 without
reporting any error.  This behavior implies that nothing would be
reported even if TFO got broken at some point.

The fourth and fifth patches are intentionally placed after the first
patch, because fixing TFO test without fixing netdevsim results in
a spurious test failure in a NetworkManager-enabled environment.

Yohei Kojima (5):
  net: netdevsim: fix inconsistent carrier state after link/unlink
  selftests: netdevsim: test that linking already-connected devices
    fails
  selftests: netdevsim: add carrier state consistency test
  selftests: net: improve error handling in TFO test
  selftests: net: report SKIP if TFO test processes timed out

 drivers/net/netdevsim/bus.c                   |  6 ++
 .../selftests/drivers/net/netdevsim/peer.sh   | 79 ++++++++++++++++++-
 tools/testing/selftests/net/tfo.c             | 10 ++-
 tools/testing/selftests/net/tfo_passive.sh    | 15 +++-
 4 files changed, 101 insertions(+), 9 deletions(-)

-- 
2.51.2

[PATCH net 2/5] selftests: netdevsim: test that linking already-connected devices fails

[yk]

From: Yohei Kojima <yk@y-koj.net>

This patch adds a testcase to check if linking already-connected
netdevsim interfaces fails.

This patch also moves the testcase on invalid ifidx before linking
two netdevsims so that the test would fail if argument validation code
got broken: after linking two netdevsims, the test might not fail
because it attempts to link an already-connected netdevsim with
non-existing netdevsim.

Additionally, this patch adds comments for readability and details the
error message.

Signed-off-by: Yohei Kojima <yk@y-koj.net>
---
 .../selftests/drivers/net/netdevsim/peer.sh      | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/netdevsim/peer.sh b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
index 7f32b5600925..338c844fe632 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/peer.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
@@ -76,6 +76,7 @@ NSIM_DEV_2_FD=$((256 + RANDOM % 256))
 exec {NSIM_DEV_2_FD}</var/run/netns/nscl
 NSIM_DEV_2_IFIDX=$(ip netns exec nscl cat /sys/class/net/$NSIM_DEV_2_NAME/ifindex)
 
+# argument error checking
 echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:2000" > $NSIM_DEV_SYS_LINK 2>/dev/null
 if [ $? -eq 0 ]; then
 	echo "linking with non-existent netdevsim should fail"
@@ -97,6 +98,14 @@ if [ $? -eq 0 ]; then
 	exit 1
 fi
 
+echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:a" > $NSIM_DEV_SYS_LINK 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "linking with invalid ifidx should fail"
+	cleanup_ns
+	exit 1
+fi
+
+# link two netdevsim interfaces
 echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:$NSIM_DEV_2_IFIDX" > $NSIM_DEV_SYS_LINK
 if [ $? -ne 0 ]; then
 	echo "linking netdevsim1 with netdevsim2 should succeed"
@@ -104,11 +113,10 @@ if [ $? -ne 0 ]; then
 	exit 1
 fi
 
-# argument error checking
-
-echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:a" > $NSIM_DEV_SYS_LINK 2>/dev/null
+# semantic error checking
+echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:$NSIM_DEV_2_IFIDX" > $NSIM_DEV_SYS_LINK 2>/dev/null
 if [ $? -eq 0 ]; then
-	echo "invalid arg should fail"
+	echo "linking already-connected netdevsim should fail"
 	cleanup_ns
 	exit 1
 fi
-- 
2.51.2

[PATCH net 3/5] selftests: netdevsim: add carrier state consistency test

[yk]

From: Yohei Kojima <yk@y-koj.net>

This commit adds a test case for netdevsim carrier state consistency.
Specifically, the added test verifies the carrier state during the
following operations:

1. Unlink two netdevsims
2. ifdown one netdevsim, then ifup again
3. Link the netdevsims again
4. ifdown one netdevsim, then ifup again

These steps verifies that the carrier is UP iff two netdevsims are
linked and ifuped.

Signed-off-by: Yohei Kojima <yk@y-koj.net>
---
 .../selftests/drivers/net/netdevsim/peer.sh   | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/netdevsim/peer.sh b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
index 338c844fe632..d15218e4bf5c 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/peer.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
@@ -52,6 +52,43 @@ cleanup_ns()
 	ip netns del nssv
 }
 
+is_carrier_up()
+{
+	local netns="$1"
+	local nsim_dev="$2"
+
+	# 0: DOWN
+	# 1: UP
+	local is_up=$(ip netns exec "$netns"	\
+		cat /sys/class/net/"$nsim_dev"/carrier 2>/dev/null)
+
+	test "$is_up" -eq 1
+}
+
+assert_carrier_up()
+{
+	local netns="$1"
+	local nsim_dev="$2"
+
+	if ! is_carrier_up "$netns" "$nsim_dev"; then
+		echo "$nsim_dev's carrier should be UP, but it isn't"
+		cleanup_ns
+		exit 1
+	fi
+}
+
+assert_carrier_down()
+{
+	local netns="$1"
+	local nsim_dev="$2"
+
+	if is_carrier_up "$netns" "$nsim_dev"; then
+		echo "$nsim_dev's carrier should be DOWN, but it isn't"
+		cleanup_ns
+		exit 1
+	fi
+}
+
 ###
 ### Code start
 ###
@@ -121,6 +158,32 @@ if [ $? -eq 0 ]; then
 	exit 1
 fi
 
+# netdevsim carrier state consistency checking
+assert_carrier_up nssv "$NSIM_DEV_1_NAME"
+assert_carrier_up nscl "$NSIM_DEV_2_NAME"
+
+echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX" > $NSIM_DEV_SYS_UNLINK
+
+assert_carrier_down nssv "$NSIM_DEV_1_NAME"
+assert_carrier_down nscl "$NSIM_DEV_2_NAME"
+
+ip netns exec nssv ip link set dev "$NSIM_DEV_1_NAME" down
+ip netns exec nssv ip link set dev "$NSIM_DEV_1_NAME" up
+
+assert_carrier_down nssv "$NSIM_DEV_1_NAME"
+assert_carrier_down nscl "$NSIM_DEV_2_NAME"
+
+echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:$NSIM_DEV_2_IFIDX" > $NSIM_DEV_SYS_LINK
+
+assert_carrier_up nssv "$NSIM_DEV_1_NAME"
+assert_carrier_up nscl "$NSIM_DEV_2_NAME"
+
+ip netns exec nssv ip link set dev "$NSIM_DEV_1_NAME" down
+ip netns exec nssv ip link set dev "$NSIM_DEV_1_NAME" up
+
+assert_carrier_up nssv "$NSIM_DEV_1_NAME"
+assert_carrier_up nscl "$NSIM_DEV_2_NAME"
+
 # send/recv packets
 
 tmp_file=$(mktemp)
-- 
2.51.2

[PATCH net 4/5] selftests: net: improve error handling in TFO test

[yk]

From: Yohei Kojima <yk@y-koj.net>

This commit improves the error handling in TCP Fast Open (TFO) test by
(1) adding the sendto() return value check and (2) changing read() error
handling code to exit with 1.

Signed-off-by: Yohei Kojima <yk@y-koj.net>
---
 tools/testing/selftests/net/tfo.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/tfo.c b/tools/testing/selftests/net/tfo.c
index 8d82140f0f76..4572eb9b8968 100644
--- a/tools/testing/selftests/net/tfo.c
+++ b/tools/testing/selftests/net/tfo.c
@@ -82,7 +82,8 @@ static void run_server(void)
 		error(1, errno, "getsockopt(SO_INCOMING_NAPI_ID)");
 
 	if (read(connfd, buf, 64) < 0)
-		perror("read()");
+		error(1, errno, "read()");
+
 	fprintf(outfile, "%d\n", opt);
 
 	fclose(outfile);
@@ -92,14 +93,17 @@ static void run_server(void)
 
 static void run_client(void)
 {
-	int fd;
+	int fd, ret;
 	char *msg = "Hello, world!";
 
 	fd = socket(AF_INET6, SOCK_STREAM, 0);
 	if (fd == -1)
 		error(1, errno, "socket()");
 
-	sendto(fd, msg, strlen(msg), MSG_FASTOPEN, (struct sockaddr *)&cfg_addr, sizeof(cfg_addr));
+	ret = sendto(fd, msg, strlen(msg), MSG_FASTOPEN,
+		     (struct sockaddr *)&cfg_addr, sizeof(cfg_addr));
+	if (ret < 0)
+		error(1, errno, "sendto()");
 
 	close(fd);
 }
-- 
2.51.2

[PATCH net 5/5] selftests: net: report SKIP if TFO test processes timed out

[yk]

From: Yohei Kojima <yk@y-koj.net>

This patch improves the TCP Fast Open (TFO) test to report the timeout
events and client/server error events by introducing better process
management.

Previously, TFO test didn't provide any information about the test
client/server processes' exit status, and just reported "ok". This
behavior is sometimes misleading in case TFO is unsupported by the
kernel, or there was a bug in the backing network devices (netdevsim).

Signed-off-by: Yohei Kojima <yk@y-koj.net>
---
 tools/testing/selftests/net/tfo_passive.sh | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/tfo_passive.sh b/tools/testing/selftests/net/tfo_passive.sh
index a4550511830a..1e89f1006c42 100755
--- a/tools/testing/selftests/net/tfo_passive.sh
+++ b/tools/testing/selftests/net/tfo_passive.sh
@@ -76,7 +76,7 @@ echo "$NSIM_SV_FD:$NSIM_SV_IFIDX $NSIM_CL_FD:$NSIM_CL_IFIDX" > \
 if [ $? -ne 0 ]; then
 	echo "linking netdevsim1 with netdevsim2 should succeed"
 	cleanup_ns
-	exit 1
+	exit "$ksft_fail"
 fi
 
 out_file=$(mktemp)
@@ -85,12 +85,15 @@ timeout -k 1s 30s ip netns exec nssv ./tfo        \
 				-s                \
 				-p ${SERVER_PORT} \
 				-o ${out_file}&
+server_pid="$!"
 
 wait_local_port_listen nssv ${SERVER_PORT} tcp
 
 ip netns exec nscl ./tfo -c -h ${SERVER_IP} -p ${SERVER_PORT}
+client_exit_status="$?"
 
-wait
+wait "$server_pid"
+server_exit_status="$?"
 
 res=$(cat $out_file)
 rm $out_file
@@ -101,6 +104,14 @@ if [ "$res" = "0" ]; then
 	exit 1
 fi
 
+if [ "$client_exit_status" -ne 0 ] || [ "$server_exit_status" -ne 0 ]; then
+	# Note: timeout(1) exits with 124 if it timed out
+	echo "client exited with ${client_exit_status}"
+	echo "server exited with ${server_exit_status}"
+	cleanup_ns
+	exit "$ksft_skip"
+fi
+
 echo "$NSIM_SV_FD:$NSIM_SV_IFIDX" > $NSIM_DEV_SYS_UNLINK
 
 echo $NSIM_CL_ID > $NSIM_DEV_SYS_DEL
-- 
2.51.2