diff mbox series

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

Message ID 20240827193417.2792223-1-willemdebruijn.kernel@gmail.com (mailing list archive)
State New
Headers show
Series [net-next,RFC] selftests/net: integrate packetdrill with ksft | expand

Commit Message

Willem de Bruijn Aug. 27, 2024, 7:32 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 implementation and reuses the
ksft python library for its KTAP, ksft, NetNS and other such tooling.

It also anticipates the large number of testcases, by creating a
separate kselftest for each feature (directory). It does this by
copying the template script packetdrill_ksft.py for each directory,
and putting those in TEST_CUSTOM_PROGS so that kselftests runs each.

To demonstrate the code with minimal patch size, 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.

Any future imports of packetdrill tests should require no additional
coding. Just add the tcp/$FEATURE directory with *.pkt files.

Implementation notes:
- restore alphabetical order when adding the new directory to
  tools/testing/selftests/Makefile
- copied *.pkt files and support verbatim from the github project,
  except for
    - update common/defaults.sh path (there are two paths on github)
    - add SPDX headers
    - remove one author statement
    - Acknowledgment: drop an e (checkpatch)

Tested:
	make -C tools/testing/selftests/ \
	  TARGETS=net/packetdrill \
	  install INSTALL_PATH=$KSFT_INSTALL_PATH

	# in virtme-ng
	sudo ./run_kselftest.sh -c net/packetdrill
	sudo ./run_kselftest.sh -t net/packetdrill:tcp_inq.py

Result:
	kselftest: Running tests in net/packetdrill
	TAP version 13
	1..2
	# timeout set to 45
	# selftests: net/packetdrill: tcp_inq.py
	# KTAP version 1
	# 1..4
	# ok 1 tcp_inq.client-v4
	# ok 2 tcp_inq.client-v6
	# ok 3 tcp_inq.server-v4
	# ok 4 tcp_inq.server-v6
	# # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
	ok 1 selftests: net/packetdrill: tcp_inq.py
	# timeout set to 45
	# selftests: net/packetdrill: tcp_md5.py
	# KTAP version 1
	# 1..2
	# ok 1 tcp_md5.md5-only-on-client-ack-v4
	# ok 2 tcp_md5.md5-only-on-client-ack-v6
	# # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
	ok 2 selftests: net/packetdrill: tcp_md5.py

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

RFC points for discussion

ksft: the choice for this python framework introduces a dependency on
the YNL scripts, and some non-obvious code:
- to include the net/lib dep in tools/testing/selftests/Makefile
- a boilerplate lib/py/__init__.py that each user of ksft will need
It seems preferable to me to use ksft.py over reinventing the wheel,
e.g., to print KTAP output. But perhaps we can make it more obvious
for future ksft users, and make the dependency on YNL optional.

kselftest-per-directory: copying packetdrill_ksft.py to create a
separate script per dir is a bit of a hack. A single script is much
simpler, optionally with nested KTAP (not supported yet by ksft). But,
I'm afraid that running time without intermediate output will be very
long when we integrate all packetdrill scripts.

nf_conntrack: we can dedup the common.sh.

*pkt files: which of the 150+ scripts on github are candidates for
kselftests, all or a subset? To avoid change detector tests. And what
is the best way to eventually send up to 150 files, 7K LoC.
---
 tools/testing/selftests/Makefile              |  7 +-
 .../selftests/net/packetdrill/.gitignore      |  1 +
 .../selftests/net/packetdrill/Makefile        | 28 ++++++
 .../net/packetdrill/lib/py/__init__.py        | 15 ++++
 .../net/packetdrill/packetdrill_ksft.py       | 90 +++++++++++++++++++
 .../net/packetdrill/tcp/common/defaults.sh    | 63 +++++++++++++
 .../net/packetdrill/tcp/common/set_sysctls.py | 38 ++++++++
 .../net/packetdrill/tcp/inq/client.pkt        | 51 +++++++++++
 .../net/packetdrill/tcp/inq/server.pkt        | 51 +++++++++++
 .../tcp/md5/md5-only-on-client-ack.pkt        | 28 ++++++
 10 files changed, 369 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/net/packetdrill/.gitignore
 create mode 100644 tools/testing/selftests/net/packetdrill/Makefile
 create mode 100644 tools/testing/selftests/net/packetdrill/lib/py/__init__.py
 create mode 100755 tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
 create mode 100755 tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh
 create mode 100755 tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
 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

Jakub Kicinski Aug. 28, 2024, 12:39 a.m. UTC | #1
Very exciting to see packetdrill tests making their way upstream!

On Tue, 27 Aug 2024 15:32:29 -0400 Willem de Bruijn wrote:
> RFC points for discussion
> 
> ksft: the choice for this python framework introduces a dependency on
> the YNL scripts, and some non-obvious code:
> - to include the net/lib dep in tools/testing/selftests/Makefile
> - a boilerplate lib/py/__init__.py that each user of ksft will need
> It seems preferable to me to use ksft.py over reinventing the wheel,
> e.g., to print KTAP output. But perhaps we can make it more obvious
> for future ksft users, and make the dependency on YNL optional.

No preference here, the wrapper script uses little of the python
code, and use of YNL seems unlikely, so it's a coin toss whether
it's worth linking up to net/lib/py or just writing a bit of bash.

> kselftest-per-directory: copying packetdrill_ksft.py to create a
> separate script per dir is a bit of a hack. A single script is much
> simpler, optionally with nested KTAP (not supported yet by ksft). But,
> I'm afraid that running time without intermediate output will be very
> long when we integrate all packetdrill scripts.

Not sure what you mean by intermediate output (the perl wrapper prints
immediately, do you have perl?). We do have some nested ktap parsing
in nipa, but small tweaks would be necessary to "decapsulate" the first
layer completely. My bigger concern would be runtime, if the time it
takes to run all tests grows we spawn multiple VMs and load balance
the test cases. 

All in all, trying to support single script is probably not worth the
extra effort.

> nf_conntrack: we can dedup the common.sh.
> 
> *pkt files: which of the 150+ scripts on github are candidates for
> kselftests, all or a subset? To avoid change detector tests.

Other than avoiding known flakes - no concerns. Is the distinction
between "change detector" vs hard tests (uAPI change / RFC compliance),
well defined? Not sure if that's really possible but if so it would be
nice to "sort" the tests into different dirs.

> And what
> is the best way to eventually send up to 150 files, 7K LoC.

Just send them, slice into a handful (<10) of patches if you can. 
7k LoC is same order of magnitude as initial drivers we merge.

To be clear let's start with a small patch like this one.
Since this is a new target I'll have to reconfigure the runners.
So we'll see how well this works once it's merged.
Spinning up new runners is a bit tedious but here new target seems 
quite justified.

>  tools/testing/selftests/Makefile              |  7 +-
>  .../selftests/net/packetdrill/.gitignore      |  1 +
>  .../selftests/net/packetdrill/Makefile        | 28 ++++++
>  .../net/packetdrill/lib/py/__init__.py        | 15 ++++
>  .../net/packetdrill/packetdrill_ksft.py       | 90 +++++++++++++++++++
>  .../net/packetdrill/tcp/common/defaults.sh    | 63 +++++++++++++
>  .../net/packetdrill/tcp/common/set_sysctls.py | 38 ++++++++
>  .../net/packetdrill/tcp/inq/client.pkt        | 51 +++++++++++
>  .../net/packetdrill/tcp/inq/server.pkt        | 51 +++++++++++
>  .../tcp/md5/md5-only-on-client-ack.pkt        | 28 ++++++

prolly need a config file to enable kconfig for MD5 ?
perils of adding new targets

> diff --git a/tools/testing/selftests/net/packetdrill/.gitignore b/tools/testing/selftests/net/packetdrill/.gitignore
> new file mode 100644
> index 0000000000000..a40f1a600eb94
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/.gitignore
> @@ -0,0 +1 @@
> +tcp*sh

Is this right or should it be tcp_*.py ?

> diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile
> new file mode 100644
> index 0000000000000..d94c51098d1f0
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/Makefile
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0

Would be nice to add a few lines here with an overview of how the
packetdrill tests get executed. People may jump in here to try to
add new tests, since that's how ksft usually operates.

> +def scriptname_to_testdir(filepath):
> +    """Extract the directory to run from this filename."""
> +
> +    suffix = ".sh"

.sh again ?

> diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh b/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh

> +# 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
> +

nit: double new line

> diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
> new file mode 100755
> index 0000000000000..5ddf456ae973a
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py

What calls this one? I can't grep it out.
Paolo Abeni Aug. 28, 2024, 8:20 a.m. UTC | #2
Adding Mat(s) for awareness, it would be great (but difficult) to have 
mptcp too in the long run ;)

On 8/27/24 21:32, 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,Addin
> in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based
> conntrack tests").
> 
> This patch takes a slightly different implementation and reuses the
> ksft python library for its KTAP, ksft, NetNS and other such tooling.
> 
> It also anticipates the large number of testcases, by creating a
> separate kselftest for each feature (directory). It does this by
> copying the template script packetdrill_ksft.py for each directory,
> and putting those in TEST_CUSTOM_PROGS so that kselftests runs each.
> 
> To demonstrate the code with minimal patch size, 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.
> 
> Any future imports of packetdrill tests should require no additional
> coding. Just add the tcp/$FEATURE directory with *.pkt files.
> 
> Implementation notes:
> - restore alphabetical order when adding the new directory to
>    tools/testing/selftests/Makefile
> - copied *.pkt files and support verbatim from the github project,
>    except for
>      - update common/defaults.sh path (there are two paths on github)
>      - add SPDX headers
>      - remove one author statement
>      - Acknowledgment: drop an e (checkpatch)
> 
> Tested:
> 	make -C tools/testing/selftests/ \
> 	  TARGETS=net/packetdrill \
> 	  install INSTALL_PATH=$KSFT_INSTALL_PATH
> 
> 	# in virtme-ng
> 	sudo ./run_kselftest.sh -c net/packetdrill
> 	sudo ./run_kselftest.sh -t net/packetdrill:tcp_inq.py
> 
> Result:
> 	kselftest: Running tests in net/packetdrill
> 	TAP version 13
> 	1..2
> 	# timeout set to 45
> 	# selftests: net/packetdrill: tcp_inq.py
> 	# KTAP version 1
> 	# 1..4
> 	# ok 1 tcp_inq.client-v4
> 	# ok 2 tcp_inq.client-v6
> 	# ok 3 tcp_inq.server-v4
> 	# ok 4 tcp_inq.server-v6
> 	# # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
> 	ok 1 selftests: net/packetdrill: tcp_inq.py
> 	# timeout set to 45
> 	# selftests: net/packetdrill: tcp_md5.py
> 	# KTAP version 1
> 	# 1..2
> 	# ok 1 tcp_md5.md5-only-on-client-ack-v4
> 	# ok 2 tcp_md5.md5-only-on-client-ack-v6
> 	# # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> 	ok 2 selftests: net/packetdrill: tcp_md5.py
> 
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> 
> ---
> 
> RFC points for discussion
> 
> ksft: the choice for this python framework introduces a dependency on
> the YNL scripts, and some non-obvious code:
> - to include the net/lib dep in tools/testing/selftests/Makefile
> - a boilerplate lib/py/__init__.py that each user of ksft will need
> It seems preferable to me to use ksft.py over reinventing the wheel,
> e.g., to print KTAP output. But perhaps we can make it more obvious
> for future ksft users, and make the dependency on YNL optional.
> 
> kselftest-per-directory: copying packetdrill_ksft.py to create a
> separate script per dir is a bit of a hack. 

Additionally, in some setups the test directory is RO, avoding file 
creation there would be better.

What about placing in each subdiretory a trivial wrapper invoking the 
'main' packetdrill_ksft.py script specifying as an argument the 
(sub-)directory to run/process?

> A single script is much
> simpler, optionally with nested KTAP (not supported yet by ksft). But,
> I'm afraid that running time without intermediate output will be very
> long when we integrate all packetdrill scripts.

If I read correctly, this runs the scripts in the given directory 
sequentially (as opposed to the default pktdrill run_all.py behavior 
that uses many concurrent threads).

I guess/fear that running all the pktdrill tests in a single batch would 
take quite a long time, which in turn could be not so good for CI 
integration. Currently there are a couple of CI test-cases with runtime 
 > 1h, but that is bad ;)

> nf_conntrack: we can dedup the common.sh.
> 
> *pkt files: which of the 150+ scripts on github are candidates for
> kselftests, all or a subset? To avoid change detector tests. And what
> is the best way to eventually send up to 150 files, 7K LoC.

