[email protected] writes:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The promised skip when IPv6 is unavailable is incomplete in
> test_sctp_connect_v6().
>
> --
>
> selftests/net/openvswitch: add SCTP flow key test over IPv6
>
> This adds test_sctp_connect_v6 to the openvswitch selftest, mirroring the
> existing IPv4 SCTP test: icmpv6() flows forward neighbour discovery while
> ipv6(proto=132),sctp(dst=4443)/sctp(src=4443) flows gate an SCTP
> association across the datapath, with a keyless ipv6(proto=132) install
> expected to be refused with EINVAL.
>
>> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> index a31f7fb6882d..aa84fafc3201 100755
>> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>
> [ ... ]
>
>> @@ -700,6 +701,114 @@ test_sctp_connect_v4() {
>> return 0
>> }
>>
>> +# sctp_connect_v6 test
>> +# - sctp(dst=4443) matches client-to-server INIT
>> +# - sctp(src=4443) matches server-to-client INIT-ACK
>> +# - icmpv6 NS/NA flows forward neighbour discovery
>> +# - remove flows and verify connection fails, reinstall and recover
>> +test_sctp_connect_v6() {
>> + local t="test_sctp_connect_v6"
>> + local v6="eth_type(0x86dd),ipv6(proto=132)"
>> + local payload="SCTP6_DATA_OK"
>> + local rxfile="${ovs_base}/${t}/sctp-rx.txt"
>> +
>> + modprobe -q sctp 2>/dev/null || return "$ksft_skip"
>> + socat -V 2>&1 | grep -q "define WITH_SCTP" || return "$ksft_skip"
>> + socat -V 2>&1 | grep -q "define WITH_IP6" || return "$ksft_skip"
>> + [ -e /proc/sys/net/ipv6 ] || return "$ksft_skip"
>
> [Severity: Medium]
>
> Is this gate enough to cover the case the changelog describes as "IPv6 is
> unavailable"? /proc/sys/net/ipv6 only tells us inet6_init() ran. Only
> ipv6.disable=1 short-circuits it:
We don't do much to check that ipv6 is available. The ICMPv6 test
doesn't do any additional safeguards.
> net/ipv6/af_inet6.c:inet6_init() {
> ...
> if (disable_ipv6_mod) {
> pr_info("Loaded, but administratively disabled, reboot required
> to enable\n");
> goto out;
> }
> ...
> }
>
> With ipv6.disable_ipv6=1 instead, initialization completes and the sysctl
> tree exists, so this check passes, but the module default is copied into
> every new netns:
>
> net/ipv6/addrconf.c:addrconf_init_net() {
> ...
> /* these will be inherited by all namespaces */
> dflt->autoconf = ipv6_defaults.autoconf;
> dflt->disable_ipv6 = ipv6_defaults.disable_ipv6;
> ...
> }
>
> so the veths created for the client and server namespaces come up with
> cnf.disable_ipv6 set, and address assignment is rejected:
>
> net/ipv6/addrconf.c:ipv6_add_addr() {
> ...
> if (idev->cnf.disable_ipv6) {
> NL_SET_ERR_MSG_MOD(extack, "IPv6 is disabled on this device");
> err = -EACCES;
> goto out;
> }
> ...
> }
>
> Would testing something that reflects usability, for example whether an
> address can actually be added in a test namespace, match the promised skip
> better?
We don't generally have a check for this. Maybe it makes sense, but
that could be a separate cleanup if it really is needed.
>> +
>> + sbx_add "$t" || return $?
>> + ovs_add_dp "$t" sctp6 || return 1
>> +
>> + info "create namespaces"
>> + for ns in client server; do
>> + ovs_add_netns_and_veths "$t" "sctp6" "$ns" \
>> + "${ns:0:1}0" "${ns:0:1}1" || return 1
>> + done
>> +
>> + ip netns exec client ip addr add fd00::1/64 dev c1 nodad
>> + ip netns exec client ip link set c1 up
>> + ip netns exec server ip addr add fd00::2/64 dev s1 nodad
>> + ip netns exec server ip link set s1 up
>
> [Severity: Medium]
>
> These four setup commands in test_sctp_connect_v6() discard their exit
> status, unlike the surrounding calls which all use "|| return 1". If the
> ip addr add calls fail with EACCES because disable_ipv6 was inherited by
> the new namespaces, does the test keep going with no IPv6 addresses
> configured and then report a failure at the association check below?
>
> Would checking these with "|| return $ksft_skip" (or at least
> "|| return 1" with an info message naming the failed step) make the
> outcome match the cause?
See above - same answer.
> [ ... ]
>
>> + info "verify SCTP association with port-keyed flows"
>> + ovs_sbx "$t" ip netns exec client \
>> + timeout 3 socat -u STDIN "SCTP6-CONNECT:[fd00::2]:4443" </dev/null \
>> + || return 1
>
> [Severity: Medium]
>
> This is the point where an unusable IPv6 setup surfaces. With
> ipv6.disable_ipv6=1 the connect cannot succeed, so run_test() reports FAIL,
> while the changelog says:
>
> Skips when the sctp module is missing, socat lacks SCTP or IPv6
> support, or IPv6 is unavailable
>
> Should this configuration end up as a skip rather than a failure, or should
> the changelog be narrowed to say the skip only covers IPv6 not being
> initialized at all?
I don't think either is needed for this series.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
