Two typo fixes, noticed by Mohammad's review. And a fix for an issue that got uncovered.
Signed-off-by: Dmitry Safonov dima@arista.com --- Dmitry Safonov (2): selftests/net: Rectify key counters checks selftests/net: Clean-up double assignment
Mohammad Nassiri (1): selftests/net: Argument value mismatch when calling verify_counters()
.../testing/selftests/net/tcp_ao/key-management.c | 46 ++++++++++++---------- tools/testing/selftests/net/tcp_ao/lib/sock.c | 1 - 2 files changed, 26 insertions(+), 21 deletions(-) --- base-commit: 296455ade1fdcf5f8f8c033201633b60946c589a change-id: 20240118-tcp-ao-test-key-mgmt-bb51a5fe15a2
Best regards,
From: Mohammad Nassiri mnassiri@ciena.com
The end_server() function only operates in the server thread and always takes an accept socket instead of a listen socket as its input argument. To align with this, invert the boolean values used when calling verify_counters() within the end_server() function.
Fixes: 3c3ead555648 ("selftests/net: Add TCP-AO key-management test") Signed-off-by: Mohammad Nassiri mnassiri@ciena.com Link: https://lore.kernel.org/all/934627c5-eebb-4626-be23-cfb134c01d1a@arista.com/ [amended 'Fixes' tag and carried-over to lkml] Signed-off-by: Dmitry Safonov dima@arista.com --- tools/testing/selftests/net/tcp_ao/key-management.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/tcp_ao/key-management.c b/tools/testing/selftests/net/tcp_ao/key-management.c index c48b4970ca17..f6a9395e3cd7 100644 --- a/tools/testing/selftests/net/tcp_ao/key-management.c +++ b/tools/testing/selftests/net/tcp_ao/key-management.c @@ -843,7 +843,7 @@ static void end_server(const char *tst_name, int sk, synchronize_threads(); /* 4: verified => closed */ close(sk);
- verify_counters(tst_name, true, false, begin, &end); + verify_counters(tst_name, false, true, begin, &end); synchronize_threads(); /* 5: counters */ }
On Thu, Jan 18, 2024 at 02:51:34AM +0000, Dmitry Safonov wrote:
Hi Dmitry,
I see that this is correct, but I am wondering if it fixes a user-visible bug. If so, could some text about that go here?
...
Hi Simon,
On 1/19/24 16:26, Simon Horman wrote:
Yep. The test as the result doesn't correctly check tcp counters for non-symmetrical scenario, where peer-A uses a key <a:a2> to send data, but peer-B uses <b:b2> key to send its data. So, in simple terms, different keys for TX and RX on peers.
I'll send v2 with an improved patch message.
Thanks, Dmitry
As the names of (struct test_key) members didn't reflect whether the key was used for TX or RX, the verification for the counters was done incorrectly for asymmetrical selftests.
Rename these with _tx appendix and fix checks in verify_counters(). While at it, as the checks are now correct, introduce skip_counters_checks, which is intended for tests where it's expected that a key that was set with setsockopt(sk, IPPROTO_TCP, TCP_AO_INFO, ...) might had no chance of getting used on the wire.
Fixes the following failures, exposed by the previous commit:
Cc: Mohammad Nassiri mnassiri@ciena.com Fixes: 3c3ead555648 ("selftests/net: Add TCP-AO key-management test") Signed-off-by: Dmitry Safonov dima@arista.com --- .../testing/selftests/net/tcp_ao/key-management.c | 44 ++++++++++++---------- 1 file changed, 25 insertions(+), 19 deletions(-)
diff --git a/tools/testing/selftests/net/tcp_ao/key-management.c b/tools/testing/selftests/net/tcp_ao/key-management.c index f6a9395e3cd7..24e62120b792 100644 --- a/tools/testing/selftests/net/tcp_ao/key-management.c +++ b/tools/testing/selftests/net/tcp_ao/key-management.c @@ -417,9 +417,9 @@ struct test_key { matches_vrf : 1, is_current : 1, is_rnext : 1, - used_on_handshake : 1, - used_after_accept : 1, - used_on_client : 1; + used_on_server_tx : 1, + used_on_client_tx : 1, + skip_counters_checks : 1; };
struct key_collection { @@ -609,16 +609,14 @@ static int key_collection_socket(bool server, unsigned int port) addr = &this_ip_dest; sndid = key->client_keyid; rcvid = key->server_keyid; - set_current = key->is_current; - set_rnext = key->is_rnext; + key->used_on_client_tx = set_current = key->is_current; + key->used_on_server_tx = set_rnext = key->is_rnext; }
if (test_add_key_cr(sk, key->password, key->len, *addr, vrf, sndid, rcvid, key->maclen, key->alg, set_current, set_rnext)) test_key_error("setsockopt(TCP_AO_ADD_KEY)", key); - if (set_current || set_rnext) - key->used_on_handshake = 1; #ifdef DEBUG test_print("%s [%u/%u] key: { %s, %u:%u, %u, %u:%u:%u:%u (%u)}", server ? "server" : "client", i, collection.nr_keys, @@ -640,22 +638,22 @@ static void verify_counters(const char *tst_name, bool is_listen_sk, bool server for (i = 0; i < collection.nr_keys; i++) { struct test_key *key = &collection.keys[i]; uint8_t sndid, rcvid; - bool was_used; + bool rx_cnt_expected;
+ if (key->skip_counters_checks) + continue; if (server) { sndid = key->server_keyid; rcvid = key->client_keyid; - if (is_listen_sk) - was_used = key->used_on_handshake; - else - was_used = key->used_after_accept; + rx_cnt_expected = key->used_on_client_tx; } else { sndid = key->client_keyid; rcvid = key->server_keyid; - was_used = key->used_on_client; + rx_cnt_expected = key->used_on_server_tx; }
- test_tcp_ao_key_counters_cmp(tst_name, a, b, was_used, + test_tcp_ao_key_counters_cmp(tst_name, a, b, + rx_cnt_expected ? TEST_CNT_KEY_GOOD : 0, sndid, rcvid); } test_tcp_ao_counters_free(a); @@ -916,9 +914,8 @@ static int run_client(const char *tst_name, unsigned int port, current_index = nr_keys - 1; if (rnext_index < 0) rnext_index = nr_keys - 1; - collection.keys[current_index].used_on_handshake = 1; - collection.keys[rnext_index].used_after_accept = 1; - collection.keys[rnext_index].used_on_client = 1; + collection.keys[current_index].used_on_client_tx = 1; + collection.keys[rnext_index].used_on_server_tx = 1;
synchronize_threads(); /* 3: accepted => send data */ if (test_client_verify(sk, msg_sz, msg_nr, TEST_TIMEOUT_SEC)) { @@ -1059,7 +1056,16 @@ static void check_current_back(const char *tst_name, unsigned int port, test_error("Can't change the current key"); if (test_client_verify(sk, msg_len, nr_packets, TEST_TIMEOUT_SEC)) test_fail("verify failed"); - collection.keys[rotate_to_index].used_after_accept = 1; + /* There is a race here: between setting the current_key with + * setsockopt(TCP_AO_INFO) and starting to send some data - there + * might have been a segment received with the desired + * RNext_key set. In turn that would mean that the first outgoing + * segment will have the desired current_key (flipped back). + * Which is what the user/test wants. As it's racy, skip checking + * the counters, yet check what are the resulting current/rnext + * keys on both sides. + */ + collection.keys[rotate_to_index].skip_counters_checks = 1;
end_client(tst_name, sk, nr_keys, current_index, rnext_index, &tmp); } @@ -1089,7 +1095,7 @@ static void roll_over_keys(const char *tst_name, unsigned int port, } verify_current_rnext(tst_name, sk, -1, collection.keys[i].server_keyid); - collection.keys[i].used_on_client = 1; + collection.keys[i].used_on_server_tx = 1; synchronize_threads(); /* verify current/rnext */ } end_client(tst_name, sk, nr_keys, current_index, rnext_index, &tmp);
Yeah, copy'n'paste typo.
Fixes: 3c3ead555648 ("selftests/net: Add TCP-AO key-management test") Reported-by: Nassiri, Mohammad mnassiri@ciena.com Closes: https://lore.kernel.org/all/DM6PR04MB4202BC58A9FD5BDD24A16E8EC56F2@DM6PR04MB... Signed-off-by: Dmitry Safonov dima@arista.com --- tools/testing/selftests/net/tcp_ao/lib/sock.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/net/tcp_ao/lib/sock.c b/tools/testing/selftests/net/tcp_ao/lib/sock.c index c75d82885a2e..923a9bb4f1ca 100644 --- a/tools/testing/selftests/net/tcp_ao/lib/sock.c +++ b/tools/testing/selftests/net/tcp_ao/lib/sock.c @@ -377,7 +377,6 @@ int test_get_tcp_ao_counters(int sk, struct tcp_ao_counters *out)
key_dump[0].nkeys = nr_keys; key_dump[0].get_all = 1; - key_dump[0].get_all = 1; err = getsockopt(sk, IPPROTO_TCP, TCP_AO_GET_KEYS, key_dump, &key_dump_sz); if (err) {
On Thu, 18 Jan 2024 02:51:33 +0000 Dmitry Safonov wrote:
Two typo fixes, noticed by Mohammad's review. And a fix for an issue that got uncovered.
Somewhat unrelated to these fixes but related to the tcp_ao selftests in general - could you please also add a config file so that it's easy to build a minimal kernel for running the tests?
Something like:
make defconfig make kvm_guest.config make tools/testing/selftests/net/tcp_ao/config
should give us a suitable config. Differently put it'd be great to have a config we can pass to vmtest or virtme-ng and run the tests.
On 1/18/24 16:51, Jakub Kicinski wrote:
Yep, sounds good to me. I'll take as a base tools/testing/selftests/net/config and add any needed config options on the top.
should give us a suitable config. Differently put it'd be great to have a config we can pass to vmtest or virtme-ng and run the tests.
Will check that it works with them.
Thanks, Dmitry
On Thu, 18 Jan 2024 17:04:25 +0000 Dmitry Safonov wrote:
You probably want something smaller to be honest. tools/testing/selftests/net/config has a lot of stuff in it and it's actually missing a lot more. I'm working thru adding the missing options to tools/testing/selftests/net/config right now so far I got:
# tun / tap +CONFIG_TUN=y +CONFIG_MACVLAN=y +CONFIG_MACVTAP=y +CONFIG_NET_SCH_FQ_CODEL=m +# l2tp +CONFIG_L2TP=m +CONFIG_L2TP_V3=y +CONFIG_L2TP_IP=m +CONFIG_L2TP_ETH=m +# sctp-vrf (need SCTP_DIAG to appear) +CONFIG_INET_DIAG=y +# txtimestamp +CONFIG_NET_CLS_U32=m +# test-vxlan-mdb-sh etc. +CONFIG_BRIDGE_VLAN_FILTERING=y +# gre_gso.sh etc. +CONFIG_NET_IPGRE_DEMUX=m +CONFIG_IP_GRE=m +CONFIG_IPV6_GRE=m +# ./srv6_end_dt*_l3vpn_test.sh +CONFIG_IPV6_SEG6_LWTUNNEL=y +# local port something.. +CONFIG_MPTCP=y +# fib_test.sh +CONFIG_NET_CLS_BASIC=m
On Fri, 19 Jan 2024 18:39:14 +0000 Dmitry Safonov wrote:
Hi Dmitry!
I put TCP_AO and VETH in the config and the tests seem to fail with
selftests: net/tcp_ao: rst_ipv4 not ok 1 # error 834[lib/kconfig.c:143] Failed to initialize kconfig 2: No such file or directory # Planned tests != run tests (0 != 1) # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:0 error:1
The script does:
target=net/tcp_ao make mrproper
vng -v -b -f tools/testing/selftests/$target # build the scripts make headers make -C tools/testing/selftests/$target
vng -v -r arch/x86/boot/bzImage --user root # inside the VM make -C tools/testing/selftests TARGETS=$target run_tests
Hi Jakub,
On 1/24/24 15:12, Jakub Kicinski wrote:
Thanks for wiring it up and for https://netdev.bots.linux.dev/status.html%21
Hehe, yeah I wanted to detect kernels with !CONFIG_TCP_AO, to SKIP the test, rather than FAIL it, which this lib/kconfig.c does. But from a glance, I think it's failing in your run because there are checks with and without TCP_AO, but I didn't think of checking for the hashing algorithms support.
I think what happens is has_tcp_ao(): : strcpy(tmp.alg_name, "hmac(sha1)"); ... : if (setsockopt(sk, IPPROTO_TCP, TCP_AO_ADD_KEY, &tmp, sizeof(tmp)) < 0)
Could you check that what I suppose is failing, is actually failing? [dima@Mindolluin linux-master]$ grep -e '<CONFIG_CRYPTO_SHA1>' -e '<CONFIG_CRYPTO_HMAC>' .config CONFIG_CRYPTO_HMAC=y CONFIG_CRYPTO_SHA1=y
If that's the case, I'll add the detection for hashing algorithms to lib/kconfig.c (together with a patch for tools/testing/selftests/net/config). And also heads up for key-management.c - that tries a bunch of hashing algorithms to check that the work and that the key rotation between different algorithms works:
: const char *test_algos[] = { : "cmac(aes128)", : "hmac(sha1)", "hmac(sha512)", "hmac(sha384)", "hmac(sha256)", : "hmac(sha224)", "hmac(sha3-512)", : /* only if !CONFIG_FIPS */ : #define TEST_NON_FIPS_ALGOS 2 : "hmac(rmd160)", "hmac(md5)" : };
Thanks, Dmitry
On Wed, 24 Jan 2024 17:46:10 +0000 Dmitry Safonov wrote:
FWIW the config used is uploaded with the results. If you click on the remote it should take you to a location like this:
https://netdev-2.bots.linux.dev/vmksft-tcp-ao/results/435369/
and there should be a config file in there.
I was stuck in a meeting and I started playing around with the options for TCP-AO :) I added these options now:
CONFIG_CRYPTO_HMAC=y CONFIG_CRYPTO_SHA1=y CONFIG_CRYPTO_RMD160=y CONFIG_IPV6=y CONFIG_TCP_AO=y CONFIG_TCP_MD5SIG=y CONFIG_VETH=m
And it looks much better! There are still some failures:
https://netdev.bots.linux.dev/contest.html?branch=net-next-2024-01-24--18-00...
I added VRF so that should hopefully take care of the MD5 skips on the next run. But the failures of the rst-ip* tests don't look like an obvious config problem.
linux-kselftest-mirror@lists.linaro.org