diff mbox series

[net-next,v2,2/2] selftests/net: integrate packetdrill with ksft

Message ID 20240905231653.2427327-3-willemdebruijn.kernel@gmail.com (mailing list archive)
State Accepted
Commit 8a405552fd3b1eefe186e724343e88790f6be832
Delegated to: Netdev Maintainers
Headers show
Series selftests/net: add packetdrill | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 17 this patch: 17
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success net selftest script(s) already in Makefile
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 22 this patch: 22
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-09-07--06-00 (tests: 722)

Commit Message

Willem de Bruijn Sept. 5, 2024, 11:15 p.m. UTC
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.kernel@gmail.com/
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

Comments

Matthieu Baerts (NGI0) Sept. 6, 2024, 11:43 a.m. UTC | #1
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
Willem de Bruijn Sept. 6, 2024, 3:36 p.m. UTC | #2
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.
>
Matthieu Baerts (NGI0) Sept. 6, 2024, 4:26 p.m. UTC | #3
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
Willem de Bruijn Sept. 6, 2024, 11:28 p.m. UTC | #4
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.
Jakub Kicinski Sept. 7, 2024, 12:04 a.m. UTC | #5
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
Matthieu Baerts (NGI0) Sept. 9, 2024, 7:42 a.m. UTC | #6
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
Willem de Bruijn Sept. 9, 2024, 1:15 p.m. UTC | #7
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.
Matthieu Baerts (NGI0) Sept. 9, 2024, 2:14 p.m. UTC | #8
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
diff mbox series

Patch

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