Re: [PATCH net-next v4 2/2] selftests/net/openvswitch: add SCTP NAT test with port translation

[netdev-bot+sashiko@kernel.org]

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