Re: [PATCH net-next v5 1/2] selftests/net/openvswitch: add SCTP flow key test over IPv6

[Aaron Conole <aconole@redhat.com>]

netdev-bot+sashiko@kernel.org 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.