From: Willem de Bruijn willemb@google.com
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
1/2: add kselftest infra for TEST_PROGS that need an interpreter
2/2: add the specific packetdrill tests
Both can go through net-next, I imagine. But let me know if the core infra should go through linux-kselftest.
Willem de Bruijn (2): selftests: support interpreted scripts with ksft_runner.sh selftests/net: integrate packetdrill with ksft
tools/testing/selftests/Makefile | 5 +- tools/testing/selftests/kselftest/runner.sh | 7 ++- .../selftests/net/packetdrill/Makefile | 9 +++ .../testing/selftests/net/packetdrill/config | 5 ++ .../selftests/net/packetdrill/defaults.sh | 63 +++++++++++++++++++ .../selftests/net/packetdrill/ksft_runner.sh | 41 ++++++++++++ .../net/packetdrill/tcp_inq_client.pkt | 51 +++++++++++++++ .../net/packetdrill/tcp_inq_server.pkt | 51 +++++++++++++++ .../tcp_md5_md5-only-on-client-ack.pkt | 28 +++++++++ 9 files changed, 256 insertions(+), 4 deletions(-) create mode 100644 tools/testing/selftests/net/packetdrill/Makefile create mode 100644 tools/testing/selftests/net/packetdrill/config create mode 100755 tools/testing/selftests/net/packetdrill/defaults.sh create mode 100755 tools/testing/selftests/net/packetdrill/ksft_runner.sh create mode 100644 tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt create mode 100644 tools/testing/selftests/net/packetdrill/tcp_inq_server.pkt create mode 100644 tools/testing/selftests/net/packetdrill/tcp_md5_md5-only-on-client-ack.pkt
From: Willem de Bruijn willemb@google.com
Support testcases that are themselves not executable, but need an interpreter to run them.
If a test file is not executable, but an executable file ksft_runner.sh exists in the TARGET dir, kselftest will run
./ksft_runner.sh ./$BASENAME_TEST
Packetdrill may add hundreds of packetdrill scripts for testing. These scripts must be passed to the packetdrill process.
Have kselftest run each test directly, as it already solves common runner requirements like parallel execution and isolation (netns). A previous RFC added a wrapper in between, which would have to reimplement such functionality.
Link: https://lore.kernel.org/netdev/66d4d97a4cac_3df182941a@willemb.c.googlers.co... Signed-off-by: Willem de Bruijn willemb@google.com --- tools/testing/selftests/kselftest/runner.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh index 74954f6a8f94b..2c3c58e65a419 100644 --- a/tools/testing/selftests/kselftest/runner.sh +++ b/tools/testing/selftests/kselftest/runner.sh @@ -111,8 +111,11 @@ run_one() stdbuf="/usr/bin/stdbuf --output=L " fi eval kselftest_cmd_args="$${kselftest_cmd_args_ref:-}" - cmd="$stdbuf ./$BASENAME_TEST $kselftest_cmd_args" - if [ ! -x "$TEST" ]; then + if [ -x "$TEST" ]; then + cmd="$stdbuf ./$BASENAME_TEST $kselftest_cmd_args" + elif [ -x "./ksft_runner.sh" ]; then + cmd="$stdbuf ./ksft_runner.sh ./$BASENAME_TEST" + else echo "# Warning: file $TEST is not executable"
if [ $(head -n 1 "$TEST" | cut -c -2) = "#!" ]
From: Willem de Bruijn willemb@google.com
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
Florian recently added support for packetdrill tests in nf_conntrack, in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based conntrack tests").
This patch takes a slightly different approach. It relies on ksft_runner.sh to run every *.pkt file in the directory.
Any future imports of packetdrill tests should require no additional coding. Just add the *.pkt files.
Initially import only two features/directories from github. One with a single script, and one with two. This was the only reason to pick tcp/inq and tcp/md5.
The path replaces the directory hierarchy in github with a flat space of files: $(subst /,_,$(wildcard tcp/**/*.pkt)). This is the most straightforward option to integrate with kselftests. The Linked thread reviewed two ways to maintain the hierarchy: TEST_PROGS_RECURSE and PRESERVE_TEST_DIRS. But both introduce significant changes to kselftest infra and with that risk to existing tests.
Implementation notes: - restore alphabetical order when adding the new directory to tools/testing/selftests/Makefile - imported *.pkt files and support verbatim from the github project, except for - update `source ./defaults.sh` path (to adjust for flat dir) - add SPDX headers - remove one author statement - Acknowledgment: drop an e (checkpatch)
Tested: make -C tools/testing/selftests \ TARGETS=net/packetdrill \ run_tests
make -C tools/testing/selftests \ TARGETS=net/packetdrill \ install INSTALL_PATH=$KSFT_INSTALL_PATH
# in virtme-ng ./run_kselftest.sh -c net/packetdrill ./run_kselftest.sh -t net/packetdrill:tcp_inq_client.pkt
Link: https://lore.kernel.org/netdev/20240827193417.2792223-1-willemdebruijn.kerne... Signed-off-by: Willem de Bruijn willemb@google.com
---
Changes: - RFC -> v1 - replace custom runner with ksft_runner.sh (previous patch in series) and ktap_helpers.sh - flatten the github tcp/**/*.pkt directory structure - add config for MD5 dependency - drop unused set_sysctls.py - v1 -> v2 - add missing CONFIGs - (minor) drop obsolete KSELFTEST_PKT_INTERP ref in commit message --- tools/testing/selftests/Makefile | 5 +- .../selftests/net/packetdrill/Makefile | 9 +++ .../testing/selftests/net/packetdrill/config | 5 ++ .../selftests/net/packetdrill/defaults.sh | 63 +++++++++++++++++++ .../selftests/net/packetdrill/ksft_runner.sh | 41 ++++++++++++ .../net/packetdrill/tcp_inq_client.pkt | 51 +++++++++++++++ .../net/packetdrill/tcp_inq_server.pkt | 51 +++++++++++++++ .../tcp_md5_md5-only-on-client-ack.pkt | 28 +++++++++ 8 files changed, 251 insertions(+), 2 deletions(-) create mode 100644 tools/testing/selftests/net/packetdrill/Makefile create mode 100644 tools/testing/selftests/net/packetdrill/config create mode 100755 tools/testing/selftests/net/packetdrill/defaults.sh create mode 100755 tools/testing/selftests/net/packetdrill/ksft_runner.sh create mode 100644 tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt create mode 100644 tools/testing/selftests/net/packetdrill/tcp_inq_server.pkt create mode 100644 tools/testing/selftests/net/packetdrill/tcp_md5_md5-only-on-client-ack.pkt
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index a5f1c0c27dff9..3b7df54773170 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -65,10 +65,11 @@ TARGETS += net/af_unix TARGETS += net/forwarding TARGETS += net/hsr TARGETS += net/mptcp -TARGETS += net/openvswitch -TARGETS += net/tcp_ao TARGETS += net/netfilter +TARGETS += net/openvswitch +TARGETS += net/packetdrill TARGETS += net/rds +TARGETS += net/tcp_ao TARGETS += nsfs TARGETS += perf_events TARGETS += pidfd diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile new file mode 100644 index 0000000000000..870f7258dc8d7 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/Makefile @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0 + +TEST_INCLUDES := ksft_runner.sh \ + defaults.sh \ + ../../kselftest/ktap_helpers.sh + +TEST_PROGS := $(wildcard *.pkt) + +include ../../lib.mk diff --git a/tools/testing/selftests/net/packetdrill/config b/tools/testing/selftests/net/packetdrill/config new file mode 100644 index 0000000000000..0d402830f18d8 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/config @@ -0,0 +1,5 @@ +CONFIG_IPV6=y +CONFIG_NET_SCH_FIFO=y +CONFIG_PROC_SYSCTL=y +CONFIG_TCP_MD5SIG=y +CONFIG_TUN=y diff --git a/tools/testing/selftests/net/packetdrill/defaults.sh b/tools/testing/selftests/net/packetdrill/defaults.sh new file mode 100755 index 0000000000000..1095a7b22f44d --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/defaults.sh @@ -0,0 +1,63 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Set standard production config values that relate to TCP behavior. + +# Flush old cached data (fastopen cookies). +ip tcp_metrics flush all > /dev/null 2>&1 + +# TCP min, default, and max receive and send buffer sizes. +sysctl -q net.ipv4.tcp_rmem="4096 540000 $((15*1024*1024))" +sysctl -q net.ipv4.tcp_wmem="4096 $((256*1024)) 4194304" + +# TCP timestamps. +sysctl -q net.ipv4.tcp_timestamps=1 + +# TCP SYN(ACK) retry thresholds +sysctl -q net.ipv4.tcp_syn_retries=5 +sysctl -q net.ipv4.tcp_synack_retries=5 + +# TCP Forward RTO-Recovery, RFC 5682. +sysctl -q net.ipv4.tcp_frto=2 + +# TCP Selective Acknowledgements (SACK) +sysctl -q net.ipv4.tcp_sack=1 + +# TCP Duplicate Selective Acknowledgements (DSACK) +sysctl -q net.ipv4.tcp_dsack=1 + +# TCP FACK (Forward Acknowldgement) +sysctl -q net.ipv4.tcp_fack=0 + +# TCP reordering degree ("dupthresh" threshold for entering Fast Recovery). +sysctl -q net.ipv4.tcp_reordering=3 + +# TCP congestion control. +sysctl -q net.ipv4.tcp_congestion_control=cubic + +# TCP slow start after idle. +sysctl -q net.ipv4.tcp_slow_start_after_idle=0 + +# TCP RACK and TLP. +sysctl -q net.ipv4.tcp_early_retrans=4 net.ipv4.tcp_recovery=1 + +# TCP method for deciding when to defer sending to accumulate big TSO packets. +sysctl -q net.ipv4.tcp_tso_win_divisor=3 + +# TCP Explicit Congestion Notification (ECN) +sysctl -q net.ipv4.tcp_ecn=0 + +sysctl -q net.ipv4.tcp_pacing_ss_ratio=200 +sysctl -q net.ipv4.tcp_pacing_ca_ratio=120 +sysctl -q net.ipv4.tcp_notsent_lowat=4294967295 > /dev/null 2>&1 + +sysctl -q net.ipv4.tcp_fastopen=0x70403 +sysctl -q net.ipv4.tcp_fastopen_key=a1a1a1a1-b2b2b2b2-c3c3c3c3-d4d4d4d4 + +sysctl -q net.ipv4.tcp_syncookies=1 + +# Override the default qdisc on the tun device. +# Many tests fail with timing errors if the default +# is FQ and that paces their flows. +tc qdisc add dev tun0 root pfifo + diff --git a/tools/testing/selftests/net/packetdrill/ksft_runner.sh b/tools/testing/selftests/net/packetdrill/ksft_runner.sh new file mode 100755 index 0000000000000..2f62caccbbbc5 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh @@ -0,0 +1,41 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +source "$(dirname $(realpath $0))/../../kselftest/ktap_helpers.sh" + +readonly ipv4_args=('--ip_version=ipv4 ' + '--local_ip=192.168.0.1 ' + '--gateway_ip=192.168.0.1 ' + '--netmask_ip=255.255.0.0 ' + '--remote_ip=192.0.2.1 ' + '-D CMSG_LEVEL_IP=SOL_IP ' + '-D CMSG_TYPE_RECVERR=IP_RECVERR ') + +readonly ipv6_args=('--ip_version=ipv6 ' + '--mtu=1520 ' + '--local_ip=fd3d:0a0b:17d6::1 ' + '--gateway_ip=fd3d:0a0b:17d6:8888::1 ' + '--remote_ip=fd3d:fa7b:d17d::1 ' + '-D CMSG_LEVEL_IP=SOL_IPV6 ' + '-D CMSG_TYPE_RECVERR=IPV6_RECVERR ') + +if [ $# -ne 1 ]; then + ktap_exit_fail_msg "usage: $0 <script>" + exit "$KSFT_FAIL" +fi +script="$1" + +if [ -z "$(which packetdrill)" ]; then + ktap_skip_all "packetdrill not found in PATH" + exit "$KSFT_SKIP" +fi + +ktap_print_header +ktap_set_plan 2 + +packetdrill ${ipv4_args[@]} $(basename $script) > /dev/null \ + && ktap_test_pass "ipv4" || ktap_test_fail "ipv4" +packetdrill ${ipv6_args[@]} $(basename $script) > /dev/null \ + && ktap_test_pass "ipv6" || ktap_test_fail "ipv6" + +ktap_finished diff --git a/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt new file mode 100644 index 0000000000000..df49c67645ac8 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 +// Test TCP_INQ and TCP_CM_INQ on the client side. +`./defaults.sh +` + +// Create a socket and set it to non-blocking. + 0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3 + +0 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR) + +0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0 + +// Connect to the server and enable TCP_INQ. + +0 connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress) + +0 setsockopt(3, SOL_TCP, TCP_INQ, [1], 4) = 0 + + +0 > S 0:0(0) <mss 1460,sackOK,TS val 100 ecr 0,nop,wscale 8> + +.01 < S. 0:0(0) ack 1 win 5792 <mss 1460,sackOK,TS val 700 ecr 100,nop,wscale 7> + +0 > . 1:1(0) ack 1 <nop,nop,TS val 200 ecr 700> + +// Now we have 10K of data ready on the socket. + +0 < . 1:10001(10000) ack 1 win 514 + +0 > . 1:1(0) ack 10001 <nop,nop,TS val 200 ecr 700> + +// We read 1K and we should have 9K ready to read. + +0 recvmsg(3, {msg_name(...)=..., + msg_iov(1)=[{..., 1000}], + msg_flags=0, + msg_control=[{cmsg_level=SOL_TCP, + cmsg_type=TCP_CM_INQ, + cmsg_data=9000}]}, 0) = 1000 +// We read 9K and we should have no further data ready to read. + +0 recvmsg(3, {msg_name(...)=..., + msg_iov(1)=[{..., 9000}], + msg_flags=0, + msg_control=[{cmsg_level=SOL_TCP, + cmsg_type=TCP_CM_INQ, + cmsg_data=0}]}, 0) = 9000 + +// Server sends more data and closes the connections. + +0 < F. 10001:20001(10000) ack 1 win 514 + +0 > . 1:1(0) ack 20002 <nop,nop,TS val 200 ecr 700> + +// We read 10K and we should have one "fake" byte because the connection is +// closed. + +0 recvmsg(3, {msg_name(...)=..., + msg_iov(1)=[{..., 10000}], + msg_flags=0, + msg_control=[{cmsg_level=SOL_TCP, + cmsg_type=TCP_CM_INQ, + cmsg_data=1}]}, 0) = 10000 +// Now, receive EOF. + +0 read(3, ..., 2000) = 0 diff --git a/tools/testing/selftests/net/packetdrill/tcp_inq_server.pkt b/tools/testing/selftests/net/packetdrill/tcp_inq_server.pkt new file mode 100644 index 0000000000000..04a5e2590c62c --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/tcp_inq_server.pkt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 +// Test TCP_INQ and TCP_CM_INQ on the server side. +`./defaults.sh +` + +// Initialize connection + 0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3 + +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 + +0 bind(3, ..., ...) = 0 + +0 listen(3, 1) = 0 + + +0 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 10> + +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8> + +.01 < . 1:1(0) ack 1 win 514 + +// Accept the connection and enable TCP_INQ. + +0 accept(3, ..., ...) = 4 + +0 setsockopt(4, SOL_TCP, TCP_INQ, [1], 4) = 0 + +// Now we have 10K of data ready on the socket. + +0 < . 1:10001(10000) ack 1 win 514 + +0 > . 1:1(0) ack 10001 + +// We read 2K and we should have 8K ready to read. + +0 recvmsg(4, {msg_name(...)=..., + msg_iov(1)=[{..., 2000}], + msg_flags=0, + msg_control=[{cmsg_level=SOL_TCP, + cmsg_type=TCP_CM_INQ, + cmsg_data=8000}]}, 0) = 2000 +// We read 8K and we should have no further data ready to read. + +0 recvmsg(4, {msg_name(...)=..., + msg_iov(1)=[{..., 8000}], + msg_flags=0, + msg_control=[{cmsg_level=SOL_TCP, + cmsg_type=TCP_CM_INQ, + cmsg_data=0}]}, 0) = 8000 +// Client sends more data and closes the connections. + +0 < F. 10001:20001(10000) ack 1 win 514 + +0 > . 1:1(0) ack 20002 + +// We read 10K and we should have one "fake" byte because the connection is +// closed. + +0 recvmsg(4, {msg_name(...)=..., + msg_iov(1)=[{..., 10000}], + msg_flags=0, + msg_control=[{cmsg_level=SOL_TCP, + cmsg_type=TCP_CM_INQ, + cmsg_data=1}]}, 0) = 10000 +// Now, receive error. + +0 read(3, ..., 2000) = -1 ENOTCONN (Transport endpoint is not connected) diff --git a/tools/testing/selftests/net/packetdrill/tcp_md5_md5-only-on-client-ack.pkt b/tools/testing/selftests/net/packetdrill/tcp_md5_md5-only-on-client-ack.pkt new file mode 100644 index 0000000000000..25dfef95d3f84 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/tcp_md5_md5-only-on-client-ack.pkt @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: GPL-2.0 +// Test what happens when client does not provide MD5 on SYN, +// but then does on the ACK that completes the three-way handshake. + +`./defaults.sh` + +// Establish a connection. + 0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3 + +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 + +0 bind(3, ..., ...) = 0 + +0 listen(3, 1) = 0 + + +0 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 10> + +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8> +// Ooh, weird: client provides MD5 option on the ACK: + +.01 < . 1:1(0) ack 1 win 514 <md5 000102030405060708090a0b0c0d0e0f,nop,nop> + +.01 < . 1:1(0) ack 1 win 514 <md5 000102030405060708090a0b0c0d0e0f,nop,nop> + +// The TCP listener refcount should be 2, but on buggy kernels it can be 0: + +0 `grep " 0A " /proc/net/tcp /proc/net/tcp6 | grep ":1F90"` + +// Now here comes the legit ACK: + +.01 < . 1:1(0) ack 1 win 514 + +// Make sure the connection is OK: + +0 accept(3, ..., ...) = 4 + + +.01 write(4, ..., 1000) = 1000
Hi Willem,
On 06/09/2024 01:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
Florian recently added support for packetdrill tests in nf_conntrack, in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based conntrack tests").
This patch takes a slightly different approach. It relies on ksft_runner.sh to run every *.pkt file in the directory.
Thank you for adding this support! I'm looking forward to seeing more packetdrill tests being validated by the CI, and I hope that will encourage people to add more tests, and increase the code coverage!
I have some questions about how the packetdrill should be executed: should they be isolated in dedicated netns?
There are some other comments, but feel free to ignore them if they are not relevant, or if they can be fixed later.
Tested: make -C tools/testing/selftests \ TARGETS=net/packetdrill \ run_tests
Please note that this will not run the packetdrill tests in a dedicated netns. I don't think that's a good idea. Especially when sysctl knobs are being changed during the tests, and more.
make -C tools/testing/selftests \ TARGETS=net/packetdrill \ install INSTALL_PATH=$KSFT_INSTALL_PATH
# in virtme-ng ./run_kselftest.sh -c net/packetdrill ./run_kselftest.sh -t net/packetdrill:tcp_inq_client.pkt
I see that ./run_kselftest.sh can be executed with the "-n | --netns" option to run each test in a dedicated net namespace, but it doesn't seem to work:
# ./run_kselftest.sh -n -c net/packetdrill [ 411.087208] kselftest: Running tests in net/packetdrill TAP version 13 1..3 Cannot open network namespace "tcp_inq_client.pkt-TaQ8iN": No such file or directory setting the network namespace "tcp_inq_server.pkt-sW8YCz" failed: Invalid argument Cannot open network namespace "tcp_md5_md5-only-on-client-ack.pkt-YzJal6": No such file or directory
But maybe it would be better to create the netns in ksft_runner.sh? Please see below.
(...)
diff --git a/tools/testing/selftests/net/packetdrill/defaults.sh b/tools/testing/selftests/net/packetdrill/defaults.sh new file mode 100755 index 0000000000000..1095a7b22f44d --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/defaults.sh @@ -0,0 +1,63 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Set standard production config values that relate to TCP behavior.
+# Flush old cached data (fastopen cookies). +ip tcp_metrics flush all > /dev/null 2>&1
(Why ignoring errors if any?)
+# TCP min, default, and max receive and send buffer sizes. +sysctl -q net.ipv4.tcp_rmem="4096 540000 $((15*1024*1024))" +sysctl -q net.ipv4.tcp_wmem="4096 $((256*1024)) 4194304"
+# TCP timestamps. +sysctl -q net.ipv4.tcp_timestamps=1
+# TCP SYN(ACK) retry thresholds +sysctl -q net.ipv4.tcp_syn_retries=5 +sysctl -q net.ipv4.tcp_synack_retries=5
+# TCP Forward RTO-Recovery, RFC 5682. +sysctl -q net.ipv4.tcp_frto=2
+# TCP Selective Acknowledgements (SACK) +sysctl -q net.ipv4.tcp_sack=1
+# TCP Duplicate Selective Acknowledgements (DSACK) +sysctl -q net.ipv4.tcp_dsack=1
+# TCP FACK (Forward Acknowldgement) +sysctl -q net.ipv4.tcp_fack=0
+# TCP reordering degree ("dupthresh" threshold for entering Fast Recovery). +sysctl -q net.ipv4.tcp_reordering=3
+# TCP congestion control. +sysctl -q net.ipv4.tcp_congestion_control=cubic
(maybe safer to add CONFIG_TCP_CONG_CUBIC=y?)
+# TCP slow start after idle. +sysctl -q net.ipv4.tcp_slow_start_after_idle=0
+# TCP RACK and TLP. +sysctl -q net.ipv4.tcp_early_retrans=4 net.ipv4.tcp_recovery=1
+# TCP method for deciding when to defer sending to accumulate big TSO packets. +sysctl -q net.ipv4.tcp_tso_win_divisor=3
+# TCP Explicit Congestion Notification (ECN) +sysctl -q net.ipv4.tcp_ecn=0
+sysctl -q net.ipv4.tcp_pacing_ss_ratio=200 +sysctl -q net.ipv4.tcp_pacing_ca_ratio=120 +sysctl -q net.ipv4.tcp_notsent_lowat=4294967295 > /dev/null 2>&1
+sysctl -q net.ipv4.tcp_fastopen=0x70403 +sysctl -q net.ipv4.tcp_fastopen_key=a1a1a1a1-b2b2b2b2-c3c3c3c3-d4d4d4d4
+sysctl -q net.ipv4.tcp_syncookies=1
(maybe safer to add CONFIG_SYN_COOKIES=y?)
+# Override the default qdisc on the tun device. +# Many tests fail with timing errors if the default +# is FQ and that paces their flows. +tc qdisc add dev tun0 root pfifo
(when applying your patches with 'b4 shazam', I got the following error, I guess it was due to this blank line at the end)
Applying: selftests: support interpreted scripts with ksft_runner.sh Applying: selftests/net: integrate packetdrill with ksft .git/rebase-apply/patch:142: new blank line at EOF.
diff --git a/tools/testing/selftests/net/packetdrill/ksft_runner.sh b/tools/testing/selftests/net/packetdrill/ksft_runner.sh new file mode 100755 index 0000000000000..2f62caccbbbc5 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh @@ -0,0 +1,41 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0
+source "$(dirname $(realpath $0))/../../kselftest/ktap_helpers.sh"
+readonly ipv4_args=('--ip_version=ipv4 '
'--local_ip=192.168.0.1 ''--gateway_ip=192.168.0.1 ''--netmask_ip=255.255.0.0 ''--remote_ip=192.0.2.1 ''-D CMSG_LEVEL_IP=SOL_IP ''-D CMSG_TYPE_RECVERR=IP_RECVERR ')+readonly ipv6_args=('--ip_version=ipv6 '
'--mtu=1520 ''--local_ip=fd3d:0a0b:17d6::1 ''--gateway_ip=fd3d:0a0b:17d6:8888::1 ''--remote_ip=fd3d:fa7b:d17d::1 ''-D CMSG_LEVEL_IP=SOL_IPV6 ''-D CMSG_TYPE_RECVERR=IPV6_RECVERR ')
(nit: that's a strange Bash array with spaces in the strings :) You can simply remove the quotes, then below, you can use the variable with double quotes: "${ipv4_args[@]}" and shellcheck will stop reporting an error for that)
+if [ $# -ne 1 ]; then
- ktap_exit_fail_msg "usage: $0 <script>"
- exit "$KSFT_FAIL"
+fi +script="$1"
+if [ -z "$(which packetdrill)" ]; then
- ktap_skip_all "packetdrill not found in PATH"
- exit "$KSFT_SKIP"
+fi
+ktap_print_header +ktap_set_plan 2
+packetdrill ${ipv4_args[@]} $(basename $script) > /dev/null \
(Why muting stdout? I see that the TCP MD5 test output lines from /proc/net/tcp*, is it why? Is this info useful? Or should it be removed from the test?)
- && ktap_test_pass "ipv4" || ktap_test_fail "ipv4"
+packetdrill ${ipv6_args[@]} $(basename $script) > /dev/null \
- && ktap_test_pass "ipv6" || ktap_test_fail "ipv6"
Even if "run_kselftest.sh" creates dedicated netns before running this script (RUN_IN_NETNS=1), it looks like the tests in v4 and in v6 will share the same netns. Is it OK? It means that if a packetdrill test sets something that is not reset by 'defaults.sh', it might break the following v6 test.
Why not having "ksft_runner.sh" creating the netns? It should be easy to do so, using helpers from the "../lib.sh" file:
ns_v4= ns_v6=
trap cleanup_all_ns EXIT if ! setup_ns ns_v4 ns_v6; then (...) # fail + exit fi
ip netns exec "${ns_v4}" packetdrill "${ipv4_args[@]}" \ "$(basename "${script}")" (...)
(Note that if these tests are isolated in dedicated netns, and if later we want to accelerate their execution, it should be easy to run these two tests in parallel, something like the following)
ip netns exec "${ns_v4}" (...) & pid_v4=$! ip netns exec "${ns_v6}" (...) & pid_v6=$!
wait ${pid_v4} && tap_test_pass "ipv4" || ktap_test_fail "ipv4" wait ${pid_v6} && tap_test_pass "ipv6" || ktap_test_fail "ipv6"
+ktap_finished diff --git a/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt new file mode 100644 index 0000000000000..df49c67645ac8 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 +// Test TCP_INQ and TCP_CM_INQ on the client side. +`./defaults.sh +`
(I guess you prefer not to modify these tests, and keep them self-contained, but just in case it is easier for you, this line could be removed, and have ksft_runner.sh sourcing this file before executing the packetdrill test.)
Cheers, Matt
Matthieu Baerts wrote:
Hi Willem,
On 06/09/2024 01:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
Florian recently added support for packetdrill tests in nf_conntrack, in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based conntrack tests").
This patch takes a slightly different approach. It relies on ksft_runner.sh to run every *.pkt file in the directory.
Thank you for adding this support! I'm looking forward to seeing more packetdrill tests being validated by the CI, and I hope that will encourage people to add more tests, and increase the code coverage!
Thanks for taking a look and your feedback.
I have some questions about how the packetdrill should be executed: should they be isolated in dedicated netns?
Yes. The runner decides that, by passing -n
There are some other comments, but feel free to ignore them if they are not relevant, or if they can be fixed later.
Tested: make -C tools/testing/selftests \ TARGETS=net/packetdrill \ run_tests
Please note that this will not run the packetdrill tests in a dedicated netns. I don't think that's a good idea. Especially when sysctl knobs are being changed during the tests, and more.
make -C tools/testing/selftests \ TARGETS=net/packetdrill \ install INSTALL_PATH=$KSFT_INSTALL_PATH
# in virtme-ng ./run_kselftest.sh -c net/packetdrill ./run_kselftest.sh -t net/packetdrill:tcp_inq_client.pkt
I see that ./run_kselftest.sh can be executed with the "-n | --netns" option to run each test in a dedicated net namespace, but it doesn't seem to work:
# ./run_kselftest.sh -n -c net/packetdrill [ 411.087208] kselftest: Running tests in net/packetdrill TAP version 13 1..3 Cannot open network namespace "tcp_inq_client.pkt-TaQ8iN": No such file or directory setting the network namespace "tcp_inq_server.pkt-sW8YCz" failed: Invalid argument Cannot open network namespace "tcp_md5_md5-only-on-client-ack.pkt-YzJal6": No such file or directory
Ah, I guess this requires adding CONFIG_NET_NS=y to tools/testing/selftests/net/packetdrill/config
But maybe it would be better to create the netns in ksft_runner.sh? Please see below.
No, we opted for this design exactly to use existing kselftest infra, rather than reimplementing that in our wrapper, as I did in the RFC.
(...)
diff --git a/tools/testing/selftests/net/packetdrill/defaults.sh b/tools/testing/selftests/net/packetdrill/defaults.sh new file mode 100755 index 0000000000000..1095a7b22f44d --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/defaults.sh @@ -0,0 +1,63 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Set standard production config values that relate to TCP behavior.
+# Flush old cached data (fastopen cookies). +ip tcp_metrics flush all > /dev/null 2>&1
(Why ignoring errors if any?)
I don't know. Just importing this verbatim from github. As that is debugged over a long time and proven to work.
+# TCP min, default, and max receive and send buffer sizes. +sysctl -q net.ipv4.tcp_rmem="4096 540000 $((15*1024*1024))" +sysctl -q net.ipv4.tcp_wmem="4096 $((256*1024)) 4194304"
+# TCP timestamps. +sysctl -q net.ipv4.tcp_timestamps=1
+# TCP SYN(ACK) retry thresholds +sysctl -q net.ipv4.tcp_syn_retries=5 +sysctl -q net.ipv4.tcp_synack_retries=5
+# TCP Forward RTO-Recovery, RFC 5682. +sysctl -q net.ipv4.tcp_frto=2
+# TCP Selective Acknowledgements (SACK) +sysctl -q net.ipv4.tcp_sack=1
+# TCP Duplicate Selective Acknowledgements (DSACK) +sysctl -q net.ipv4.tcp_dsack=1
+# TCP FACK (Forward Acknowldgement) +sysctl -q net.ipv4.tcp_fack=0
+# TCP reordering degree ("dupthresh" threshold for entering Fast Recovery). +sysctl -q net.ipv4.tcp_reordering=3
+# TCP congestion control. +sysctl -q net.ipv4.tcp_congestion_control=cubic
(maybe safer to add CONFIG_TCP_CONG_CUBIC=y?)
Ack, thanks. Also below.
+# TCP slow start after idle. +sysctl -q net.ipv4.tcp_slow_start_after_idle=0
+# TCP RACK and TLP. +sysctl -q net.ipv4.tcp_early_retrans=4 net.ipv4.tcp_recovery=1
+# TCP method for deciding when to defer sending to accumulate big TSO packets. +sysctl -q net.ipv4.tcp_tso_win_divisor=3
+# TCP Explicit Congestion Notification (ECN) +sysctl -q net.ipv4.tcp_ecn=0
+sysctl -q net.ipv4.tcp_pacing_ss_ratio=200 +sysctl -q net.ipv4.tcp_pacing_ca_ratio=120 +sysctl -q net.ipv4.tcp_notsent_lowat=4294967295 > /dev/null 2>&1
+sysctl -q net.ipv4.tcp_fastopen=0x70403 +sysctl -q net.ipv4.tcp_fastopen_key=a1a1a1a1-b2b2b2b2-c3c3c3c3-d4d4d4d4
+sysctl -q net.ipv4.tcp_syncookies=1
(maybe safer to add CONFIG_SYN_COOKIES=y?)
+# Override the default qdisc on the tun device. +# Many tests fail with timing errors if the default +# is FQ and that paces their flows. +tc qdisc add dev tun0 root pfifo
(when applying your patches with 'b4 shazam', I got the following error, I guess it was due to this blank line at the end)
Applying: selftests: support interpreted scripts with ksft_runner.sh Applying: selftests/net: integrate packetdrill with ksft .git/rebase-apply/patch:142: new blank line at EOF.
diff --git a/tools/testing/selftests/net/packetdrill/ksft_runner.sh b/tools/testing/selftests/net/packetdrill/ksft_runner.sh new file mode 100755 index 0000000000000..2f62caccbbbc5 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh @@ -0,0 +1,41 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0
+source "$(dirname $(realpath $0))/../../kselftest/ktap_helpers.sh"
+readonly ipv4_args=('--ip_version=ipv4 '
'--local_ip=192.168.0.1 ''--gateway_ip=192.168.0.1 ''--netmask_ip=255.255.0.0 ''--remote_ip=192.0.2.1 ''-D CMSG_LEVEL_IP=SOL_IP ''-D CMSG_TYPE_RECVERR=IP_RECVERR ')+readonly ipv6_args=('--ip_version=ipv6 '
'--mtu=1520 ''--local_ip=fd3d:0a0b:17d6::1 ''--gateway_ip=fd3d:0a0b:17d6:8888::1 ''--remote_ip=fd3d:fa7b:d17d::1 ''-D CMSG_LEVEL_IP=SOL_IPV6 ''-D CMSG_TYPE_RECVERR=IPV6_RECVERR ')(nit: that's a strange Bash array with spaces in the strings :) You can simply remove the quotes, then below, you can use the variable with double quotes: "${ipv4_args[@]}" and shellcheck will stop reporting an error for that)
Thanks!
+if [ $# -ne 1 ]; then
- ktap_exit_fail_msg "usage: $0 <script>"
- exit "$KSFT_FAIL"
+fi +script="$1"
+if [ -z "$(which packetdrill)" ]; then
- ktap_skip_all "packetdrill not found in PATH"
- exit "$KSFT_SKIP"
+fi
+ktap_print_header +ktap_set_plan 2
+packetdrill ${ipv4_args[@]} $(basename $script) > /dev/null \
(Why muting stdout? I see that the TCP MD5 test output lines from /proc/net/tcp*, is it why? Is this info useful? Or should it be removed from the test?)
Indeed that's why. If a test fails in automated testing we can run manually to inspect such output.
- && ktap_test_pass "ipv4" || ktap_test_fail "ipv4"
+packetdrill ${ipv6_args[@]} $(basename $script) > /dev/null \
- && ktap_test_pass "ipv6" || ktap_test_fail "ipv6"
Even if "run_kselftest.sh" creates dedicated netns before running this script (RUN_IN_NETNS=1), it looks like the tests in v4 and in v6 will share the same netns. Is it OK? It means that if a packetdrill test sets something that is not reset by 'defaults.sh', it might break the following v6 test.
That should be fine. If a test cares about a sysctl, then it needs to set it at the start. In this case, they both will set exactly the same anyway.
Why not having "ksft_runner.sh" creating the netns? It should be easy to do so, using helpers from the "../lib.sh" file:
See above.
ns_v4= ns_v6=
trap cleanup_all_ns EXIT if ! setup_ns ns_v4 ns_v6; then (...) # fail + exit fi
ip netns exec "${ns_v4}" packetdrill "${ipv4_args[@]}" \ "$(basename "${script}")" (...)
(Note that if these tests are isolated in dedicated netns, and if later we want to accelerate their execution, it should be easy to run these two tests in parallel, something like the following)
ip netns exec "${ns_v4}" (...) & pid_v4=$! ip netns exec "${ns_v6}" (...) & pid_v6=$!
wait ${pid_v4} && tap_test_pass "ipv4" || ktap_test_fail "ipv4" wait ${pid_v6} && tap_test_pass "ipv6" || ktap_test_fail "ipv6"
+ktap_finished diff --git a/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt new file mode 100644 index 0000000000000..df49c67645ac8 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 +// Test TCP_INQ and TCP_CM_INQ on the client side. +`./defaults.sh +`
(I guess you prefer not to modify these tests, and keep them self-contained, but just in case it is easier for you, this line could be removed, and have ksft_runner.sh sourcing this file before executing the packetdrill test.)
Future packetdrill tests can have different shell preambles. Let's indeed leave it to the tests themselves.
Cheers, Matt -- Sponsored by the NGI0 Core fund.
On 06/09/2024 17:36, Willem de Bruijn wrote:
Matthieu Baerts wrote:
Hi Willem,
On 06/09/2024 01:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
Florian recently added support for packetdrill tests in nf_conntrack, in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based conntrack tests").
This patch takes a slightly different approach. It relies on ksft_runner.sh to run every *.pkt file in the directory.
Thank you for adding this support! I'm looking forward to seeing more packetdrill tests being validated by the CI, and I hope that will encourage people to add more tests, and increase the code coverage!
Thanks for taking a look and your feedback.
You are welcome!
I have some questions about how the packetdrill should be executed: should they be isolated in dedicated netns?
Yes. The runner decides that, by passing -n
But then it means that by default, the tests are not isolated. I think many (most?) selftests are running in a dedicated netns by default, no?
To be honest, that's the first time I hear about:
./run_kselftest.sh --netns
I don't know if it is common to use it, nor if we can enable this feature when using 'make run_tests'. And I don't know if many CI runs multiple selftests in parallel from the same VM.
There are some other comments, but feel free to ignore them if they are not relevant, or if they can be fixed later.
Tested: make -C tools/testing/selftests \ TARGETS=net/packetdrill \ run_tests
Please note that this will not run the packetdrill tests in a dedicated netns. I don't think that's a good idea. Especially when sysctl knobs are being changed during the tests, and more.
make -C tools/testing/selftests \ TARGETS=net/packetdrill \ install INSTALL_PATH=$KSFT_INSTALL_PATH
# in virtme-ng ./run_kselftest.sh -c net/packetdrill ./run_kselftest.sh -t net/packetdrill:tcp_inq_client.pkt
I see that ./run_kselftest.sh can be executed with the "-n | --netns" option to run each test in a dedicated net namespace, but it doesn't seem to work:
# ./run_kselftest.sh -n -c net/packetdrill [ 411.087208] kselftest: Running tests in net/packetdrill TAP version 13 1..3 Cannot open network namespace "tcp_inq_client.pkt-TaQ8iN": No such file or directory setting the network namespace "tcp_inq_server.pkt-sW8YCz" failed: Invalid argument Cannot open network namespace "tcp_md5_md5-only-on-client-ack.pkt-YzJal6": No such file or directory
Ah, I guess this requires adding CONFIG_NET_NS=y to tools/testing/selftests/net/packetdrill/config
Good point. But I have CONFIG_NET_NS=y on my side. I didn't investigate more, I was first wondering if other people tried this option.
But maybe it would be better to create the netns in ksft_runner.sh? Please see below.
No, we opted for this design exactly to use existing kselftest infra, rather than reimplementing that in our wrapper, as I did in the RFC.
OK, I understood from the discussions from the RFC that by using the kselftest infra, the tests would be automatically executed in dedicated netns, and it could also help running tests in parallel. That sounded great to me, but that's not the case by default from what I see.
(...)
diff --git a/tools/testing/selftests/net/packetdrill/defaults.sh b/tools/testing/selftests/net/packetdrill/defaults.sh new file mode 100755 index 0000000000000..1095a7b22f44d --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/defaults.sh @@ -0,0 +1,63 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Set standard production config values that relate to TCP behavior.
+# Flush old cached data (fastopen cookies). +ip tcp_metrics flush all > /dev/null 2>&1
(Why ignoring errors if any?)
I don't know. Just importing this verbatim from github. As that is debugged over a long time and proven to work.
All good, I'm fine like that.
(...)
diff --git a/tools/testing/selftests/net/packetdrill/ksft_runner.sh b/tools/testing/selftests/net/packetdrill/ksft_runner.sh new file mode 100755 index 0000000000000..2f62caccbbbc5 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh @@ -0,0 +1,41 @@
(...)
+packetdrill ${ipv4_args[@]} $(basename $script) > /dev/null \
(Why muting stdout? I see that the TCP MD5 test output lines from /proc/net/tcp*, is it why? Is this info useful? Or should it be removed from the test?)
Indeed that's why. If a test fails in automated testing we can run manually to inspect such output.
If we can reproduce the issue :)
Or maybe it is not an issue to store such output in the logs?
But if you tell me that this output has never been helpful in the past, it is fine for me to mute it, not to have to modify the tests.
- && ktap_test_pass "ipv4" || ktap_test_fail "ipv4"
+packetdrill ${ipv6_args[@]} $(basename $script) > /dev/null \
- && ktap_test_pass "ipv6" || ktap_test_fail "ipv6"
Even if "run_kselftest.sh" creates dedicated netns before running this script (RUN_IN_NETNS=1), it looks like the tests in v4 and in v6 will share the same netns. Is it OK? It means that if a packetdrill test sets something that is not reset by 'defaults.sh', it might break the following v6 test.
That should be fine. If a test cares about a sysctl, then it needs to set it at the start. In this case, they both will set exactly the same anyway.
Ack.
Why not having "ksft_runner.sh" creating the netns? It should be easy to do so, using helpers from the "../lib.sh" file:
See above.
Before the discussions from the RFC, I initially thought that the selftest itself had to deal with the netns. But then I understood there was a possibility to force the kselftest infra to execute the different tests in a dedicated netns. To be honest, it is not clear to me who should be in charge of creating these netns :)
Maybe that's fine like that then, but it feels strange to me not to have such isolation :)
In Florian's version, the packetdrill tests are executed in a dedicated netns using 'unshare -n'. That's an easier alternative than the one I suggested in my previous email with setup_ns.
(...)
diff --git a/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt new file mode 100644 index 0000000000000..df49c67645ac8 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 +// Test TCP_INQ and TCP_CM_INQ on the client side. +`./defaults.sh +`
(I guess you prefer not to modify these tests, and keep them self-contained, but just in case it is easier for you, this line could be removed, and have ksft_runner.sh sourcing this file before executing the packetdrill test.)
Future packetdrill tests can have different shell preambles. Let's indeed leave it to the tests themselves.
It makes sense.
Cheers, Matt
Matthieu Baerts wrote:
On 06/09/2024 17:36, Willem de Bruijn wrote:
Matthieu Baerts wrote:
Hi Willem,
On 06/09/2024 01:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
Florian recently added support for packetdrill tests in nf_conntrack, in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based conntrack tests").
This patch takes a slightly different approach. It relies on ksft_runner.sh to run every *.pkt file in the directory.
Thank you for adding this support! I'm looking forward to seeing more packetdrill tests being validated by the CI, and I hope that will encourage people to add more tests, and increase the code coverage!
Thanks for taking a look and your feedback.
You are welcome!
I have some questions about how the packetdrill should be executed: should they be isolated in dedicated netns?
Yes. The runner decides that, by passing -n
But then it means that by default, the tests are not isolated. I think many (most?) selftests are running in a dedicated netns by default, no?
To be honest, that's the first time I hear about:
./run_kselftest.sh --netns
I don't know if it is common to use it, nor if we can enable this feature when using 'make run_tests'. And I don't know if many CI runs multiple selftests in parallel from the same VM.
There are some other comments, but feel free to ignore them if they are not relevant, or if they can be fixed later.
Tested: make -C tools/testing/selftests \ TARGETS=net/packetdrill \ run_tests
Please note that this will not run the packetdrill tests in a dedicated netns. I don't think that's a good idea. Especially when sysctl knobs are being changed during the tests, and more.
make -C tools/testing/selftests \ TARGETS=net/packetdrill \ install INSTALL_PATH=$KSFT_INSTALL_PATH
# in virtme-ng ./run_kselftest.sh -c net/packetdrill ./run_kselftest.sh -t net/packetdrill:tcp_inq_client.pkt
I see that ./run_kselftest.sh can be executed with the "-n | --netns" option to run each test in a dedicated net namespace, but it doesn't seem to work:
# ./run_kselftest.sh -n -c net/packetdrill [ 411.087208] kselftest: Running tests in net/packetdrill TAP version 13 1..3 Cannot open network namespace "tcp_inq_client.pkt-TaQ8iN": No such file or directory setting the network namespace "tcp_inq_server.pkt-sW8YCz" failed: Invalid argument Cannot open network namespace "tcp_md5_md5-only-on-client-ack.pkt-YzJal6": No such file or directory
Ah, I guess this requires adding CONFIG_NET_NS=y to tools/testing/selftests/net/packetdrill/config
Good point. But I have CONFIG_NET_NS=y on my side. I didn't investigate more, I was first wondering if other people tried this option.
But maybe it would be better to create the netns in ksft_runner.sh? Please see below.
No, we opted for this design exactly to use existing kselftest infra, rather than reimplementing that in our wrapper, as I did in the RFC.
OK, I understood from the discussions from the RFC that by using the kselftest infra, the tests would be automatically executed in dedicated netns, and it could also help running tests in parallel. That sounded great to me, but that's not the case by default from what I see.
Perhaps that's something to change in the defaults for run_tests.
Since the infra exist, that is preferable over reimplementing it for one particular subset of tests.
Or if not all kselftests can run in netns (quite likely), this needs to be opt-in. Then a variable defined in the Makefile perhaps. To tell kselftest to enable the feature for this target.
On Fri, 06 Sep 2024 19:28:08 -0400 Willem de Bruijn wrote:
No, we opted for this design exactly to use existing kselftest infra, rather than reimplementing that in our wrapper, as I did in the RFC.
OK, I understood from the discussions from the RFC that by using the kselftest infra, the tests would be automatically executed in dedicated netns, and it could also help running tests in parallel. That sounded great to me, but that's not the case by default from what I see.
Perhaps that's something to change in the defaults for run_tests.
Since the infra exist, that is preferable over reimplementing it for one particular subset of tests.
Or if not all kselftests can run in netns (quite likely), this needs to be opt-in. Then a variable defined in the Makefile perhaps. To tell kselftest to enable the feature for this target.
Indeed, I was thinking along the same lines.
We're closing net-next in a week, it'd be great to have the baseline ksft interpreter mechanism in place in the next couple of days. The exact implementation of packetdrill/ksft_runner.sh can be changed later as needed, and the current one works fine for NIPA.
Hopefully we can also discuss at LPC/netconf what to do about libraries (where setup / cleanup code could live). Looking at MPTCP tests - do they work out of tree? I see mptcp_lib.sh does:
. "$(dirname "${0}")/../lib.sh" . "$(dirname "${0}")/../net_helper.sh"
but lib/sh and net_helper.sh are not listed in the Makefile. So they won't get packaged...
We should make sure we support running the tests with make run_tests and in installed mode.
If we agree that the current situation with support for library code is far from ideal, I think we have three(ish) directions to explore:
1 build netns handling into runner.sh + already mostly there + simpler tests, no need to worry about netns, it just happens - not all tests need netns (HW-adjacent tests especially) - netns setup is the main thing we need but not the only thing, wait helpers, python code, etc. also need to be handled
2a improve library bundling at the ksft level + we already have a net/lib "meta-target", it kinda works + hopefully in a way that lets us Python - no idea how
2b put all the code in kselftest/, like ktap_helpers.sh ? + easy to do + helps other subsystems - could cause git conflicts - won't help Python?
3 give up on target proliferation; on a quick count we have 15 targets in ksft for various bits of networking, faaar more than anyone else + fewer targets limits the need for libraries, libraries local to the target are trivial to handle - ksft has no other form of "grouping" tests, if we collapse into a small number of targets it will be hard to run a group of tests
Hi Jakub,
On 07/09/2024 02:04, Jakub Kicinski wrote:
On Fri, 06 Sep 2024 19:28:08 -0400 Willem de Bruijn wrote:
No, we opted for this design exactly to use existing kselftest infra, rather than reimplementing that in our wrapper, as I did in the RFC.
OK, I understood from the discussions from the RFC that by using the kselftest infra, the tests would be automatically executed in dedicated netns, and it could also help running tests in parallel. That sounded great to me, but that's not the case by default from what I see.
Perhaps that's something to change in the defaults for run_tests.
Since the infra exist, that is preferable over reimplementing it for one particular subset of tests.
Or if not all kselftests can run in netns (quite likely), this needs to be opt-in. Then a variable defined in the Makefile perhaps. To tell kselftest to enable the feature for this target.
Indeed, I was thinking along the same lines.
Yes, I was also thinking about a variable defined in the Makefile.
Because I suppose this variable will not be added in this cycle, and if a v3 is planned, would it be OK to simply prefix the 'packetdrill' commands with "unshare -n"? That would be similar to what is already done in Netfilter, and it prevents messing up with other tests/host settings?
We're closing net-next in a week, it'd be great to have the baseline ksft interpreter mechanism in place in the next couple of days. The exact implementation of packetdrill/ksft_runner.sh can be changed later as needed, and the current one works fine for NIPA.
It is fine for me if the v2 is applied. The suggested modifications were small, not blocking, fixes can be sent after (or could be ignored if preferred).
Hopefully we can also discuss at LPC/netconf what to do about libraries (where setup / cleanup code could live).
Good idea!
Looking at MPTCP tests - do they work out of tree? I see mptcp_lib.sh does:
. "$(dirname "${0}")/../lib.sh" . "$(dirname "${0}")/../net_helper.sh"
but lib/sh and net_helper.sh are not listed in the Makefile. So they won't get packaged...
Indeed, I noticed that when I reviewed Willem's series. It's only recently that we started to use these files. I already fixed these two on my side, I will share patches later after some testing.
We should make sure we support running the tests with make run_tests and in installed mode.
If we agree that the current situation with support for library code is far from ideal, I think we have three(ish) directions to explore:
1 build netns handling into runner.sh
- already mostly there
- simpler tests, no need to worry about netns, it just happens
- not all tests need netns (HW-adjacent tests especially)
- netns setup is the main thing we need but not the only thing, wait helpers, python code, etc. also need to be handled
Indeed, almost there. 'unshare' could also be used in runner.sh to simplify things.
Please note that for some tests, multiple netns are needed, e.g. to split the client and the server (and routers) in different netns. I don't think such setups should be handled by runner.sh.
If the use of a dedicated netns is triggered by a variable in the Makefile, it also means that all tests listed in this Makefile should work the same way, which is not always the case.
On the other hand, creating the netns is not difficult, even easier with the helper from lib.sh, and 'unshare -n' might be enough for simple cases.
2a improve library bundling at the ksft level
- we already have a net/lib "meta-target", it kinda works
- hopefully in a way that lets us Python
- no idea how
If the idea is only to launch tests in a dedicated netns, can we not change the shebang to launch the test script with:
unshare (...) <interpreter>
2b put all the code in kselftest/, like ktap_helpers.sh ?
- easy to do
- helps other subsystems
- could cause git conflicts
For me, that would be great to share more helpers between subsystems, but that looks difficult from a maintenance point of view.
- won't help Python?
3 give up on target proliferation; on a quick count we have 15 targets in ksft for various bits of networking, faaar more than anyone else
- fewer targets limits the need for libraries, libraries local to the target are trivial to handle
- ksft has no other form of "grouping" tests, if we collapse into a small number of targets it will be hard to run a group of tests
It is good to have targets, to easily run a group of tests related to a modification that has just been done, and to limit the size of the required kernel config, etc. Probably easier to have different libs per target/subsystem, and when something can be re-used elsewhere, it can be extracted to a more generic lib maybe?
Cheers, Matt
Matthieu Baerts wrote:
Hi Jakub,
On 07/09/2024 02:04, Jakub Kicinski wrote:
On Fri, 06 Sep 2024 19:28:08 -0400 Willem de Bruijn wrote:
No, we opted for this design exactly to use existing kselftest infra, rather than reimplementing that in our wrapper, as I did in the RFC.
OK, I understood from the discussions from the RFC that by using the kselftest infra, the tests would be automatically executed in dedicated netns, and it could also help running tests in parallel. That sounded great to me, but that's not the case by default from what I see.
Perhaps that's something to change in the defaults for run_tests.
Since the infra exist, that is preferable over reimplementing it for one particular subset of tests.
Or if not all kselftests can run in netns (quite likely), this needs to be opt-in. Then a variable defined in the Makefile perhaps. To tell kselftest to enable the feature for this target.
Indeed, I was thinking along the same lines.
Yes, I was also thinking about a variable defined in the Makefile.
Because I suppose this variable will not be added in this cycle, and if a v3 is planned, would it be OK to simply prefix the 'packetdrill' commands with "unshare -n"? That would be similar to what is already done in Netfilter, and it prevents messing up with other tests/host settings?
Each target is built and booted separately, right?
These three initial tests share set_defaults.sh, so in practice this should be fine. Most importantly, not affecting any tests outside net/packetdrill.
But agreed that netns are needed before adding more.
The unshare approach sounds fine to me. Easier than to plumb a Makefile variable through to the standalone run_kselftest.sh.
3 give up on target proliferation; on a quick count we have 15 targets in ksft for various bits of networking, faaar more than anyone else
- fewer targets limits the need for libraries, libraries local to the target are trivial to handle
- ksft has no other form of "grouping" tests, if we collapse into a small number of targets it will be hard to run a group of tests
It is good to have targets, to easily run a group of tests related to a modification that has just been done, and to limit the size of the required kernel config, etc. Probably easier to have different libs per target/subsystem, and when something can be re-used elsewhere, it can be extracted to a more generic lib maybe?
The conflicting CONFIGs between targets could be an issue. Even with packetdrill I had to check HZ and saw a difference with net/bpf.
That said, there could probably be a way to select tests between -c (collection/target) and -t (individual test) that uses a wildcard.
Hi Willem,
On 09/09/2024 15:15, Willem de Bruijn wrote:
Matthieu Baerts wrote:
Hi Jakub,
On 07/09/2024 02:04, Jakub Kicinski wrote:
On Fri, 06 Sep 2024 19:28:08 -0400 Willem de Bruijn wrote:
No, we opted for this design exactly to use existing kselftest infra, rather than reimplementing that in our wrapper, as I did in the RFC.
OK, I understood from the discussions from the RFC that by using the kselftest infra, the tests would be automatically executed in dedicated netns, and it could also help running tests in parallel. That sounded great to me, but that's not the case by default from what I see.
Perhaps that's something to change in the defaults for run_tests.
Since the infra exist, that is preferable over reimplementing it for one particular subset of tests.
Or if not all kselftests can run in netns (quite likely), this needs to be opt-in. Then a variable defined in the Makefile perhaps. To tell kselftest to enable the feature for this target.
Indeed, I was thinking along the same lines.
Yes, I was also thinking about a variable defined in the Makefile.
Because I suppose this variable will not be added in this cycle, and if a v3 is planned, would it be OK to simply prefix the 'packetdrill' commands with "unshare -n"? That would be similar to what is already done in Netfilter, and it prevents messing up with other tests/host settings?
Each target is built and booted separately, right?
From what I see, on NIPA, there are two executors dedicated to packetdrill: one with and one without a debug kernel config. Each of them is running in a dedicated VM.
So yes, on NIPA, we are safe.
But someone could run these packetdrill tests on a test machine manually, then switch to something else and have unexpected behaviours.
These three initial tests share set_defaults.sh, so in practice this should be fine. Most importantly, not affecting any tests outside net/packetdrill.
But agreed that netns are needed before adding more.
The unshare approach sounds fine to me. Easier than to plumb a Makefile variable through to the standalone run_kselftest.sh.
Yes, easier indeed.
3 give up on target proliferation; on a quick count we have 15 targets in ksft for various bits of networking, faaar more than anyone else
- fewer targets limits the need for libraries, libraries local to the target are trivial to handle
- ksft has no other form of "grouping" tests, if we collapse into a small number of targets it will be hard to run a group of tests
It is good to have targets, to easily run a group of tests related to a modification that has just been done, and to limit the size of the required kernel config, etc. Probably easier to have different libs per target/subsystem, and when something can be re-used elsewhere, it can be extracted to a more generic lib maybe?
The conflicting CONFIGs between targets could be an issue. Even with packetdrill I had to check HZ and saw a difference with net/bpf.
If some kernel configs are conflicting, it might be needed to add a check in ksft_runner.sh, because I know some CIs like LKFT build the kernel once mixing different kernel config files, then run the different targets on different VMs.
But maybe it is not needed to consider this case, and just adding CONFIG_HZ_100=y in the config file is enough.
That said, there could probably be a way to select tests between -c (collection/target) and -t (individual test) that uses a wildcard.
There are probably too many ways to run the selftests: personally, I never use run_kselftest.sh. Either I use 'make run_tests' to run all tests, or, when I need to check one specific selftest, I often directly execute the script manually, instead of dealing with the kselftest infrastructure. In this case, I can also simply use a for-loop using a wildcard in bash to execute all the tests I want.
Cheers, Matt
On Thu, 5 Sep 2024 19:15:50 -0400 Willem de Bruijn wrote:
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
1/2: add kselftest infra for TEST_PROGS that need an interpreter
2/2: add the specific packetdrill tests
Both can go through net-next, I imagine. But let me know if the core infra should go through linux-kselftest.
Okay, looks like the set has survived DaveM's weekend cleanup of patchwork :) I'm planning to apply it in the afternoon (Pacific time), please LMK if anyone has objections / needs more time to review. I'm expecting 'unshare -n' as a follow up.
Jakub Kicinski wrote:
On Thu, 5 Sep 2024 19:15:50 -0400 Willem de Bruijn wrote:
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
1/2: add kselftest infra for TEST_PROGS that need an interpreter
2/2: add the specific packetdrill tests
Both can go through net-next, I imagine. But let me know if the core infra should go through linux-kselftest.
Okay, looks like the set has survived DaveM's weekend cleanup of patchwork :) I'm planning to apply it in the afternoon (Pacific time), please LMK if anyone has objections / needs more time to review. I'm expecting 'unshare -n' as a follow up.
Which literally just inserting uname -n into the two invocations, right?
+++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh @@ -33,9 +33,9 @@ fi ktap_print_header ktap_set_plan 2
-packetdrill ${ipv4_args[@]} $(basename $script) > /dev/null \ +unshare -n packetdrill ${ipv4_args[@]} $(basename $script) > /dev/null \ && ktap_test_pass "ipv4" || ktap_test_fail "ipv4" -packetdrill ${ipv6_args[@]} $(basename $script) > /dev/null \ +unshare -n packetdrill ${ipv6_args[@]} $(basename $script) > /dev/null \ && ktap_test_pass "ipv6" || ktap_test_fail "ipv6"
I can send that. And then also add some CONFIGs we discussed:
+++ b/tools/testing/selftests/net/packetdrill/config @@ -1,5 +1,10 @@ CONFIG_IPV6=y +CONFIG_HZ_1000=y +CONFIG_HZ=1000 +CONFIG_NET_NS=y CONFIG_NET_SCH_FIFO=y CONFIG_PROC_SYSCTL=y +CONFIG_SYN_COOKIES=y +CONFIG_TCP_CONG_CUBIC=y CONFIG_TCP_MD5SIG=y CONFIG_TUN=y
And in the same series another two packetdrill testsuites, around 20 tests, say.
On Tue, 10 Sep 2024 13:45:41 -0400 Willem de Bruijn wrote:
Which literally just inserting uname -n into the two invocations, right?
Yes, AFAIU.
I can send that. And then also add some CONFIGs we discussed:
SG!
And in the same series another two packetdrill testsuites, around 20 tests, say.
SG!
Hello:
This series was applied to netdev/net-next.git (main) by Jakub Kicinski kuba@kernel.org:
On Thu, 5 Sep 2024 19:15:50 -0400 you wrote:
From: Willem de Bruijn willemb@google.com
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
1/2: add kselftest infra for TEST_PROGS that need an interpreter
[...]
Here is the summary with links: - [net-next,v2,1/2] selftests: support interpreted scripts with ksft_runner.sh https://git.kernel.org/netdev/net-next/c/dbd61921a6ad - [net-next,v2,2/2] selftests/net: integrate packetdrill with ksft https://git.kernel.org/netdev/net-next/c/8a405552fd3b
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Jakub Kicinski -
Matthieu Baerts -
patchwork-bot+netdevbpf＠kernel.org -
Willem de Bruijn