Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 0
- [Medium] tools/testing/selftests/net/openvswitch/config gains
CONFIG_NF_CT_PROTO_SCTP=y for the new test_sctp_nat_connect_v4 test…
--
selftests/net/openvswitch: add SCTP NAT test with port translation
Adds test_sctp_nat_connect_v4, which runs an SCTP association through a
ct(commit,nat(dst=ip:port)) DNAT and matches the translated tuple after
recirculation, plus CONFIG_NF_CT_PROTO_SCTP=y in the config fragment.
> diff --git a/tools/testing/selftests/net/openvswitch/config
> b/tools/testing/selftests/net/openvswitch/config
> index a825e0b5c88e..fa90475dc388 100644
> --- a/tools/testing/selftests/net/openvswitch/config
> +++ b/tools/testing/selftests/net/openvswitch/config
> @@ -7,6 +7,7 @@ CONFIG_NET_IPGRE=m
> CONFIG_NET_IPGRE_DEMUX=m
> CONFIG_NF_CONNTRACK=m
> CONFIG_NF_CONNTRACK_OVS=y
> +CONFIG_NF_CT_PROTO_SCTP=y
> CONFIG_OPENVSWITCH=m
> CONFIG_PSAMPLE=m
> CONFIG_VETH=y
[Severity: Medium]
Should this fragment also request CONFIG_NF_NAT=m (and
CONFIG_NETFILTER_ADVANCED=y, which the changelog says the SCTP conntrack
symbol needs)?
The new test's probe uses a nat() action, and net/openvswitch/Kconfig only
has:
select NF_NAT_OVS if NF_NAT
so it never turns NF_NAT on itself. In net/netfilter/Kconfig, NF_NAT is:
config NF_NAT
tristate "Network Address Translation support"
depends on NF_CONNTRACK
default m if NETFILTER_ADVANCED=n
With NETFILTER_ADVANCED=y and no iptables/nftables NAT option selected,
NF_NAT ends up n, and in net/openvswitch/conntrack.c the nat attribute is
compiled out:
#if IS_ENABLED(CONFIG_NF_NAT)
case OVS_CT_ATTR_NAT: {
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 1160f8ed7d8a..138a705fc359 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
[ ... ]
> @@ -1199,6 +1200,98 @@ test_nat_connect_v4 () {
[ ... ]
> + # Check if the ct and nat actions can be configured.
> + ovs_add_flow "test_sctp_nat_connect_v4" sctpnat4 \
> + 'in_port(1),eth(),eth_type(0x0800),ipv4()' \
> + 'ct(commit,nat(dst=172.31.110.20:5555)),recirc(0x1)' \
> + &> /dev/null
> + if [ $? == 1 ]; then
> + info "no support for ct/nat actions - skipping"
> + ovs_exit_sig
> + return $ksft_skip
> + fi
[Severity: Medium]
Following on from the config question above: when NF_NAT is n, does this
probe in test_sctp_nat_connect_v4() always fail so the test permanently
returns $ksft_skip?
That would leave the SCTP arm of ovs_nat_update_key() with no coverage,
which is the path the changelog says is currently unexercised, and the loss
would be silent. The existing test_nat_connect_v4() fails loudly instead:
ovs_sbx "test_nat_connect_v4" ip netns exec client ping -c 1 -W 1 \
172.31.110.20 || return 1
Would adding CONFIG_NF_NAT to the fragment keep this test running on the
reference config?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909122521.2687193-1-houminxi%40gmail.com
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