I have no idea WRT the overall test stability, is some specific case/dir 
is subject to very frequent false positive/false negative we could 
postpone importing them, but ideally IMHO all the github scripts are 
good candidates.

Side note: I think it would be great to have some easy command line 
parameter to run only the specified script/test-case.

Thanks!

Paolo
Willem de Bruijn Aug. 28, 2024, 1:58 p.m. UTC | #3
Jakub Kicinski wrote:
> Very exciting to see packetdrill tests making their way upstream!
> 
> On Tue, 27 Aug 2024 15:32:29 -0400 Willem de Bruijn wrote:
> > RFC points for discussion
> > 
> > ksft: the choice for this python framework introduces a dependency on
> > the YNL scripts, and some non-obvious code:
> > - to include the net/lib dep in tools/testing/selftests/Makefile
> > - a boilerplate lib/py/__init__.py that each user of ksft will need
> > It seems preferable to me to use ksft.py over reinventing the wheel,
> > e.g., to print KTAP output. But perhaps we can make it more obvious
> > for future ksft users, and make the dependency on YNL optional.
> 
> No preference here, the wrapper script uses little of the python
> code, and use of YNL seems unlikely, so it's a coin toss whether
> it's worth linking up to net/lib/py or just writing a bit of bash.
> 
> > kselftest-per-directory: copying packetdrill_ksft.py to create a
> > separate script per dir is a bit of a hack. A single script is much
> > simpler, optionally with nested KTAP (not supported yet by ksft). But,
> > I'm afraid that running time without intermediate output will be very
> > long when we integrate all packetdrill scripts.
> 
> Not sure what you mean by intermediate output (the perl wrapper prints
> immediately, do you have perl?). We do have some nested ktap parsing
> in nipa, but small tweaks would be necessary to "decapsulate" the first
> layer completely. My bigger concern would be runtime, if the time it
> takes to run all tests grows we spawn multiple VMs and load balance
> the test cases. 
>
> All in all, trying to support single script is probably not worth the
> extra effort.
> 
> > nf_conntrack: we can dedup the common.sh.
> > 
> > *pkt files: which of the 150+ scripts on github are candidates for
> > kselftests, all or a subset? To avoid change detector tests.
> 
> Other than avoiding known flakes - no concerns. Is the distinction
> between "change detector" vs hard tests (uAPI change / RFC compliance),
> well defined? Not sure if that's really possible but if so it would be
> nice to "sort" the tests into different dirs.

It's not as clear-cut as API/RFC.

The major change detector issues are with timing, as packetdrill
scripts have timestamped lines. I think usec TCP timestamps were an
issue at some point. On-going is that we increase allowed variance on
debug builds. Note to self that that is missing here.

I think that all tests we open sourced to github so far were chosen to
be robust. Will double check and err on the side of caution.
 
> > And what
> > is the best way to eventually send up to 150 files, 7K LoC.
> 
> Just send them, slice into a handful (<10) of patches if you can. 
> 7k LoC is same order of magnitude as initial drivers we merge.
> 
> To be clear let's start with a small patch like this one.
> Since this is a new target I'll have to reconfigure the runners.
> So we'll see how well this works once it's merged.
> Spinning up new runners is a bit tedious but here new target seems 
> quite justified.

Ack. Thanks for that guidance.
 
> >  tools/testing/selftests/Makefile              |  7 +-
> >  .../selftests/net/packetdrill/.gitignore      |  1 +
> >  .../selftests/net/packetdrill/Makefile        | 28 ++++++
> >  .../net/packetdrill/lib/py/__init__.py        | 15 ++++
> >  .../net/packetdrill/packetdrill_ksft.py       | 90 +++++++++++++++++++
> >  .../net/packetdrill/tcp/common/defaults.sh    | 63 +++++++++++++
> >  .../net/packetdrill/tcp/common/set_sysctls.py | 38 ++++++++
> >  .../net/packetdrill/tcp/inq/client.pkt        | 51 +++++++++++
> >  .../net/packetdrill/tcp/inq/server.pkt        | 51 +++++++++++
> >  .../tcp/md5/md5-only-on-client-ack.pkt        | 28 ++++++
> 
> prolly need a config file to enable kconfig for MD5 ?
> perils of adding new targets
> 
> > diff --git a/tools/testing/selftests/net/packetdrill/.gitignore b/tools/testing/selftests/net/packetdrill/.gitignore
> > new file mode 100644
> > index 0000000000000..a40f1a600eb94
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/packetdrill/.gitignore
> > @@ -0,0 +1 @@
> > +tcp*sh
> 
> Is this right or should it be tcp_*.py ?
> 
> > diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile
> > new file mode 100644
> > index 0000000000000..d94c51098d1f0
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/packetdrill/Makefile
> > @@ -0,0 +1,28 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Would be nice to add a few lines here with an overview of how the
> packetdrill tests get executed. People may jump in here to try to
> add new tests, since that's how ksft usually operates.
> 
> > +def scriptname_to_testdir(filepath):
> > +    """Extract the directory to run from this filename."""
> > +
> > +    suffix = ".sh"
> 
> .sh again ?
> 
> > diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh b/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh
> 
> > +# 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
> > +
> 
> nit: double new line
> 
> > diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
> > new file mode 100755
> > index 0000000000000..5ddf456ae973a
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
> 
> What calls this one? I can't grep it out.

Whoops. I changed test directories and apparently these ones don't
rely on this.

Thanks for the review. I'll take a quick stab at a standalone script
to see whether that is easier. Else will resubmit with these and
Paolo's feedback addressed.

In particular to Paolo's feedback: running multiple tests in parallel
like run_all.py can. Come to think of it, maybe I should import
run_all.py, modifying it to output KTAP and ksft return codes.
Willem de Bruijn Aug. 28, 2024, 2:03 p.m. UTC | #4
Paolo Abeni wrote:
> Adding Mat(s) for awareness, it would be great (but difficult) to have 
> mptcp too in the long run ;)
> 
> On 8/27/24 21:32, 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,Addin
> > in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based
> > conntrack tests").
> > 
> > This patch takes a slightly different implementation and reuses the
> > ksft python library for its KTAP, ksft, NetNS and other such tooling.
> > 
> > It also anticipates the large number of testcases, by creating a
> > separate kselftest for each feature (directory). It does this by
> > copying the template script packetdrill_ksft.py for each directory,
> > and putting those in TEST_CUSTOM_PROGS so that kselftests runs each.
> > 
> > To demonstrate the code with minimal patch size, 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.
> > 
> > Any future imports of packetdrill tests should require no additional
> > coding. Just add the tcp/$FEATURE directory with *.pkt files.
> > 
> > Implementation notes:
> > - restore alphabetical order when adding the new directory to
> >    tools/testing/selftests/Makefile
> > - copied *.pkt files and support verbatim from the github project,
> >    except for
> >      - update common/defaults.sh path (there are two paths on github)
> >      - add SPDX headers
> >      - remove one author statement
> >      - Acknowledgment: drop an e (checkpatch)
> > 
> > Tested:
> > 	make -C tools/testing/selftests/ \
> > 	  TARGETS=net/packetdrill \
> > 	  install INSTALL_PATH=$KSFT_INSTALL_PATH
> > 
> > 	# in virtme-ng
> > 	sudo ./run_kselftest.sh -c net/packetdrill
> > 	sudo ./run_kselftest.sh -t net/packetdrill:tcp_inq.py
> > 
> > Result:
> > 	kselftest: Running tests in net/packetdrill
> > 	TAP version 13
> > 	1..2
> > 	# timeout set to 45
> > 	# selftests: net/packetdrill: tcp_inq.py
> > 	# KTAP version 1
> > 	# 1..4
> > 	# ok 1 tcp_inq.client-v4
> > 	# ok 2 tcp_inq.client-v6
> > 	# ok 3 tcp_inq.server-v4
> > 	# ok 4 tcp_inq.server-v6
> > 	# # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
> > 	ok 1 selftests: net/packetdrill: tcp_inq.py
> > 	# timeout set to 45
> > 	# selftests: net/packetdrill: tcp_md5.py
> > 	# KTAP version 1
> > 	# 1..2
> > 	# ok 1 tcp_md5.md5-only-on-client-ack-v4
> > 	# ok 2 tcp_md5.md5-only-on-client-ack-v6
> > 	# # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> > 	ok 2 selftests: net/packetdrill: tcp_md5.py
> > 
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > 
> > ---
> > 
> > RFC points for discussion
> > 
> > ksft: the choice for this python framework introduces a dependency on
> > the YNL scripts, and some non-obvious code:
> > - to include the net/lib dep in tools/testing/selftests/Makefile
> > - a boilerplate lib/py/__init__.py that each user of ksft will need
> > It seems preferable to me to use ksft.py over reinventing the wheel,
> > e.g., to print KTAP output. But perhaps we can make it more obvious
> > for future ksft users, and make the dependency on YNL optional.
> > 
> > kselftest-per-directory: copying packetdrill_ksft.py to create a
> > separate script per dir is a bit of a hack. 
> 
> Additionally, in some setups the test directory is RO, avoding file 
> creation there would be better.
> 
> What about placing in each subdiretory a trivial wrapper invoking the 
> 'main' packetdrill_ksft.py script specifying as an argument the 
> (sub-)directory to run/process?

The files are created by kselftests, similar to any TEST_GEN_XXX or
TEST_CUSTOM_PROGS.

If instead we prepopulate in each directory that is duplicative
boilerplate committed to git, and we still need to add it to
kselftests. Not sure whether TEST_PROGS with paths extending into
subdirectories are supported. And we do not want a TARGET for each
subdirectory.

It probably can be done, but I don't think that it is needed or
simpler.
 
> > A single script is much
> > simpler, optionally with nested KTAP (not supported yet by ksft). But,
> > I'm afraid that running time without intermediate output will be very
> > long when we integrate all packetdrill scripts.
> 
> If I read correctly, this runs the scripts in the given directory 
> sequentially (as opposed to the default pktdrill run_all.py behavior 
> that uses many concurrent threads).
> 
> I guess/fear that running all the pktdrill tests in a single batch would 
> take quite a long time, which in turn could be not so good for CI 
> integration. Currently there are a couple of CI test-cases with runtime 
>  > 1h, but that is bad ;)

Very good point, thanks! This is the third packetdrill runner that I'm
writing. I should know this by now.. Let me see whether I can use
run_all.py rather than reinvent the wheel here.
 
> > nf_conntrack: we can dedup the common.sh.
> > 
> > *pkt files: which of the 150+ scripts on github are candidates for
> > kselftests, all or a subset? To avoid change detector tests. And what
> > is the best way to eventually send up to 150 files, 7K LoC.
> 
> I have no idea WRT the overall test stability, is some specific case/dir 
> is subject to very frequent false positive/false negative we could 
> postpone importing them, but ideally IMHO all the github scripts are 
> good candidates.
> 
> Side note: I think it would be great to have some easy command line 
> parameter to run only the specified script/test-case.

