Received-SPF: pass (google.com: domain of linux-kernel+bounces-3252-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 18 Dec 2023 17:36:18 +0800
From: Hangbin Liu <liuhangbin@gmail.com>
To: Dmitry Safonov <dima@arista.com>
Cc: Shuah Khan <shuah@kernel.org>, David Ahern <dsahern@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Salam Noureddine <noureddine@arista.com>,
	Bob Gilligan <gilligan@arista.com>, linux-kernel@vger.kernel.org,
	linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
	Dmitry Safonov <0x7f454c46@gmail.com>
Subject: Re: [PATCH 00/12] selftests/net: Add TCP-AO tests
Message-ID: <ZYASkk3sC6pDAVs1@Laptop-X1>
References: <20231215-tcp-ao-selftests-v1-0-f6c08180b985@arista.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231215-tcp-ao-selftests-v1-0-f6c08180b985@arista.com>

Hi Dmitry,

I just found the patch set has been merged. Sorry for the noise of
tested-by replies. Please feel free to ignore the question for
unsigned-md5_ipv6. The test passed with 6.7.0-rc5.

Thanks
Hangbin

On Fri, Dec 15, 2023 at 02:36:14AM +0000, Dmitry Safonov wrote:
> Hi,
> 
> An essential part of any big kernel submissions is selftests.
> At the beginning of TCP-AO project, I made patches to fcnal-test.sh
> and nettest.c to have the benefits of easy refactoring, early noticing
> breakages, putting a moat around the code, documenting
> and designing uAPI.
> 
> While tests based on fcnal-test.sh/nettest.c provided initial testing*
> and were very easy to add, the pile of TCP-AO quickly grew out of
> one-binary + shell-script testing.
> 
> The design of the TCP-AO testing is a bit different than one-big
> selftest binary as I did previously in net/ipsec.c. I found it
> beneficial to avoid implementing a tests runner/scheduler and delegate
> it to the user or Makefile. The approach is very influenced
> by CRIU/ZDTM testing[1]: it provides a static library with helper
> functions and selftest binaries that create specific scenarios.
> I also tried to utilize kselftest.h.
> 
> test_init() function does all needed preparations. To not leave
> any traces after a selftest exists, it creates a network namespace
> and if the test wants to establish a TCP connection, a child netns.
> The parent and child netns have veth pair with proper ip addresses
> and routes set up. Both peers, the client and server are different
> pthreads. The treading model was chosen over forking mostly by easiness
> of cleanup on a failure: no need to search for children, handle SIGCHLD,
> make sure not to wait for a dead peer to perform anything, etc.
> Any thread that does exit() naturally kills the tests, sweet!
> The selftests are compiled currently in two variants: ipv4 and ipv6.
> Ipv4-mapped-ipv6 addresses might be a third variant to add, but it's not
> there in this version. As pretty much all tests are shared between two
> address families, most of the code can be shared, too. To differ in code
> what kind of test is running, Makefile supplies -DIPV6_TEST to compiler
> and ifdeffery in tests can do things that have to be different between
> address families. This is similar to TARGETS_C_BOTHBITS in x86 selftests
> and also to tests code sharing in CRIU/ZDTM.
> 
> The total number of tests is 832.
> From them rst_ipv{4,6} has currently one flaky subtest, that may fail:
> > not ok 9 client connection was not reset: 0
> I'll investigate what happens there. Also, unsigned-md5_ipv{4,6}
> are flaky because of netns counter checks: it doesn't expect that
> there may be retransmitted TCP segments from a previous sub-selftest.
> That will be fixed. Besides, key-management_ipv{4,6} has 3 sub-tests
> passing with XFAIL:
> > ok 15 # XFAIL listen() after current/rnext keys set: the socket has current/rnext keys: 100:200
> > ok 16 # XFAIL listen socket, delete current key from before listen(): failed to delete the key 100:100 -16
> > ok 17 # XFAIL listen socket, delete rnext key from before listen(): failed to delete the key 200:200 -16
> ...
> > # Totals: pass:117 fail:0 xfail:3 xpass:0 skip:0 error:0
> Those need some more kernel work to pass instead of xfail.
> 
> The overview of selftests (see the diffstat at the bottom):
> ├── lib
> │   ├── aolib.h
> │   │   The header for all selftests to include.
> │   ├── kconfig.c
> │   │   Kernel kconfig detector to SKIP tests that depend on something.
> │   ├── netlink.c
> │   │   Netlink helper to add/modify/delete VETH/IPs/routes/VRFs
> │   │   I considered just using libmnl, but this is around 400 lines
> │   │   and avoids selftests dependency on out-of-tree sources/packets.
> │   ├── proc.c
> │   │   SNMP/netstat procfs parser and the counters comparator.
> │   ├── repair.c
> │   │   Heavily influenced by libsoccr and reduced to minimum TCP
> │   │   socket checkpoint/repair. Shouldn't be used out of selftests,
> │   │   though.
> │   ├── setup.c
> │   │   All the needed netns/veth/ips/etc preparations for test init.
> │   ├── sock.c
> │   │   Socket helpers: {s,g}etsockopt()s/connect()/listen()/etc.
> │   └── utils.c
> │       Random stuff (a pun intended).
> ├── bench-lookups.c
> │   The only benchmark in selftests currently: checks how well TCP-AO
> │   setsockopt()s perform, depending on the amount of keys on a socket.
> ├── connect.c
> │   Trivial sample, can be used as a boilerplate to write a new test.
> ├── connect-deny.c
> │   More-or-less what could be expected for TCP-AO in fcnal-test.sh
> ├── icmps-accept.c -> icmps-discard.c
> ├── icmps-discard.c
> │   Verifies RFC5925 (7.8) by checking that TCP-AO connection can be
> │   broken if ICMPs are accepted and survives when ::accept_icmps = 0
> ├── key-management.c
> │   Key manipulations, rotations between randomized hashing algorithms
> │   and counter checks for those scenarios.
> ├── restore.c
> │   TCP_AO_REPAIR: verifies that a socket can be re-created without
> │   TCP-AO connection being interrupted.
> ├── rst.c
> │   As RST segments are signed on a separate code-path in kernel,
> │   verifies passive/active TCP send_reset().
> ├── self-connect.c
> │   Verifies that TCP self-connect and also simultaneous open work.
> ├── seq-ext.c
> │   Utilizes TCP_AO_REPAIR to check that on SEQ roll-over SNE
> │   increment is performed and segments with different SNEs fail to
> │   pass verification.
> ├── setsockopt-closed.c
> │   Checks that {s,g}etsockopt()s are extendable syscalls and common
> │   error-paths for them.
> └── unsigned-md5.c
>     Checks listen() socket for (non-)matching peers with: AO/MD5/none
>     keys. As well as their interaction with VRFs and AO_REQUIRED flag.
> 
> There are certainly more test scenarios that can be added, but even so,
> I'm pretty happy that this much of TCP-AO functionality and uAPIs got
> covered. These selftests were iteratively developed by me during TCP-AO
> kernel upstreaming and the resulting kernel patches would have been
> worse without having these tests. They provided the user-side
> perspective but also allowed safer refactoring with less possibility
> of introducing a regression. Now it's time to use them to dig
> a moat around the TCP-AO code!
> 
> There are also people from other network companies that work on TCP-AO
> (+testing), so sharing these selftests will allow them to contribute
> and may benefit from their efforts.
> 
> The following changes since commit c7402612e2e61b76177f22e6e7f705adcbecc6fe:
> 
>   Merge tag 'net-6.7-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2023-12-14 13:11:49 -0800)
> 
> are available in the Git repository at:
> 
>   git@github.com:0x7f454c46/linux.git tcp-ao-selftests-v1
> 
> for you to fetch changes up to 85dc9bc676985d81f9043fd9c3a506f30851597b:
> 
>   selftests/net: Add TCP-AO key-management test (2023-12-15 00:44:49 +0000)
> 
> ----------------------------------------------------------------
> 
> * Planning to submit basic TCP-AO tests to fcnal-test.sh/nettest.c
>   separately.
> 
> [1]: https://github.com/checkpoint-restore/criu/tree/criu-dev/test/zdtm/static
> 
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
> Dmitry Safonov (12):
>       selftests/net: Add TCP-AO library
>       selftests/net: Verify that TCP-AO complies with ignoring ICMPs
>       selftests/net: Add TCP-AO ICMPs accept test
>       selftests/net: Add a test for TCP-AO keys matching
>       selftests/net: Add test for TCP-AO add setsockopt() command
>       selftests/net: Add TCP-AO + TCP-MD5 + no sign listen socket tests
>       selftests/net: Add test/benchmark for removing MKTs
>       selftests/net: Add TCP_REPAIR TCP-AO tests
>       selftests/net: Add SEQ number extension test
>       selftests/net: Add TCP-AO RST test
>       selftests/net: Add TCP-AO selfconnect/simultaneous connect test
>       selftests/net: Add TCP-AO key-management test
> 
>  tools/testing/selftests/Makefile                   |    1 +
>  tools/testing/selftests/net/tcp_ao/.gitignore      |    2 +
>  tools/testing/selftests/net/tcp_ao/Makefile        |   59 +
>  tools/testing/selftests/net/tcp_ao/bench-lookups.c |  358 ++++++
>  tools/testing/selftests/net/tcp_ao/connect-deny.c  |  264 +++++
>  tools/testing/selftests/net/tcp_ao/connect.c       |   90 ++
>  tools/testing/selftests/net/tcp_ao/icmps-accept.c  |    1 +
>  tools/testing/selftests/net/tcp_ao/icmps-discard.c |  449 ++++++++
>  .../testing/selftests/net/tcp_ao/key-management.c  | 1180 ++++++++++++++++++++
>  tools/testing/selftests/net/tcp_ao/lib/aolib.h     |  605 ++++++++++
>  tools/testing/selftests/net/tcp_ao/lib/kconfig.c   |  148 +++
>  tools/testing/selftests/net/tcp_ao/lib/netlink.c   |  415 +++++++
>  tools/testing/selftests/net/tcp_ao/lib/proc.c      |  273 +++++
>  tools/testing/selftests/net/tcp_ao/lib/repair.c    |  254 +++++
>  tools/testing/selftests/net/tcp_ao/lib/setup.c     |  342 ++++++
>  tools/testing/selftests/net/tcp_ao/lib/sock.c      |  592 ++++++++++
>  tools/testing/selftests/net/tcp_ao/lib/utils.c     |   30 +
>  tools/testing/selftests/net/tcp_ao/restore.c       |  236 ++++
>  tools/testing/selftests/net/tcp_ao/rst.c           |  415 +++++++
>  tools/testing/selftests/net/tcp_ao/self-connect.c  |  197 ++++
>  tools/testing/selftests/net/tcp_ao/seq-ext.c       |  245 ++++
>  .../selftests/net/tcp_ao/setsockopt-closed.c       |  835 ++++++++++++++
>  tools/testing/selftests/net/tcp_ao/unsigned-md5.c  |  742 ++++++++++++
>  23 files changed, 7733 insertions(+)
> ---
> base-commit: c7402612e2e61b76177f22e6e7f705adcbecc6fe
> change-id: 20231213-tcp-ao-selftests-d0f323006667
> 
> Best regards,
> -- 
> Dmitry Safonov <dima@arista.com>
>