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:
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?
> +
> + 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?
[ ... ]
> + 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?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918144647.2024095-1-houminxi%40gmail.com