Makes sense. Will include.
Stanislav Fomichev Aug. 28, 2024, 3:01 p.m. UTC | #5
On 08/27, 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 implementation and reuses the
> ksft python library for its KTAP, ksft, NetNS and other such tooling.
> 
> It also anticipates the large number of testcases, by creating a
> separate kselftest for each feature (directory). It does this by
> copying the template script packetdrill_ksft.py for each directory,
> and putting those in TEST_CUSTOM_PROGS so that kselftests runs each.
> 
> To demonstrate the code with minimal patch size, 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.
> 
> Any future imports of packetdrill tests should require no additional
> coding. Just add the tcp/$FEATURE directory with *.pkt files.
> 
> Implementation notes:
> - restore alphabetical order when adding the new directory to
>   tools/testing/selftests/Makefile
> - copied *.pkt files and support verbatim from the github project,
>   except for
>     - update common/defaults.sh path (there are two paths on github)
>     - add SPDX headers
>     - remove one author statement
>     - Acknowledgment: drop an e (checkpatch)
> 
> Tested:
> 	make -C tools/testing/selftests/ \
> 	  TARGETS=net/packetdrill \
> 	  install INSTALL_PATH=$KSFT_INSTALL_PATH
> 
> 	# in virtme-ng
> 	sudo ./run_kselftest.sh -c net/packetdrill
> 	sudo ./run_kselftest.sh -t net/packetdrill:tcp_inq.py
> 
> Result:
> 	kselftest: Running tests in net/packetdrill
> 	TAP version 13
> 	1..2
> 	# timeout set to 45
> 	# selftests: net/packetdrill: tcp_inq.py
> 	# KTAP version 1
> 	# 1..4
> 	# ok 1 tcp_inq.client-v4
> 	# ok 2 tcp_inq.client-v6
> 	# ok 3 tcp_inq.server-v4
> 	# ok 4 tcp_inq.server-v6
> 	# # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
> 	ok 1 selftests: net/packetdrill: tcp_inq.py
> 	# timeout set to 45
> 	# selftests: net/packetdrill: tcp_md5.py
> 	# KTAP version 1
> 	# 1..2
> 	# ok 1 tcp_md5.md5-only-on-client-ack-v4
> 	# ok 2 tcp_md5.md5-only-on-client-ack-v6
> 	# # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> 	ok 2 selftests: net/packetdrill: tcp_md5.py
> 
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> 
> ---
> 
> RFC points for discussion
> 
> ksft: the choice for this python framework introduces a dependency on
> the YNL scripts, and some non-obvious code:
> - to include the net/lib dep in tools/testing/selftests/Makefile
> - a boilerplate lib/py/__init__.py that each user of ksft will need
> It seems preferable to me to use ksft.py over reinventing the wheel,
> e.g., to print KTAP output. But perhaps we can make it more obvious
> for future ksft users, and make the dependency on YNL optional.
> 
> kselftest-per-directory: copying packetdrill_ksft.py to create a
> separate script per dir is a bit of a hack. A single script is much
> simpler, optionally with nested KTAP (not supported yet by ksft). But,
> I'm afraid that running time without intermediate output will be very
> long when we integrate all packetdrill scripts.
> 
> nf_conntrack: we can dedup the common.sh.
> 
> *pkt files: which of the 150+ scripts on github are candidates for
> kselftests, all or a subset? To avoid change detector tests. And what
> is the best way to eventually send up to 150 files, 7K LoC.
> ---
>  tools/testing/selftests/Makefile              |  7 +-
>  .../selftests/net/packetdrill/.gitignore      |  1 +
>  .../selftests/net/packetdrill/Makefile        | 28 ++++++
>  .../net/packetdrill/lib/py/__init__.py        | 15 ++++
>  .../net/packetdrill/packetdrill_ksft.py       | 90 +++++++++++++++++++
>  .../net/packetdrill/tcp/common/defaults.sh    | 63 +++++++++++++
>  .../net/packetdrill/tcp/common/set_sysctls.py | 38 ++++++++
>  .../net/packetdrill/tcp/inq/client.pkt        | 51 +++++++++++
>  .../net/packetdrill/tcp/inq/server.pkt        | 51 +++++++++++
>  .../tcp/md5/md5-only-on-client-ack.pkt        | 28 ++++++
>  10 files changed, 369 insertions(+), 3 deletions(-)
>  create mode 100644 tools/testing/selftests/net/packetdrill/.gitignore
>  create mode 100644 tools/testing/selftests/net/packetdrill/Makefile
>  create mode 100644 tools/testing/selftests/net/packetdrill/lib/py/__init__.py
>  create mode 100755 tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
>  create mode 100755 tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh
>  create mode 100755 tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
>  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..f03d6fee7ac54 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
> @@ -122,7 +123,7 @@ TARGETS_HOTPLUG = cpu-hotplug
>  TARGETS_HOTPLUG += memory-hotplug
>  
>  # Networking tests want the net/lib target, include it automatically
> -ifneq ($(filter net drivers/net drivers/net/hw,$(TARGETS)),)
> +ifneq ($(filter net net/packetdrill drivers/net drivers/net/hw,$(TARGETS)),)
>  ifeq ($(filter net/lib,$(TARGETS)),)
>  	INSTALL_DEP_TARGETS := net/lib
>  endif
> diff --git a/tools/testing/selftests/net/packetdrill/.gitignore b/tools/testing/selftests/net/packetdrill/.gitignore
> new file mode 100644
> index 0000000000000..a40f1a600eb94
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/.gitignore
> @@ -0,0 +1 @@
> +tcp*sh
> diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile
> new file mode 100644
> index 0000000000000..d94c51098d1f0
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/Makefile
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# KSFT includes
> +TEST_INCLUDES := $(wildcard lib/py/*.py ../lib/py/*.py)
> +
> +# Packetdrill support file(s)
> +TEST_INCLUDES += tcp/common/defaults.sh
> +TEST_INCLUDES += tcp/common/set_sysctls.py
> +
> +# Packetdrill scripts: all .pkt in subdirectories
> +TEST_INCLUDES += $(wildcard tcp/**/*.pkt)
> +
> +# Create a separate ksft test for each subdirectory
> +# Running all packetdrill tests in one go will take too long
> +#
> +# For each tcp/$subdir, create a test script tcp_$subdir.py
> +# Exclude tcp/common, which is a helper directory
> +TEST_DIRS := $(wildcard tcp/*)
> +TEST_DIRS := $(filter-out tcp/common, $(TEST_DIRS))
> +TEST_CUSTOM_PROGS := $(foreach dir,$(TEST_DIRS),$(subst /,_,$(dir)).py)
> +
> +$(TEST_CUSTOM_PROGS) : packetdrill_ksft.py
> +	cp $< $@
> +
> +# Needed to generate all TEST_CUSTOM_PROGS
> +all: $(TEST_CUSTOM_PROGS)
> +
> +include ../../lib.mk
> diff --git a/tools/testing/selftests/net/packetdrill/lib/py/__init__.py b/tools/testing/selftests/net/packetdrill/lib/py/__init__.py
> new file mode 100644
> index 0000000000000..51bb6dda43d65
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/lib/py/__init__.py
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import pathlib
> +import sys
> +
> +KSFT_DIR = (pathlib.Path(__file__).parent / "../../../..").resolve()
> +
> +try:
> +    sys.path.append(KSFT_DIR.as_posix())
> +    from net.lib.py import *
> +except ModuleNotFoundError as e:
> +    ksft_pr("Failed importing `net` library from kernel sources")
> +    ksft_pr(str(e))
> +    ktap_result(True, comment="SKIP")
> +    sys.exit(4)
> diff --git a/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py b/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
> new file mode 100755
> index 0000000000000..62572a5b8331c
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
> @@ -0,0 +1,90 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +"""Run packetdrill tests in the ksft harness.
> +
> +   Run all packetdrill tests in a subdirectory.
> +   Detect the relevant subdirectory from this script name.
> +   (Because the script cannot be given arguments.)
> +
> +   Run each test, for both IPv4 and IPv6.
> +   Return a separate ksft result for each test case.
> +"""
> +
> +import glob
> +import os
> +import pathlib
> +import shutil
> +
> +from lib.py import cmd, ksft_exit, ksft_run, KsftSkipEx, NetNS
> +
> +
> +def test_func_builder(pktfile_path, ipv4):
> +    """Create a function that can be passed to ksft_run."""
> +
> +    def f():
> +        if 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 "
> +                    )
> +        else:
> +            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 not shutil.which("packetdrill"):
> +            raise KsftSkipEx("Cannot find packetdrill")

I don't see anything about building the binary itself. Am I missing
something or should we also have some makefile magic to do it?
I'm not sure packetdrill is packaged by the distros... Presumably
we want the existing NIPA runners to run those tests as well?

> +
> +        netns = NetNS()
> +
> +        # Call packetdrill from the directory hosting the .pkt script,
> +        # because scripts can have relative includes.
> +        savedir = os.getcwd()
> +        os.chdir(os.path.dirname(pktfile_path))
> +        basename = os.path.basename(pktfile_path)
> +        cmd(f"packetdrill {args} {basename}", ns=netns)
> +        os.chdir(savedir)
> +
> +    if ipv4:
> +        f.__name__ = pathlib.Path(pktfile_path).stem + "-v4"
> +    else:
> +        f.__name__ = pathlib.Path(pktfile_path).stem + "-v6"

What about dualstack v4-over-v6 mode? Do we want to support that as
well?
Jakub Kicinski Aug. 28, 2024, 3:36 p.m. UTC | #6
On Wed, 28 Aug 2024 08:01:02 -0700 Stanislav Fomichev wrote:
> I don't see anything about building the binary itself. Am I missing
> something or should we also have some makefile magic to do it?
> I'm not sure packetdrill is packaged by the distros... Presumably
> we want the existing NIPA runners to run those tests as well?

We already build it from source, for netfilter.
Willem de Bruijn Aug. 28, 2024, 3:43 p.m. UTC | #7
Stanislav Fomichev wrote:
> On 08/27, 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 implementation and reuses the
> > ksft python library for its KTAP, ksft, NetNS and other such tooling.
> > 
> > It also anticipates the large number of testcases, by creating a
> > separate kselftest for each feature (directory). It does this by
> > copying the template script packetdrill_ksft.py for each directory,
> > and putting those in TEST_CUSTOM_PROGS so that kselftests runs each.
> > 
> > To demonstrate the code with minimal patch size, 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.
> > 
> > Any future imports of packetdrill tests should require no additional
> > coding. Just add the tcp/$FEATURE directory with *.pkt files.
> > 
> > Implementation notes:
> > - restore alphabetical order when adding the new directory to
> >   tools/testing/selftests/Makefile
> > - copied *.pkt files and support verbatim from the github project,
> >   except for
> >     - update common/defaults.sh path (there are two paths on github)
> >     - add SPDX headers
> >     - remove one author statement
> >     - Acknowledgment: drop an e (checkpatch)
> > 
> > Tested:
> > 	make -C tools/testing/selftests/ \
> > 	  TARGETS=net/packetdrill \
> > 	  install INSTALL_PATH=$KSFT_INSTALL_PATH
> > 
> > 	# in virtme-ng
> > 	sudo ./run_kselftest.sh -c net/packetdrill
> > 	sudo ./run_kselftest.sh -t net/packetdrill:tcp_inq.py
> > 
> > Result:
> > 	kselftest: Running tests in net/packetdrill
> > 	TAP version 13
> > 	1..2
> > 	# timeout set to 45
> > 	# selftests: net/packetdrill: tcp_inq.py
> > 	# KTAP version 1
> > 	# 1..4
> > 	# ok 1 tcp_inq.client-v4
> > 	# ok 2 tcp_inq.client-v6
> > 	# ok 3 tcp_inq.server-v4
> > 	# ok 4 tcp_inq.server-v6
> > 	# # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
> > 	ok 1 selftests: net/packetdrill: tcp_inq.py
> > 	# timeout set to 45
> > 	# selftests: net/packetdrill: tcp_md5.py
> > 	# KTAP version 1
> > 	# 1..2
> > 	# ok 1 tcp_md5.md5-only-on-client-ack-v4
> > 	# ok 2 tcp_md5.md5-only-on-client-ack-v6
> > 	# # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> > 	ok 2 selftests: net/packetdrill: tcp_md5.py
> > 
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > 
> > ---
> > 
> > RFC points for discussion
> > 
> > ksft: the choice for this python framework introduces a dependency on
> > the YNL scripts, and some non-obvious code:
> > - to include the net/lib dep in tools/testing/selftests/Makefile
> > - a boilerplate lib/py/__init__.py that each user of ksft will need
> > It seems preferable to me to use ksft.py over reinventing the wheel,
> > e.g., to print KTAP output. But perhaps we can make it more obvious
> > for future ksft users, and make the dependency on YNL optional.
> > 
> > kselftest-per-directory: copying packetdrill_ksft.py to create a
> > separate script per dir is a bit of a hack. A single script is much
> > simpler, optionally with nested KTAP (not supported yet by ksft). But,
> > I'm afraid that running time without intermediate output will be very
> > long when we integrate all packetdrill scripts.
> > 
> > nf_conntrack: we can dedup the common.sh.
> > 
> > *pkt files: which of the 150+ scripts on github are candidates for
> > kselftests, all or a subset? To avoid change detector tests. And what
> > is the best way to eventually send up to 150 files, 7K LoC.
> > ---
> >  tools/testing/selftests/Makefile              |  7 +-
> >  .../selftests/net/packetdrill/.gitignore      |  1 +
> >  .../selftests/net/packetdrill/Makefile        | 28 ++++++
> >  .../net/packetdrill/lib/py/__init__.py        | 15 ++++
> >  .../net/packetdrill/packetdrill_ksft.py       | 90 +++++++++++++++++++
> >  .../net/packetdrill/tcp/common/defaults.sh    | 63 +++++++++++++
> >  .../net/packetdrill/tcp/common/set_sysctls.py | 38 ++++++++
> >  .../net/packetdrill/tcp/inq/client.pkt        | 51 +++++++++++
> >  .../net/packetdrill/tcp/inq/server.pkt        | 51 +++++++++++
> >  .../tcp/md5/md5-only-on-client-ack.pkt        | 28 ++++++
> >  10 files changed, 369 insertions(+), 3 deletions(-)
> >  create mode 100644 tools/testing/selftests/net/packetdrill/.gitignore
> >  create mode 100644 tools/testing/selftests/net/packetdrill/Makefile
> >  create mode 100644 tools/testing/selftests/net/packetdrill/lib/py/__init__.py
> >  create mode 100755 tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
> >  create mode 100755 tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh
> >  create mode 100755 tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
> >  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..f03d6fee7ac54 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
> > @@ -122,7 +123,7 @@ TARGETS_HOTPLUG = cpu-hotplug
> >  TARGETS_HOTPLUG += memory-hotplug
> >  
> >  # Networking tests want the net/lib target, include it automatically
> > -ifneq ($(filter net drivers/net drivers/net/hw,$(TARGETS)),)
> > +ifneq ($(filter net net/packetdrill drivers/net drivers/net/hw,$(TARGETS)),)
> >  ifeq ($(filter net/lib,$(TARGETS)),)
> >  	INSTALL_DEP_TARGETS := net/lib
> >  endif
> > diff --git a/tools/testing/selftests/net/packetdrill/.gitignore b/tools/testing/selftests/net/packetdrill/.gitignore
> > new file mode 100644
> > index 0000000000000..a40f1a600eb94
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/packetdrill/.gitignore
> > @@ -0,0 +1 @@
> > +tcp*sh
> > diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile
> > new file mode 100644
> > index 0000000000000..d94c51098d1f0
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/packetdrill/Makefile
> > @@ -0,0 +1,28 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +# KSFT includes
> > +TEST_INCLUDES := $(wildcard lib/py/*.py ../lib/py/*.py)
> > +
> > +# Packetdrill support file(s)
> > +TEST_INCLUDES += tcp/common/defaults.sh
> > +TEST_INCLUDES += tcp/common/set_sysctls.py
> > +
> > +# Packetdrill scripts: all .pkt in subdirectories
> > +TEST_INCLUDES += $(wildcard tcp/**/*.pkt)
> > +
> > +# Create a separate ksft test for each subdirectory
> > +# Running all packetdrill tests in one go will take too long
> > +#
> > +# For each tcp/$subdir, create a test script tcp_$subdir.py
> > +# Exclude tcp/common, which is a helper directory
> > +TEST_DIRS := $(wildcard tcp/*)
> > +TEST_DIRS := $(filter-out tcp/common, $(TEST_DIRS))
> > +TEST_CUSTOM_PROGS := $(foreach dir,$(TEST_DIRS),$(subst /,_,$(dir)).py)
> > +
> > +$(TEST_CUSTOM_PROGS) : packetdrill_ksft.py
> > +	cp $< $@
> > +
> > +# Needed to generate all TEST_CUSTOM_PROGS
> > +all: $(TEST_CUSTOM_PROGS)
> > +
> > +include ../../lib.mk
> > diff --git a/tools/testing/selftests/net/packetdrill/lib/py/__init__.py b/tools/testing/selftests/net/packetdrill/lib/py/__init__.py
> > new file mode 100644
> > index 0000000000000..51bb6dda43d65
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/packetdrill/lib/py/__init__.py
> > @@ -0,0 +1,15 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +import pathlib
> > +import sys
> > +
> > +KSFT_DIR = (pathlib.Path(__file__).parent / "../../../..").resolve()
> > +
> > +try:
> > +    sys.path.append(KSFT_DIR.as_posix())
> > +    from net.lib.py import *
> > +except ModuleNotFoundError as e:
> > +    ksft_pr("Failed importing `net` library from kernel sources")
> > +    ksft_pr(str(e))
> > +    ktap_result(True, comment="SKIP")
> > +    sys.exit(4)
> > diff --git a/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py b/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
> > new file mode 100755
> > index 0000000000000..62572a5b8331c
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
> > @@ -0,0 +1,90 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +"""Run packetdrill tests in the ksft harness.
> > +
> > +   Run all packetdrill tests in a subdirectory.
> > +   Detect the relevant subdirectory from this script name.
> > +   (Because the script cannot be given arguments.)
> > +
> > +   Run each test, for both IPv4 and IPv6.
> > +   Return a separate ksft result for each test case.
> > +"""
> > +
> > +import glob
> > +import os
> > +import pathlib
> > +import shutil
> > +
> > +from lib.py import cmd, ksft_exit, ksft_run, KsftSkipEx, NetNS
> > +
> > +
> > +def test_func_builder(pktfile_path, ipv4):
> > +    """Create a function that can be passed to ksft_run."""
> > +
> > +    def f():
> > +        if 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 "
> > +                    )
> > +        else:
> > +            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 not shutil.which("packetdrill"):
> > +            raise KsftSkipEx("Cannot find packetdrill")
> 
> I don't see anything about building the binary itself. Am I missing
> something or should we also have some makefile magic to do it?
> I'm not sure packetdrill is packaged by the distros... Presumably
> we want the existing NIPA runners to run those tests as well?

As Jakub responded.

Indeed importing the whole application into the kernel sources is what
stopped me from attempting this before. Florian's nf_conntrack charted
the path here.
 
> > +
> > +        netns = NetNS()
> > +
> > +        # Call packetdrill from the directory hosting the .pkt script,
> > +        # because scripts can have relative includes.
> > +        savedir = os.getcwd()
> > +        os.chdir(os.path.dirname(pktfile_path))
> > +        basename = os.path.basename(pktfile_path)
> > +        cmd(f"packetdrill {args} {basename}", ns=netns)
> > +        os.chdir(savedir)
> > +
> > +    if ipv4:
> > +        f.__name__ = pathlib.Path(pktfile_path).stem + "-v4"
> > +    else:
> > +        f.__name__ = pathlib.Path(pktfile_path).stem + "-v6"
> 
> What about dualstack v4-over-v6 mode? Do we want to support that as
> well?

Does it give a sufficient signal? I skipped over it on purpose. But
can enable if that is the consensus.
Jakub Kicinski Aug. 28, 2024, 4:01 p.m. UTC | #8
On Wed, 28 Aug 2024 10:03:55 -0400 Willem de Bruijn wrote:
> > > A single script is much
> > > simpler, optionally with nested KTAP (not supported yet by ksft). But,
> > > I'm afraid that running time without intermediate output will be very
> > > long when we integrate all packetdrill scripts.  
> > 
> > If I read correctly, this runs the scripts in the given directory 
> > sequentially (as opposed to the default pktdrill run_all.py behavior 
> > that uses many concurrent threads).
> > 
> > I guess/fear that running all the pktdrill tests in a single batch would 
> > take quite a long time, which in turn could be not so good for CI 
> > integration. Currently there are a couple of CI test-cases with runtime   
> >  > 1h, but that is bad ;)  
> 
> Very good point, thanks! This is the third packetdrill runner that I'm
> writing. I should know this by now.. Let me see whether I can use
> run_all.py rather than reinvent the wheel here.

Do we have to worry about this now? If you're planning to add a runner
specifically for packetdrill... IDK if we should. We already have a few
runners, not to mention that run_kselftest.sh itself can run all the
test cases in parallel in separate network namespaces!

What I was wondering is whether we can use shebang to direct the .pkt
files to be "executed" by the python script. Alternatively we can add
support to specifying "interpreter" for a given test in ksft infra
(kinda like we can pass cmd line arguments to a test). Completely
untested but it should give better idea what I mean than a description:

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index 74954f6a8f94..429c279e9c6e 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -56,6 +56,7 @@ run_one()
 	export kselftest_timeout="$kselftest_default_timeout"
 
 	# Safe default if tr not available
+	kselftest_interp_ref="KSELFTEST_INTERP"
 	kselftest_cmd_args_ref="KSELFTEST_ARGS"
 
 	# Optional arguments for this command, possibly defined as an
@@ -78,6 +79,14 @@ run_one()
 					$TR_CMD [:lower:] [:upper:])
 		kselftest_cmd_args_ref="KSELFTEST_${BASENAME_SANITIZED}_ARGS"
 	fi
+	# Optional interpreter to run the test case
+	if [ -n "$TR_CMD" ]; then
+		SUFFIX_SANITIZED=$(echo "${BASENAME_TEST#*.}" | \
+				       $TR_CMD -d "[:blank:][:cntrl:]" | \
+				       $TR_CMD -c "[:alnum:]_" "_" | \
+				       $TR_CMD [:lower:] [:upper:])
+		kselftest_interp_ref="KSELFTEST_${SUFFIX_SANITIZED}_INTERP"
+	fi
 
 	# Load per-test-directory kselftest "settings" file.
 	settings="$BASE_DIR/$DIR/settings"
@@ -110,8 +119,12 @@ run_one()
 		if [ -x /usr/bin/stdbuf ]; then
 			stdbuf="/usr/bin/stdbuf --output=L "
 		fi
+		eval kselftest_interp="\$${kselftest_interp_ref:-}"
+		# Add a space at the end if interpreter is set to work in $cmd
+		[ -n "$kselftest_interp" ] && \
+		    kselftest_interp="$kselftest_interp "
 		eval kselftest_cmd_args="\$${kselftest_cmd_args_ref:-}"
-		cmd="$stdbuf ./$BASENAME_TEST $kselftest_cmd_args"
+		cmd="$stdbuf ./$kselftest_interp$BASENAME_TEST $kselftest_cmd_args"
 		if [ ! -x "$TEST" ]; then
 			echo "# Warning: file $TEST is not executable"
Matthieu Baerts Aug. 28, 2024, 4:26 p.m. UTC | #9
Hi Willem, Paolo,

Thank you for this series, that would be great to have the CI validating
these packetdrill tests!

(Having the Netdev CI validating these packetdrill tests was part of my
suggestions for the discussion we will have at NetConf :) )

On 28/08/2024 16:03, Willem de Bruijn wrote:
> Paolo Abeni wrote:
>> Adding Mat(s) for awareness, it would be great (but difficult) to have 
>> mptcp too in the long run ;)

Thank you for having Cced me! Yes, MPTCP's packetdrill is still not
ready for upstream :-/

But we could eventually have a runner on our side doing the validation
of the MPTCP packetdrill tests.

>> On 8/27/24 21:32, Willem de Bruijn wrote:
>>> From: Willem de Bruijn <willemb@google.com>

(...)

>>> A single script is much
>>> simpler, optionally with nested KTAP (not supported yet by ksft). But,
>>> I'm afraid that running time without intermediate output will be very
>>> long when we integrate all packetdrill scripts.
>>
>> If I read correctly, this runs the scripts in the given directory 
>> sequentially (as opposed to the default pktdrill run_all.py behavior 
>> that uses many concurrent threads).
>>
>> I guess/fear that running all the pktdrill tests in a single batch would 
>> take quite a long time, which in turn could be not so good for CI 
>> integration. Currently there are a couple of CI test-cases with runtime 
>>  > 1h, but that is bad ;)
> 
> Very good point, thanks! This is the third packetdrill runner that I'm
> writing. I should know this by now.. Let me see whether I can use
> run_all.py rather than reinvent the wheel here.

If you use run_all.py, you might want to merge this PR:

  https://github.com/google/packetdrill/pull/83

On MPTCP side, we run packetdrill using:

  run_all.py -t ${TAP_DIR} -l -v -P $(($(nproc) * 2)) ${PKTD_DIR}

For our case, it is easier to generate the TAP output in a different
directory, but I guess that could also be part of the standard output.

Also, if we have to limit the number of tests being executed in parallel
with -P, to reduce the instabilities.

Cheers,
Matt
Stanislav Fomichev Aug. 28, 2024, 5:20 p.m. UTC | #10
On 08/28, Willem de Bruijn wrote:
> Stanislav Fomichev wrote:
> > On 08/27, 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 implementation and reuses the
> > > ksft python library for its KTAP, ksft, NetNS and other such tooling.
> > > 
> > > It also anticipates the large number of testcases, by creating a
> > > separate kselftest for each feature (directory). It does this by
> > > copying the template script packetdrill_ksft.py for each directory,
> > > and putting those in TEST_CUSTOM_PROGS so that kselftests runs each.
> > > 
> > > To demonstrate the code with minimal patch size, 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.
> > > 
> > > Any future imports of packetdrill tests should require no additional
> > > coding. Just add the tcp/$FEATURE directory with *.pkt files.
> > > 
> > > Implementation notes:
> > > - restore alphabetical order when adding the new directory to
> > >   tools/testing/selftests/Makefile
> > > - copied *.pkt files and support verbatim from the github project,
> > >   except for
> > >     - update common/defaults.sh path (there are two paths on github)
> > >     - add SPDX headers
> > >     - remove one author statement
> > >     - Acknowledgment: drop an e (checkpatch)
> > > 
> > > Tested:
> > > 	make -C tools/testing/selftests/ \
> > > 	  TARGETS=net/packetdrill \
> > > 	  install INSTALL_PATH=$KSFT_INSTALL_PATH
> > > 
> > > 	# in virtme-ng
> > > 	sudo ./run_kselftest.sh -c net/packetdrill
> > > 	sudo ./run_kselftest.sh -t net/packetdrill:tcp_inq.py
> > > 
> > > Result:
> > > 	kselftest: Running tests in net/packetdrill
> > > 	TAP version 13
> > > 	1..2
> > > 	# timeout set to 45
> > > 	# selftests: net/packetdrill: tcp_inq.py
> > > 	# KTAP version 1
> > > 	# 1..4
> > > 	# ok 1 tcp_inq.client-v4
> > > 	# ok 2 tcp_inq.client-v6
> > > 	# ok 3 tcp_inq.server-v4
> > > 	# ok 4 tcp_inq.server-v6
> > > 	# # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
> > > 	ok 1 selftests: net/packetdrill: tcp_inq.py
> > > 	# timeout set to 45
> > > 	# selftests: net/packetdrill: tcp_md5.py
> > > 	# KTAP version 1
> > > 	# 1..2
> > > 	# ok 1 tcp_md5.md5-only-on-client-ack-v4
> > > 	# ok 2 tcp_md5.md5-only-on-client-ack-v6
> > > 	# # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> > > 	ok 2 selftests: net/packetdrill: tcp_md5.py
> > > 
> > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > 
> > > ---
> > > 
> > > RFC points for discussion
> > > 
> > > ksft: the choice for this python framework introduces a dependency on
> > > the YNL scripts, and some non-obvious code:
> > > - to include the net/lib dep in tools/testing/selftests/Makefile
> > > - a boilerplate lib/py/__init__.py that each user of ksft will need
> > > It seems preferable to me to use ksft.py over reinventing the wheel,
> > > e.g., to print KTAP output. But perhaps we can make it more obvious
> > > for future ksft users, and make the dependency on YNL optional.
> > > 
> > > kselftest-per-directory: copying packetdrill_ksft.py to create a
> > > separate script per dir is a bit of a hack. A single script is much
> > > simpler, optionally with nested KTAP (not supported yet by ksft). But,
> > > I'm afraid that running time without intermediate output will be very
> > > long when we integrate all packetdrill scripts.
> > > 
> > > nf_conntrack: we can dedup the common.sh.
> > > 
> > > *pkt files: which of the 150+ scripts on github are candidates for
> > > kselftests, all or a subset? To avoid change detector tests. And what
> > > is the best way to eventually send up to 150 files, 7K LoC.
> > > ---
> > >  tools/testing/selftests/Makefile              |  7 +-
> > >  .../selftests/net/packetdrill/.gitignore      |  1 +
> > >  .../selftests/net/packetdrill/Makefile        | 28 ++++++
> > >  .../net/packetdrill/lib/py/__init__.py        | 15 ++++
> > >  .../net/packetdrill/packetdrill_ksft.py       | 90 +++++++++++++++++++
> > >  .../net/packetdrill/tcp/common/defaults.sh    | 63 +++++++++++++
> > >  .../net/packetdrill/tcp/common/set_sysctls.py | 38 ++++++++
> > >  .../net/packetdrill/tcp/inq/client.pkt        | 51 +++++++++++
> > >  .../net/packetdrill/tcp/inq/server.pkt        | 51 +++++++++++
> > >  .../tcp/md5/md5-only-on-client-ack.pkt        | 28 ++++++
> > >  10 files changed, 369 insertions(+), 3 deletions(-)
> > >  create mode 100644 tools/testing/selftests/net/packetdrill/.gitignore
> > >  create mode 100644 tools/testing/selftests/net/packetdrill/Makefile
> > >  create mode 100644 tools/testing/selftests/net/packetdrill/lib/py/__init__.py
> > >  create mode 100755 tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
> > >  create mode 100755 tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh
> > >  create mode 100755 tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
> > >  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..f03d6fee7ac54 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
> > > @@ -122,7 +123,7 @@ TARGETS_HOTPLUG = cpu-hotplug
> > >  TARGETS_HOTPLUG += memory-hotplug
> > >  
> > >  # Networking tests want the net/lib target, include it automatically
> > > -ifneq ($(filter net drivers/net drivers/net/hw,$(TARGETS)),)
> > > +ifneq ($(filter net net/packetdrill drivers/net drivers/net/hw,$(TARGETS)),)
> > >  ifeq ($(filter net/lib,$(TARGETS)),)
> > >  	INSTALL_DEP_TARGETS := net/lib
> > >  endif
> > > diff --git a/tools/testing/selftests/net/packetdrill/.gitignore b/tools/testing/selftests/net/packetdrill/.gitignore
> > > new file mode 100644
> > > index 0000000000000..a40f1a600eb94
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/net/packetdrill/.gitignore
> > > @@ -0,0 +1 @@
> > > +tcp*sh
> > > diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile
> > > new file mode 100644
> > > index 0000000000000..d94c51098d1f0
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/net/packetdrill/Makefile
> > > @@ -0,0 +1,28 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +
> > > +# KSFT includes
> > > +TEST_INCLUDES := $(wildcard lib/py/*.py ../lib/py/*.py)
> > > +
> > > +# Packetdrill support file(s)
> > > +TEST_INCLUDES += tcp/common/defaults.sh
> > > +TEST_INCLUDES += tcp/common/set_sysctls.py
> > > +
> > > +# Packetdrill scripts: all .pkt in subdirectories
> > > +TEST_INCLUDES += $(wildcard tcp/**/*.pkt)
> > > +
> > > +# Create a separate ksft test for each subdirectory
> > > +# Running all packetdrill tests in one go will take too long
> > > +#
> > > +# For each tcp/$subdir, create a test script tcp_$subdir.py
> > > +# Exclude tcp/common, which is a helper directory
> > > +TEST_DIRS := $(wildcard tcp/*)
> > > +TEST_DIRS := $(filter-out tcp/common, $(TEST_DIRS))
> > > +TEST_CUSTOM_PROGS := $(foreach dir,$(TEST_DIRS),$(subst /,_,$(dir)).py)
> > > +
> > > +$(TEST_CUSTOM_PROGS) : packetdrill_ksft.py
> > > +	cp $< $@
> > > +
> > > +# Needed to generate all TEST_CUSTOM_PROGS
> > > +all: $(TEST_CUSTOM_PROGS)
> > > +
> > > +include ../../lib.mk
> > > diff --git a/tools/testing/selftests/net/packetdrill/lib/py/__init__.py b/tools/testing/selftests/net/packetdrill/lib/py/__init__.py
> > > new file mode 100644
> > > index 0000000000000..51bb6dda43d65
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/net/packetdrill/lib/py/__init__.py
> > > @@ -0,0 +1,15 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +
> > > +import pathlib
> > > +import sys
> > > +
> > > +KSFT_DIR = (pathlib.Path(__file__).parent / "../../../..").resolve()
> > > +
> > > +try:
> > > +    sys.path.append(KSFT_DIR.as_posix())
> > > +    from net.lib.py import *
> > > +except ModuleNotFoundError as e:
> > > +    ksft_pr("Failed importing `net` library from kernel sources")
> > > +    ksft_pr(str(e))
> > > +    ktap_result(True, comment="SKIP")
> > > +    sys.exit(4)
> > > diff --git a/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py b/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
> > > new file mode 100755
> > > index 0000000000000..62572a5b8331c
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
> > > @@ -0,0 +1,90 @@
> > > +#!/usr/bin/env python3
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +
> > > +"""Run packetdrill tests in the ksft harness.
> > > +
> > > +   Run all packetdrill tests in a subdirectory.
> > > +   Detect the relevant subdirectory from this script name.
> > > +   (Because the script cannot be given arguments.)
> > > +
> > > +   Run each test, for both IPv4 and IPv6.
> > > +   Return a separate ksft result for each test case.
> > > +"""
> > > +
> > > +import glob
> > > +import os
> > > +import pathlib
> > > +import shutil
> > > +
> > > +from lib.py import cmd, ksft_exit, ksft_run, KsftSkipEx, NetNS
> > > +
> > > +
> > > +def test_func_builder(pktfile_path, ipv4):
> > > +    """Create a function that can be passed to ksft_run."""
> > > +
> > > +    def f():
> > > +        if 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 "
> > > +                    )
> > > +        else:
> > > +            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 not shutil.which("packetdrill"):
> > > +            raise KsftSkipEx("Cannot find packetdrill")
> > 
> > I don't see anything about building the binary itself. Am I missing
> > something or should we also have some makefile magic to do it?
> > I'm not sure packetdrill is packaged by the distros... Presumably
> > we want the existing NIPA runners to run those tests as well?
> 
> As Jakub responded.
> 
> Indeed importing the whole application into the kernel sources is what
> stopped me from attempting this before. Florian's nf_conntrack charted
> the path here.

Thanks both. I was expecting to find some build rules :-) But as long
as we have this story sorted out, we're good.
 
> > > +
> > > +        netns = NetNS()
> > > +
> > > +        # Call packetdrill from the directory hosting the .pkt script,
> > > +        # because scripts can have relative includes.
> > > +        savedir = os.getcwd()
> > > +        os.chdir(os.path.dirname(pktfile_path))
> > > +        basename = os.path.basename(pktfile_path)
> > > +        cmd(f"packetdrill {args} {basename}", ns=netns)
> > > +        os.chdir(savedir)
> > > +
> > > +    if ipv4:
> > > +        f.__name__ = pathlib.Path(pktfile_path).stem + "-v4"
> > > +    else:
> > > +        f.__name__ = pathlib.Path(pktfile_path).stem + "-v6"
> > 
> > What about dualstack v4-over-v6 mode? Do we want to support that as
> > well?
> 
> Does it give a sufficient signal? I skipped over it on purpose. But
> can enable if that is the consensus.

Makes sense. Agreed that most of the time v4-over-v6 just exercises
the same v4 paths so it's not too much value. I'm assuming that the tests
that care about v4-over-v6 peculiarities can just
do --ip_version=ipv4-mapped-ipv6 in their .pkt file (similar to
what gtests/net/tcp/mtu_probe/basic-v6.pkt is doing)?
Jakub Kicinski Aug. 28, 2024, 6:23 p.m. UTC | #11
On Wed, 28 Aug 2024 10:20:47 -0700 Stanislav Fomichev wrote:
> > As Jakub responded.
> > 
> > Indeed importing the whole application into the kernel sources is what
> > stopped me from attempting this before. Florian's nf_conntrack charted
> > the path here.  
> 
> Thanks both. I was expecting to find some build rules :-) But as long
> as we have this story sorted out, we're good.

FWIW I lied to you, the "script" for buildings things wasn't committed
in the repo, I pushed it now:
https://github.com/linux-netdev/nipa/commit/dde0f093774a4a4c85e595b5cf44bd3e4bd434d1
Mina Almasry Aug. 28, 2024, 6:26 p.m. UTC | #12
On Tue, Aug 27, 2024 at 12:34 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> 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 implementation and reuses the
> ksft python library for its KTAP, ksft, NetNS and other such tooling.
>
> It also anticipates the large number of testcases, by creating a
> separate kselftest for each feature (directory). It does this by
> copying the template script packetdrill_ksft.py for each directory,
> and putting those in TEST_CUSTOM_PROGS so that kselftests runs each.
>
> To demonstrate the code with minimal patch size, 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.
>
> Any future imports of packetdrill tests should require no additional
> coding. Just add the tcp/$FEATURE directory with *.pkt files.
>
> Implementation notes:
> - restore alphabetical order when adding the new directory to
>   tools/testing/selftests/Makefile
> - copied *.pkt files and support verbatim from the github project,
>   except for
>     - update common/defaults.sh path (there are two paths on github)
>     - add SPDX headers
>     - remove one author statement
>     - Acknowledgment: drop an e (checkpatch)
>
> Tested:
>         make -C tools/testing/selftests/ \
>           TARGETS=net/packetdrill \
>           install INSTALL_PATH=$KSFT_INSTALL_PATH
>
>         # in virtme-ng
>         sudo ./run_kselftest.sh -c net/packetdrill
>         sudo ./run_kselftest.sh -t net/packetdrill:tcp_inq.py

I did not know about run_kselftest.sh. From a quick look, it seems to
require selftests to be installed to run them. I think nipa relies on
something slightly different, it does something like this, AFAIU this
runs an individual kselftest without requiring it to be installed
first (which is nice):

make -C tools/testing/selftests TARGETS="net"
TEST_PROGS="rxtimestamp.sh" TEST_GEN_PROGS="" run_tests

It may be worth it to check if these added tests can be ran in a similar way.

This may also solve your issue with 'intermediate output' being
printed after every test. AFAIU if the tests can be ran individually
using a command like this, then you don't need intermediate output,
maybe.
Stanislav Fomichev Aug. 28, 2024, 6:36 p.m. UTC | #13
On 08/28, Jakub Kicinski wrote:
> On Wed, 28 Aug 2024 10:20:47 -0700 Stanislav Fomichev wrote:
> > > As Jakub responded.
> > > 
> > > Indeed importing the whole application into the kernel sources is what
> > > stopped me from attempting this before. Florian's nf_conntrack charted
> > > the path here.  
> > 
> > Thanks both. I was expecting to find some build rules :-) But as long
> > as we have this story sorted out, we're good.
> 
> FWIW I lied to you, the "script" for buildings things wasn't committed
> in the repo, I pushed it now:
> https://github.com/linux-netdev/nipa/commit/dde0f093774a4a4c85e595b5cf44bd3e4bd434d1

Perfect, thanks!
Jakub Kicinski Aug. 28, 2024, 6:39 p.m. UTC | #14
On Wed, 28 Aug 2024 11:26:28 -0700 Mina Almasry wrote:
> I did not know about run_kselftest.sh. From a quick look, it seems to
> require selftests to be installed to run them. I think nipa relies on
> something slightly different, it does something like this, AFAIU this
> runs an individual kselftest without requiring it to be installed
> first (which is nice):
> 
> make -C tools/testing/selftests TARGETS="net"
> TEST_PROGS="rxtimestamp.sh" TEST_GEN_PROGS="" run_tests
> 
> It may be worth it to check if these added tests can be ran in a similar way.

They should, the ksft infra converges into the same bash script
(runner.sh) whether we enter via make or via the "installed" runner.

> This may also solve your issue with 'intermediate output' being
> printed after every test. AFAIU if the tests can be ran individually
> using a command like this, then you don't need intermediate output,
> maybe.

selftest infra tries to prefix raw output with # to make sure that
random prints don't get mixed with KTAP. IIRC there are two ways
to do that, one is a perl script and that doesn't buffer, but if
there's no perl in PATH it runs the output thru sed, and sed buffers.
Willem de Bruijn Aug. 28, 2024, 7:33 p.m. UTC | #15
Jakub Kicinski wrote:
> On Wed, 28 Aug 2024 10:03:55 -0400 Willem de Bruijn wrote:
> > > > A single script is much
> > > > simpler, optionally with nested KTAP (not supported yet by ksft). But,
> > > > I'm afraid that running time without intermediate output will be very
> > > > long when we integrate all packetdrill scripts.  
> > > 
> > > If I read correctly, this runs the scripts in the given directory 
> > > sequentially (as opposed to the default pktdrill run_all.py behavior 
> > > that uses many concurrent threads).
> > > 
> > > I guess/fear that running all the pktdrill tests in a single batch would 
> > > take quite a long time, which in turn could be not so good for CI 
> > > integration. Currently there are a couple of CI test-cases with runtime   
> > >  > 1h, but that is bad ;)  
> > 
> > Very good point, thanks! This is the third packetdrill runner that I'm
> > writing. I should know this by now.. Let me see whether I can use
> > run_all.py rather than reinvent the wheel here.
> 
> Do we have to worry about this now? If you're planning to add a runner
> specifically for packetdrill... IDK if we should. We already have a few
> runners, not to mention that run_kselftest.sh itself can run all the
> test cases in parallel in separate network namespaces!
> 
> What I was wondering is whether we can use shebang to direct the .pkt
> files to be "executed" by the python script. Alternatively we can add
> support to specifying "interpreter" for a given test in ksft infra
> (kinda like we can pass cmd line arguments to a test). Completely
> untested but it should give better idea what I mean than a description:
> 
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index 74954f6a8f94..429c279e9c6e 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -56,6 +56,7 @@ run_one()
>  	export kselftest_timeout="$kselftest_default_timeout"
>  
>  	# Safe default if tr not available
> +	kselftest_interp_ref="KSELFTEST_INTERP"
>  	kselftest_cmd_args_ref="KSELFTEST_ARGS"
>  
>  	# Optional arguments for this command, possibly defined as an
> @@ -78,6 +79,14 @@ run_one()
>  					$TR_CMD [:lower:] [:upper:])
>  		kselftest_cmd_args_ref="KSELFTEST_${BASENAME_SANITIZED}_ARGS"
>  	fi
> +	# Optional interpreter to run the test case
> +	if [ -n "$TR_CMD" ]; then
> +		SUFFIX_SANITIZED=$(echo "${BASENAME_TEST#*.}" | \
> +				       $TR_CMD -d "[:blank:][:cntrl:]" | \
> +				       $TR_CMD -c "[:alnum:]_" "_" | \
> +				       $TR_CMD [:lower:] [:upper:])
> +		kselftest_interp_ref="KSELFTEST_${SUFFIX_SANITIZED}_INTERP"
> +	fi
>  
>  	# Load per-test-directory kselftest "settings" file.
>  	settings="$BASE_DIR/$DIR/settings"
> @@ -110,8 +119,12 @@ run_one()
>  		if [ -x /usr/bin/stdbuf ]; then
>  			stdbuf="/usr/bin/stdbuf --output=L "
>  		fi
> +		eval kselftest_interp="\$${kselftest_interp_ref:-}"
> +		# Add a space at the end if interpreter is set to work in $cmd
> +		[ -n "$kselftest_interp" ] && \
> +		    kselftest_interp="$kselftest_interp "
>  		eval kselftest_cmd_args="\$${kselftest_cmd_args_ref:-}"
> -		cmd="$stdbuf ./$BASENAME_TEST $kselftest_cmd_args"
> +		cmd="$stdbuf ./$kselftest_interp$BASENAME_TEST $kselftest_cmd_args"
>  		if [ ! -x "$TEST" ]; then
>  			echo "# Warning: file $TEST is not executable"

That could work.

Is reporting one KTAP and exitcode per directory vs per packetdrill
invocation good/bad/neither?

Three other issues if this is calling packetdrill directly is
- passing the non-trivial IP specific flags
- running twice, for IPv4 and IPv6
- chdir into the directory of the pkt file

That can be addressed by instead calling a small wrapper shell script.

That would do the test_func_builder part of packetdrill_ksft.py.
But without the need to handle netns, popen/cmd, etc, and thus the
ksft dependencies.
Jakub Kicinski Aug. 28, 2024, 9 p.m. UTC | #16
On Wed, 28 Aug 2024 15:33:33 -0400 Willem de Bruijn wrote:
> That could work.
> 
> Is reporting one KTAP and exitcode per directory vs per packetdrill
> invocation good/bad/neither?

To me "neither", hopefully and unhelpfully I won't tell you anything
that's not common sense :) The balance is between:

 - having test cases which don't take too long (assuming debug kernel):
     <15min is good, 
     >1h I will start complaining,
     >1h30m is bad because we can't retry and still make the 3h
            deadline that NIPA has,
     >3h the test can't run in NIPA at all.

vs

 - flip side is having so many cases it's hard to keep track and render
   in the UI. JSON is relatively slow to process. If you have 150 cases,
   that's 300 per branch (debug and non-debug kernels), times 8
   branches a day => 2.4k results / day
   I think that's still fine-ish, but on the larger side for sure. For
   reference net and forwarding have ~100 tests each. FWIW we do have
   the ability to collect and display nested KTAP so the information is
   not lost (but it makes queries slower so we don't fetch it by default).

> Three other issues if this is calling packetdrill directly is
> - passing the non-trivial IP specific flags
> - running twice, for IPv4 and IPv6
> - chdir into the directory of the pkt file
> 
> That can be addressed by instead calling a small wrapper shell script.
> 
> That would do the test_func_builder part of packetdrill_ksft.py.
> But without the need to handle netns, popen/cmd, etc, and thus the
> ksft dependencies.

Right!
Willem de Bruijn Aug. 30, 2024, 3:20 p.m. UTC | #17
Jakub Kicinski wrote:
> On Wed, 28 Aug 2024 15:33:33 -0400 Willem de Bruijn wrote:
> > That could work.
> > 
> > Is reporting one KTAP and exitcode per directory vs per packetdrill
> > invocation good/bad/neither?
> 
> To me "neither", hopefully and unhelpfully I won't tell you anything
> that's not common sense :) The balance is between:
> 
>  - having test cases which don't take too long (assuming debug kernel):
>      <15min is good, 
>      >1h I will start complaining,
>      >1h30m is bad because we can't retry and still make the 3h
>             deadline that NIPA has,
>      >3h the test can't run in NIPA at all.
> 
> vs
> 
>  - flip side is having so many cases it's hard to keep track and render
>    in the UI. JSON is relatively slow to process. If you have 150 cases,
>    that's 300 per branch (debug and non-debug kernels), times 8
>    branches a day => 2.4k results / day
>    I think that's still fine-ish, but on the larger side for sure. For
>    reference net and forwarding have ~100 tests each. FWIW we do have
>    the ability to collect and display nested KTAP so the information is
>    not lost (but it makes queries slower so we don't fetch it by default).
> 
> > Three other issues if this is calling packetdrill directly is
> > - passing the non-trivial IP specific flags
> > - running twice, for IPv4 and IPv6
> > - chdir into the directory of the pkt file
> > 
> > That can be addressed by instead calling a small wrapper shell script.
> > 
> > That would do the test_func_builder part of packetdrill_ksft.py.
> > But without the need to handle netns, popen/cmd, etc, and thus the
> > ksft dependencies.
> 
> Right!

Implemented this, but hit a snag

Kselftest install does not preserve directories.

So all .pkt files are copied into net/packetdrill root. This is messy.
More fundamentally it breaks the includes in the files (e..g, `source
../common/defaults.sh`).

The output of having each test separate is also quite unreadable.

An alternative is to add each subdirectory as a separate TARGET.
But that seems similarly impractical. And does nothing to group
KTAP output.

In practice, most directories have a handful of .pkt scripts, and
each script is fast, so serialization is not a huge issue.

I found tools/testing/selftests/net/kselftest/ktap_helpers.sh,
which has helpful KTAP boilerplate for shell tests, including
KSFT exit codes. Am using that instead of python, to reduce the
external dependencies.
Jakub Kicinski Aug. 30, 2024, 5:33 p.m. UTC | #18
On Fri, 30 Aug 2024 11:20:05 -0400 Willem de Bruijn wrote:
> Kselftest install does not preserve directories.
> 
> So all .pkt files are copied into net/packetdrill root. This is messy.
> More fundamentally it breaks the includes in the files (e..g, `source
> ../common/defaults.sh`).

Can you show an example of exact commands and what happens?

We have directories in net/lib, and it's a target, and it works, no?
Willem de Bruijn Aug. 30, 2024, 6:47 p.m. UTC | #19
Jakub Kicinski wrote:
> On Fri, 30 Aug 2024 11:20:05 -0400 Willem de Bruijn wrote:
> > Kselftest install does not preserve directories.
> > 
> > So all .pkt files are copied into net/packetdrill root. This is messy.
> > More fundamentally it breaks the includes in the files (e..g, `source
> > ../common/defaults.sh`).
> 
> Can you show an example of exact commands and what happens?

Running directly works fine:

    $ KSELFTEST_PKT_INTERP=packetdrill_ksft.sh
    $ make -C tools/testing/selftests \
            TARGETS=net/packetdrill O=/tmp run_tests

    TAP version 13
    1..3
    # timeout set to 45
    # selftests: net/packetdrill: client.pkt
    # TAP version 13
    # 1..2
    # ok 1 ipv4
    # ok 2 ipv6
    # # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
    ok 1 selftests: net/packetdrill: client.pkt
    [..etc..]

Installing does not:

    $ make -C tools/testing/selftests/ \
          TARGETS=net/packetdrill \
          install INSTALL_PATH=$INSTALL_DIR
    $ cd $INSTALL_DIR
    $ export KSELFTEST_PKT_INTERP=packetdrill_ksft.sh
    $ ./run_kselftest.sh -c net/packetdrill

    TAP version 13
    1..3
    # timeout set to 45
    # selftests: net/packetdrill: client.pkt
    # TAP version 13
    # 1..2
    # sh: line 1: ../common/defaults.sh: No such file or directory
    # ./client.pkt: error executing init command: non-zero status 127
    # not ok 1 ipv4
    # sh: line 1: ../common/defaults.sh: No such file or directory
    # ./client.pkt: error executing init command: non-zero status 127
    # not ok 2 ipv6
    # # Totals: pass:0 fail:2 xfail:0 xpass:0 skip:0 error:0
    not ok 1 selftests: net/packetdrill: client.pkt # exit=1

Due to that relative path to defaults.sh inside the scripts.

It is arguably a bit weird that the relative path of the TEST_PROGS
differs before and after install.

> We have directories in net/lib, and it's a target, and it works, no?

net/lib is not a TARGET in tools/testing/selftests/Makefile. Its
Makefile only generates dependencies for other targets: TEST_FILES,
TEST_GEN_FILES and TEST_INCLUDES.

This issue with preserving paths until recently also existed for
helper files (TEST_FILES). TEST_INCLUDES was added expressly to
preserve those paths (commit 2a0683be5b4c).
Jakub Kicinski Aug. 30, 2024, 9:44 p.m. UTC | #20
On Fri, 30 Aug 2024 14:47:43 -0400 Willem de Bruijn wrote:
> > We have directories in net/lib, and it's a target, and it works, no?  
> 
> net/lib is not a TARGET in tools/testing/selftests/Makefile. Its
> Makefile only generates dependencies for other targets: TEST_FILES,
> TEST_GEN_FILES and TEST_INCLUDES.

Oh right, TEST_FILES vs TEST_INCLUDES :(

Looks like only x86 does some weird stuff and prepends $(OUTPUT) to all
test names. Otherwise the only TEST_NAME with a / in it is

x86_64/nx_huge_pages_test.sh

But then again maybe we should give up on the idea of using directories?
Use some separator like --, I mean:

mv packetdrill/tcp/inq/client.pkt packetdrill/tcp--inq--client.pkt

Assuming we're moving forward with the interpreter idea we don't need
directories for multi-threading, just for organization. Which perhaps
isn't worth the time investment? Given that we'd mostly interact with
these tests via UI which will flatten it all back?
Willem de Bruijn Aug. 30, 2024, 9:46 p.m. UTC | #21
Willem de Bruijn wrote:
> Jakub Kicinski wrote:
> > On Fri, 30 Aug 2024 11:20:05 -0400 Willem de Bruijn wrote:
> > > Kselftest install does not preserve directories.
> > > 
> > > So all .pkt files are copied into net/packetdrill root. This is messy.
> > > More fundamentally it breaks the includes in the files (e..g, `source
> > > ../common/defaults.sh`).
> > 
> > Can you show an example of exact commands and what happens?
> 
> Running directly works fine:
> 
>     $ KSELFTEST_PKT_INTERP=packetdrill_ksft.sh
>     $ make -C tools/testing/selftests \
>             TARGETS=net/packetdrill O=/tmp run_tests
> 
>     TAP version 13
>     1..3
>     # timeout set to 45
>     # selftests: net/packetdrill: client.pkt
>     # TAP version 13
>     # 1..2
>     # ok 1 ipv4
>     # ok 2 ipv6
>     # # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
>     ok 1 selftests: net/packetdrill: client.pkt
>     [..etc..]
> 
> Installing does not:
> 
>     $ make -C tools/testing/selftests/ \
>           TARGETS=net/packetdrill \
>           install INSTALL_PATH=$INSTALL_DIR
>     $ cd $INSTALL_DIR
>     $ export KSELFTEST_PKT_INTERP=packetdrill_ksft.sh
>     $ ./run_kselftest.sh -c net/packetdrill
> 
>     TAP version 13
>     1..3
>     # timeout set to 45
>     # selftests: net/packetdrill: client.pkt
>     # TAP version 13
>     # 1..2
>     # sh: line 1: ../common/defaults.sh: No such file or directory
>     # ./client.pkt: error executing init command: non-zero status 127
>     # not ok 1 ipv4
>     # sh: line 1: ../common/defaults.sh: No such file or directory
>     # ./client.pkt: error executing init command: non-zero status 127
>     # not ok 2 ipv6
>     # # Totals: pass:0 fail:2 xfail:0 xpass:0 skip:0 error:0
>     not ok 1 selftests: net/packetdrill: client.pkt # exit=1
> 
> Due to that relative path to defaults.sh inside the scripts.
> 
> It is arguably a bit weird that the relative path of the TEST_PROGS
> differs before and after install.

I guess what's weird is more what I'm trying to do. There has been
no need so far for progs in subdirectories from the TARGET.

One option is to just create a bunch of targets with a wildcard.
Something like below (entirely untested).

TARGETS += net/packetdrill/tcp/$(wildcard *)

> > We have directories in net/lib, and it's a target, and it works, no?
> 
> net/lib is not a TARGET in tools/testing/selftests/Makefile. Its
> Makefile only generates dependencies for other targets: TEST_FILES,
> TEST_GEN_FILES and TEST_INCLUDES.
> 
> This issue with preserving paths until recently also existed for
> helper files (TEST_FILES). TEST_INCLUDES was added expressly to
> preserve those paths (commit 2a0683be5b4c).

Another option is to dive yet deeper into kselftest infra and do the
same for TEST_PROGS, either TEST_PROGS itself or as a new
TEST_PROGS_FULLPATH or so.

It needs a change to INSTALL_RULE (maybe just passing -R to rsync),
to emit_tests to skip basename when writing kselftest-list.txt, and
probably to run_one() to break up the path and chdir. I'll see if I
can make this work without too much churn.
Willem de Bruijn Aug. 30, 2024, 9:52 p.m. UTC | #22
Jakub Kicinski wrote:
> On Fri, 30 Aug 2024 14:47:43 -0400 Willem de Bruijn wrote:
> > > We have directories in net/lib, and it's a target, and it works, no?  
> > 
> > net/lib is not a TARGET in tools/testing/selftests/Makefile. Its
> > Makefile only generates dependencies for other targets: TEST_FILES,
> > TEST_GEN_FILES and TEST_INCLUDES.
> 
> Oh right, TEST_FILES vs TEST_INCLUDES :(
> 
> Looks like only x86 does some weird stuff and prepends $(OUTPUT) to all
> test names. Otherwise the only TEST_NAME with a / in it is
> 
> x86_64/nx_huge_pages_test.sh

Oh interesting precedent. Let me take a look.

> But then again maybe we should give up on the idea of using directories?
> Use some separator like --, I mean:
> 
> mv packetdrill/tcp/inq/client.pkt packetdrill/tcp--inq--client.pkt
> 
> Assuming we're moving forward with the interpreter idea we don't need
> directories for multi-threading, just for organization. Which perhaps
> isn't worth the time investment? Given that we'd mostly interact with
> these tests via UI which will flatten it all back?

That's definitely simpler :)

I'd like to keep diffs between packetdrill scripts on github (and
Google internal, we have more) and selftests to a minimum. This is
invertible, as is rewriting source statements inside the pkt files.
But that might be more work and more maintenance in the end.
Willem de Bruijn Sept. 1, 2024, 9:15 p.m. UTC | #23
Willem de Bruijn wrote:
> Jakub Kicinski wrote:
> > On Fri, 30 Aug 2024 14:47:43 -0400 Willem de Bruijn wrote:
> > > > We have directories in net/lib, and it's a target, and it works, no?  
> > > 
> > > net/lib is not a TARGET in tools/testing/selftests/Makefile. Its
> > > Makefile only generates dependencies for other targets: TEST_FILES,
> > > TEST_GEN_FILES and TEST_INCLUDES.
> > 
> > Oh right, TEST_FILES vs TEST_INCLUDES :(
> > 
> > Looks like only x86 does some weird stuff and prepends $(OUTPUT) to all
> > test names. Otherwise the only TEST_NAME with a / in it is
> > 
> > x86_64/nx_huge_pages_test.sh
> 
> Oh interesting precedent. Let me take a look.
> 
> > But then again maybe we should give up on the idea of using directories?
> > Use some separator like --, I mean:
> > 
> > mv packetdrill/tcp/inq/client.pkt packetdrill/tcp--inq--client.pkt
> > 
> > Assuming we're moving forward with the interpreter idea we don't need
> > directories for multi-threading, just for organization. Which perhaps
> > isn't worth the time investment? Given that we'd mostly interact with
> > these tests via UI which will flatten it all back?
> 
> That's definitely simpler :)
> 
> I'd like to keep diffs between packetdrill scripts on github (and
> Google internal, we have more) and selftests to a minimum. This is
> invertible, as is rewriting source statements inside the pkt files.
> But that might be more work and more maintenance in the end.

Thanks again for the pointer and suggestion.

Changing kselftests to preserve directories turns out to be trivial.
Patch inline below.

But, existing TARGETS of course then start failing. Because they
depend on existing rsync without -R. In (at least) two ways:

amd-pstate fails because its TEST_FILES has includes from other
directories and it expects those files to land in the directory
with tests.

x86 prefixes all its output with $(OUTPUT) to form absolute paths,
which also creates absolute paths in kselftest-list.txt.

These two are examples, not necessarily the one instances of those
patterns. So switching to preserving directories for existing targets
like TEST_FILES seems intractable.

Plan B is to add a new TEST_PROGS_RECURSE, analogous to how
TEST_INCLUDES extended TEST_FILES with optional path preservation.
That is not much more complex.

---

+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -101,7 +112,7 @@ run_one()
                echo "# timeout set to $kselftest_timeout" >> "$logfile"
        fi
 
-       TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
+       TEST_HDR_MSG="selftests: $DIR: $TEST"

+++ b/tools/testing/selftests/lib.mk
@@ -150,7 +150,7 @@ clean_mods_dir:

 define INSTALL_SINGLE_RULE
        $(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH))
-       $(if $(INSTALL_LIST),rsync -a --copy-unsafe-links $(INSTALL_LIST) $(INSTALL_PATH)/)
+       $(if $(INSTALL_LIST),rsync -aR --copy-unsafe-links $(INSTALL_LIST) $(INSTALL_PATH)/)
 endef

 define INSTALL_MODS_RULE
@@ -180,8 +180,7 @@ endif
 
 emit_tests:
        for TEST in $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS); do \
-               BASENAME_TEST=`basename $$TEST`;        \
-               echo "$(COLLECTION):$$BASENAME_TEST";   \
+               echo "$(COLLECTION):$$TEST";    \
        done
Jakub Kicinski Sept. 2, 2024, 4:46 p.m. UTC | #24
On Sun, 01 Sep 2024 17:15:38 -0400 Willem de Bruijn wrote:
> Changing kselftests to preserve directories turns out to be trivial.
> Patch inline below.
> 
> But, existing TARGETS of course then start failing. Because they
> depend on existing rsync without -R. In (at least) two ways:
> 
> amd-pstate fails because its TEST_FILES has includes from other
> directories and it expects those files to land in the directory
> with tests.
> 
> x86 prefixes all its output with $(OUTPUT) to form absolute paths,
> which also creates absolute paths in kselftest-list.txt.
> 
> These two are examples, not necessarily the one instances of those
> patterns. So switching to preserving directories for existing targets
> like TEST_FILES seems intractable.

I wonder how many of the targets actually need this behavior (it's
intentionally useful to them) vs they grew to depend on it accidentally.

> Plan B is to add a new TEST_PROGS_RECURSE, analogous to how
> TEST_INCLUDES extended TEST_FILES with optional path preservation.
> That is not much more complex.

Alternative would be to allow opt-in (diff at the end), I'm personally
biased against yet another TEST_* group because it's hard enough already
to make sense of the existing ones. Maybe it's better for the test to
"declare":

PRESERVE_TEST_DIRS


---
 tools/testing/selftests/lib.mk | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index d6edcfcb5be8..7be9bd583642 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -51,6 +51,12 @@ else
 CC := $(CROSS_COMPILE)gcc
 endif # LLVM
 
+ifeq ($(PRESERVE_TEST_DIRS),)
+RSYNC_INSTALL_FLAGS=-a --copy-unsafe-links
+else
+RSYNC_INSTALL_FLAGS=-aR --copy-unsafe-links
+endif
+
 ifeq (0,$(MAKELEVEL))
     ifeq ($(OUTPUT),)
 	OUTPUT := $(shell pwd)
@@ -150,12 +156,12 @@ endif
 
 define INSTALL_SINGLE_RULE
 	$(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH))
-	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links $(INSTALL_LIST) $(INSTALL_PATH)/)
+	$(if $(INSTALL_LIST),rsync $(RSYNC_INSTALL_FLAGS) $(INSTALL_LIST) $(INSTALL_PATH)/)
 endef
 
 define INSTALL_MODS_RULE
 	$(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH)/$(INSTALL_LIST))
-	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links $(INSTALL_LIST)/*.ko $(INSTALL_PATH)/$(INSTALL_LIST))
+	$(if $(INSTALL_LIST),rsync $(RSYNC_INSTALL_FLAGS) $(INSTALL_LIST)/*.ko $(INSTALL_PATH)/$(INSTALL_LIST))
 endef
 
 define INSTALL_RULE
@@ -179,10 +185,17 @@ else
 endif
 
 emit_tests:
+ifeq ($(PRESERVE_TEST_DIRS),)
 	for TEST in $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS); do \
 		BASENAME_TEST=`basename $$TEST`;	\
 		echo "$(COLLECTION):$$BASENAME_TEST";	\
 	done
+else
+	for TEST in $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS); do \
+		BASENAME_TEST=$$TEST;	\
+		echo "$(COLLECTION):$$TEST";	\
+	done
+endif
 
 # define if isn't already. It is undefined in make O= case.
 ifeq ($(RM),)
Jakub Kicinski Sept. 2, 2024, 4:56 p.m. UTC | #25
On Mon, 2 Sep 2024 09:46:12 -0700 Jakub Kicinski wrote:
> PRESERVE_TEST_DIRS

This was meant to be:

PRESERVE_TEST_DIRS := 1

Testing this more, looks like rsync -aR breaks networking tests, too.
The net/lib target, specifically, is no longer able to copy out 
the files outside of tools/testing/selftests (the YAML specs which
live in Documentation/).

So unless we can pass some magic flag to rsync to skip leading ../
we'll be stuck in supporting both behaviors forever. In which case
maybe TEST_PROGS_RECURSE is indeed better. I was hoping the
PRESERVE_TEST_DIRS flag can spread and once it reaches all targets
we can make delete the old behavior. If it can't happen its no better
than TEST_PROGS_RECURSE... sorry for the noise.
Willem de Bruijn Sept. 2, 2024, 8:50 p.m. UTC | #26
Jakub Kicinski wrote:
> On Mon, 2 Sep 2024 09:46:12 -0700 Jakub Kicinski wrote:
> > PRESERVE_TEST_DIRS
> 
> This was meant to be:
> 
> PRESERVE_TEST_DIRS := 1

Oh I like this.

> Testing this more, looks like rsync -aR breaks networking tests, too.
> The net/lib target, specifically, is no longer able to copy out 
> the files outside of tools/testing/selftests (the YAML specs which
> live in Documentation/).
> 
> So unless we can pass some magic flag to rsync to skip leading ../
> we'll be stuck in supporting both behaviors forever. In which case
> maybe TEST_PROGS_RECURSE is indeed better. I was hoping the
> PRESERVE_TEST_DIRS flag can spread and once it reaches all targets
> we can make delete the old behavior. If it can't happen its no better
> than TEST_PROGS_RECURSE... sorry for the noise.

Oh right :/ 

A lot of TEST(_GEN|_CUSTOM)_PROGS(_EXTENDED) could probably be cleaned
up.

One part that I haven't looked at closely yet is x86's Makefile
prefixing everything with $(OUTPUT). That seems something that should
be consistent across target Makefiles if it exists at all, but
clearly isn't. Maybe it was when it was introduced in commit
a8ba798bc8ec.

So, we could opt out only TEST_FILES. But that still leaves a
branch there indefinitely and makes the option harder to understand.
Sounds like TEST_PROGS_RECURSE is simpler then, indeed.


Separate from the above, runner.sh also needs a few changes. This
part is mostly cosmetic, but there will be duplicate basenames,
for common terms such as client.pkt
 
-       TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
+       TEST_HDR_MSG="selftests: $DIR: $TEST"

run_in_netns and run_many need similar fixes to avoid name collisions.


Btw, for KSELFTEST_PKT_INTERP it probably makes sense to only try this
if the script is non-executable, similar to the existing opt-out that
looks at the shebang.
Willem de Bruijn Sept. 5, 2024, 3:27 a.m. UTC | #27
Jakub Kicinski wrote:
> On Fri, 30 Aug 2024 14:47:43 -0400 Willem de Bruijn wrote:
> > > We have directories in net/lib, and it's a target, and it works, no?  
> > 
> > net/lib is not a TARGET in tools/testing/selftests/Makefile. Its
> > Makefile only generates dependencies for other targets: TEST_FILES,
> > TEST_GEN_FILES and TEST_INCLUDES.
> 
> Oh right, TEST_FILES vs TEST_INCLUDES :(
> 
> Looks like only x86 does some weird stuff and prepends $(OUTPUT) to all
> test names. Otherwise the only TEST_NAME with a / in it is
> 
> x86_64/nx_huge_pages_test.sh
> 
> But then again maybe we should give up on the idea of using directories?
> Use some separator like --, I mean:
> 
> mv packetdrill/tcp/inq/client.pkt packetdrill/tcp--inq--client.pkt
> 
> Assuming we're moving forward with the interpreter idea we don't need
> directories for multi-threading, just for organization. Which perhaps
> isn't worth the time investment? Given that we'd mostly interact with
> these tests via UI which will flatten it all back?

Okay, went with the simplest approach for v1: indeed just flattening
the space like this.

Only after implementing TEST_PROGS_RECURSE and PRESERVE_TEST_DIRS.
Both make kselftest more complex and in subtle ways that could break
existing tests.

Also simplified the interpreter mechanism. Instead of requiring an
environment variable, look for a specific executable in the TARGET
dir. This will make the test just work when someone does a manual run.

All is tentative and up for revision, of course.
diff mbox series

Patch

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index a5f1c0c27dff9..f03d6fee7ac54 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
@@ -122,7 +123,7 @@  TARGETS_HOTPLUG = cpu-hotplug
 TARGETS_HOTPLUG += memory-hotplug
 
 # Networking tests want the net/lib target, include it automatically
-ifneq ($(filter net drivers/net drivers/net/hw,$(TARGETS)),)
+ifneq ($(filter net net/packetdrill drivers/net drivers/net/hw,$(TARGETS)),)
 ifeq ($(filter net/lib,$(TARGETS)),)
 	INSTALL_DEP_TARGETS := net/lib
 endif
diff --git a/tools/testing/selftests/net/packetdrill/.gitignore b/tools/testing/selftests/net/packetdrill/.gitignore
new file mode 100644
index 0000000000000..a40f1a600eb94
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/.gitignore
@@ -0,0 +1 @@ 
+tcp*sh
diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile
new file mode 100644
index 0000000000000..d94c51098d1f0
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/Makefile
@@ -0,0 +1,28 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+# KSFT includes
+TEST_INCLUDES := $(wildcard lib/py/*.py ../lib/py/*.py)
+
+# Packetdrill support file(s)
+TEST_INCLUDES += tcp/common/defaults.sh
+TEST_INCLUDES += tcp/common/set_sysctls.py
+
+# Packetdrill scripts: all .pkt in subdirectories
+TEST_INCLUDES += $(wildcard tcp/**/*.pkt)
+
+# Create a separate ksft test for each subdirectory
+# Running all packetdrill tests in one go will take too long
+#
+# For each tcp/$subdir, create a test script tcp_$subdir.py
+# Exclude tcp/common, which is a helper directory
+TEST_DIRS := $(wildcard tcp/*)
+TEST_DIRS := $(filter-out tcp/common, $(TEST_DIRS))
+TEST_CUSTOM_PROGS := $(foreach dir,$(TEST_DIRS),$(subst /,_,$(dir)).py)
+
+$(TEST_CUSTOM_PROGS) : packetdrill_ksft.py
+	cp $< $@
+
+# Needed to generate all TEST_CUSTOM_PROGS
+all: $(TEST_CUSTOM_PROGS)
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/net/packetdrill/lib/py/__init__.py b/tools/testing/selftests/net/packetdrill/lib/py/__init__.py
new file mode 100644
index 0000000000000..51bb6dda43d65
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/lib/py/__init__.py
@@ -0,0 +1,15 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+import pathlib
+import sys
+
+KSFT_DIR = (pathlib.Path(__file__).parent / "../../../..").resolve()
+
+try:
+    sys.path.append(KSFT_DIR.as_posix())
+    from net.lib.py import *
+except ModuleNotFoundError as e:
+    ksft_pr("Failed importing `net` library from kernel sources")
+    ksft_pr(str(e))
+    ktap_result(True, comment="SKIP")
+    sys.exit(4)
diff --git a/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py b/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
new file mode 100755
index 0000000000000..62572a5b8331c
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/packetdrill_ksft.py
@@ -0,0 +1,90 @@ 
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""Run packetdrill tests in the ksft harness.
+
+   Run all packetdrill tests in a subdirectory.
+   Detect the relevant subdirectory from this script name.
+   (Because the script cannot be given arguments.)
+
+   Run each test, for both IPv4 and IPv6.
+   Return a separate ksft result for each test case.
+"""
+
+import glob
+import os
+import pathlib
+import shutil
+
+from lib.py import cmd, ksft_exit, ksft_run, KsftSkipEx, NetNS
+
+
+def test_func_builder(pktfile_path, ipv4):
+    """Create a function that can be passed to ksft_run."""
+
+    def f():
+        if 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 "
+                    )
+        else:
+            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 not shutil.which("packetdrill"):
+            raise KsftSkipEx("Cannot find packetdrill")
+
+        netns = NetNS()
+
+        # Call packetdrill from the directory hosting the .pkt script,
+        # because scripts can have relative includes.
+        savedir = os.getcwd()
+        os.chdir(os.path.dirname(pktfile_path))
+        basename = os.path.basename(pktfile_path)
+        cmd(f"packetdrill {args} {basename}", ns=netns)
+        os.chdir(savedir)
+
+    if ipv4:
+        f.__name__ = pathlib.Path(pktfile_path).stem + "-v4"
+    else:
+        f.__name__ = pathlib.Path(pktfile_path).stem + "-v6"
+
+    return f
+
+
+def scriptname_to_testdir(filepath):
+    """Extract the directory to run from this filename."""
+
+    suffix = ".sh"
+
+    subdir = os.path.basename(filepath)
+    subdir = subdir[:-len(suffix)]
+    subdir = subdir.replace("_", "/")
+    return subdir
+
+
+def main() -> None:
+    subdir = scriptname_to_testdir(__file__)
+    files = glob.glob(f"{subdir}/**/*.pkt", recursive=True)
+
+    cases = []
+    for file in files:
+        for ipv4 in [True, False]:
+            cases.append(test_func_builder(file, ipv4=ipv4))
+
+    ksft_run(cases=cases)
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh b/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh
new file mode 100755
index 0000000000000..1095a7b22f44d
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/tcp/common/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/tcp/common/set_sysctls.py b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
new file mode 100755
index 0000000000000..5ddf456ae973a
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
@@ -0,0 +1,38 @@ 
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""Sets sysctl values and writes a file that restores them.
+
+The arguments are of the form "<proc-file>=<val>" separated by spaces.
+The program first reads the current value of the proc-file and creates
+a shell script named "/tmp/sysctl_restore_${PACKETDRILL_PID}.sh" which
+restores the values when executed. It then sets the new values.
+
+PACKETDRILL_PID is set by packetdrill to the pid of itself, so a .pkt
+file could restore sysctls by running `/tmp/sysctl_restore_${PPID}.sh`
+at the end.
+"""
+
+import os
+import subprocess
+import sys
+
+filename = '/tmp/sysctl_restore_%s.sh' % os.environ['PACKETDRILL_PID']
+
+# Open file for restoring sysctl values
+restore_file = open(filename, 'w')
+print('#!/bin/bash', file=restore_file)
+
+for a in sys.argv[1:]:
+  sysctl = a.split('=')
+  # sysctl[0] contains the proc-file name, sysctl[1] the new value
+
+  # read current value and add restore command to file
+  cur_val = subprocess.check_output(['cat', sysctl[0]], universal_newlines=True)
+  print('echo "%s" > %s' % (cur_val.strip(), sysctl[0]), file=restore_file)
+
+  # set new value
+  cmd = 'echo "%s" > %s' % (sysctl[1], sysctl[0])
+  os.system(cmd)
+
+os.system('chmod u+x %s' % filename)
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..8cc7798c7808f
--- /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.
+`../common/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..fd78609087b91
--- /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.
+`../common/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..42b712e14e562
--- /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.
+
+`../common/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