Matthieu Baerts wrote:
> Hi Willem,
>
> On 06/09/2024 01:15, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@xxxxxxxxxx>
> >
> > 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.
>
