Re: [PATCH net-next v2 2/2] selftests/net/openvswitch: add SCTP flow key test across conntrack NAT

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 1

- [Medium] Commit-message/coverage mismatch: the new
  `test_sctp_nat_connect_v4` cannot detect the bug class its commit…
- [Medium] `test_sctp_nat_connect_v4` probes only the plain `ct(commit)`
  action (openvswitch.sh:1224-1231) before deciding SKIP vs. run, but…
- [Medium] Unsynchronized cross-process check makes the new test flaky.
- [Low] The Open vSwitch selftest Kconfig fragment
  (tools/testing/selftests/net/openvswitch/config) is not updated for…

--

selftests/net/openvswitch: add SCTP flow key test across conntrack NAT

This adds test_sctp_nat_connect_v4 to the openvswitch selftest suite. It
builds a client/server namespace pair, installs ct(commit,nat(...)) plus
recirc flows, and checks that an SCTP association and a DATA payload cross
the translation.

A few questions about the new test below.

> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 06bd98d80a08..d5acdca0a0ac 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh

[ ... ]

> @@ -1186,6 +1187,91 @@ test_nat_connect_v4 () {
>  	return 0
>  }
>  
> +# sctp_nat_connect_v4 test
> +#  - SCTP association crosses a ct(commit,nat(dst=...)) translation
> +#  - post-recirc flows match ct_state(+trk) plus the extracted SCTP ports
> +test_sctp_nat_connect_v4 () {
> +	local t="test_sctp_nat_connect_v4"
> +	local payload="SCTP_NAT_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"
> +	# SCTP conntrack is compiled into nf_conntrack.ko, so check that
> +	# loading it actually exposed the SCTP conntrack sysctls.
> +	modprobe -q nf_conntrack 2>/dev/null || return "$ksft_skip"
> +	[ -e /proc/sys/net/netfilter/nf_conntrack_sctp_timeout_established ] \
> +	    || { info "no SCTP conntrack support - skipping"
> +	         return "$ksft_skip"; }

[Severity: Low]

Should tools/testing/selftests/net/openvswitch/config gain the kernel options
this new test depends on?

The sysctl probed here only exists when CONFIG_NF_CT_PROTO_SCTP is enabled,
and that option is an independent bool:

net/netfilter/Kconfig:
	config NF_CT_PROTO_SCTP
		bool 'SCTP protocol connection tracking support'
		depends on NETFILTER_ADVANCED
		default y

With NETFILTER_ADVANCED=n the dependency is unmet and the option is off in
spite of the default, and a base config that sets it to n is unaffected by
merging the fragment.

The current fragment has:

	CONFIG_IP_SCTP=y
	CONFIG_NF_CONNTRACK=m
	CONFIG_NF_CONNTRACK_OVS=y
	CONFIG_OPENVSWITCH=m

so in those builds the test returns $ksft_skip and the coverage the changelog
describes never runs. Would adding CONFIG_NF_CT_PROTO_SCTP=y (and
CONFIG_NF_NAT=y, see below) to the fragment make it deterministic?

[ ... ]

> +	# Check if the ct action can be configured.
> +	ovs_add_flow "test_sctp_nat_connect_v4" sctpnat4 \
> +		'in_port(1),eth(),eth_type(0x0800),ipv4()' \
> +		'ct(commit),recirc(0x1)' &> /dev/null
> +	if [ $? == 1 ]; then
> +		info "no support for ct action - skipping"
> +		ovs_exit_sig
> +		return $ksft_skip
> +	fi

[Severity: Medium]

This probe tests plain ct(commit), but the flows installed below require the
nat action. Does that turn a missing CONFIG_NF_NAT into a reported failure
instead of a skip?

In net/openvswitch/conntrack.c, both the policy entry and the parse arm are
compiled conditionally:

	#if IS_ENABLED(CONFIG_NF_NAT)
		/* NAT length is checked when parsing the nested attributes. */
		[OVS_CT_ATTR_NAT]	= { .minlen = 0, .maxlen = INT_MAX },
	#endif

With NF_NAT off the entry is {0, 0}, so the nested nat attribute fails the
length check in parse_ct() and the flow add is rejected with -EINVAL, while
the ct(commit) probe above still succeeds.

That configuration is reachable from the suite's own fragment, which does not
set CONFIG_NF_NAT, and:

net/netfilter/Kconfig:
	config NF_NAT
		tristate "Network Address Translation support"
		depends on NF_CONNTRACK
		default m if NETFILTER_ADVANCED=n

CONFIG_OPENVSWITCH only does "select NF_NAT_OVS if NF_NAT", so with the usual
NETFILTER_ADVANCED=y, NF_NAT defaults to n. Would probing with
ct(commit,nat(dst=...)) instead, or skipping rather than failing on the nat
flow adds, keep this a skip?

[ ... ]

> +	ovs_add_flow "test_sctp_nat_connect_v4" sctpnat4 \
> +		"ct_state(-trk),in_port(1),eth(),eth_type(0x0800),ipv4(dst=192.168.0.20)" \
> +		"ct(commit,nat(dst=172.31.110.20)),recirc(0x1)" || return 1
> +	ovs_add_flow "test_sctp_nat_connect_v4" sctpnat4 \
> +		"ct_state(-trk),in_port(2),eth(),eth_type(0x0800),ipv4()" \
> +		"ct(commit,nat),recirc(0x2)" || return 1
> +
> +	ovs_add_flow "test_sctp_nat_connect_v4" sctpnat4 \
> +		"recirc_id(0x1),ct_state(+trk-inv),in_port(1),eth(),eth_type(0x0800),ipv4(proto=132),sctp(dst=4443)" \
> +		"2" || return 1
> +	ovs_add_flow "test_sctp_nat_connect_v4" sctpnat4 \
> +		"recirc_id(0x2),ct_state(+trk-inv),in_port(2),eth(),eth_type(0x0800),ipv4(proto=132),sctp(src=4443)" \
> +		"1" || return 1

[Severity: Medium]

Can these flows actually detect a post-NAT SCTP flow key bug?

nat(dst=172.31.110.20) rewrites only the IPv4 destination and never a port,
and the post-recirc flows match ipv4(proto=132) plus sctp(dst=4443) /
sctp(src=4443) - exactly the fields NAT leaves alone. The translated
addresses are not matched at all.

The post-NAT ports are patched into the key here:

net/openvswitch/conntrack.c:ovs_nat_update_key() {
	...
		if (key->ip.proto == IPPROTO_UDP)
			dst = udp_hdr(skb)->dest;
		else if (key->ip.proto == IPPROTO_TCP)
			dst = tcp_hdr(skb)->dest;
		else if (key->ip.proto == IPPROTO_SCTP)
			dst = sctp_hdr(skb)->dest;
		else
			return;

		key->tp.dst = dst;
	...
}

If the IPPROTO_SCTP arms were deleted so the function took the bare return,
key->tp.dst / key->tp.src would keep their identical first-pass values and
both flows would still match, so the test would still pass.

Would matching on distinct translated SCTP ports, and/or on the translated
addresses in the post-recirc flows, give the intended coverage?

Also on the changelog:

	Conntrack NAT rewrites the packet between the first-pass and recirc
	passes, and OVS re-extracts L3/L4 on the recirculated skb via
	ovs_flow_key_update_l3l4().

Is that the path this test exercises? ovs_flow_key_update_l3l4() in
net/openvswitch/flow.c has a single caller:

net/openvswitch/conntrack.c:ovs_ct_handle_fragments() {
	...
	/* The key extracted from the fragment that completed this datagram
	 * likely didn't have an L4 header, so regenerate it.
	 */
	ovs_flow_key_update_l3l4(skb, key);
	...
}

that is, only for datagrams reassembled from IP fragments, which this test
never produces. On the plain ct+recirc path the key is not re-extracted:

net/openvswitch/actions.c:execute_recirc() {
	...
	if (!is_flow_key_valid(key)) {
		int err;

		err = ovs_flow_key_update(skb, key);
	...
}

and the CT/NAT action does not call invalidate_flow_key().

	After the association succeeds the test pushes a known payload across and
	verifies the listener received it, so a flow-key bug that matches
	handshake packets but breaks DATA chunks is caught too.

Is there such a distinction? SCTP key extraction reads only the common
header source/destination ports, which are the same for INIT and DATA
chunks.

> +	ovs_netns_spawn_daemon "test_sctp_nat_connect_v4" "server" \
> +		socat -u -t 1 SCTP4-LISTEN:4443,fork \
> +		OPEN:"$rxfile",creat,append

[ ... ]

> +	info "verify SCTP DATA chunk crosses NAT"
> +	ovs_sbx "test_sctp_nat_connect_v4" ip netns exec client \
> +	    timeout 3 socat -u STDIN "SCTP4-CONNECT:192.168.0.20:4443" \
> +	    <<< "$payload" || return 1
> +	grep -q "$payload" "$rxfile" 2>/dev/null \
> +	    || { info "server did not receive SCTP DATA payload"
> +	         return 1; }

[Severity: Medium]

Can this grep race the server writing $rxfile?

The listener is started in the background:

ovs_netns_spawn_daemon() {
	...
		ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
	pid=$!
	...
}

and ",fork" means a separate per-association child does the read() and the
write() into $rxfile. The client socat exiting only means its own socket was
closed; delivery to the server socket, the child's read() and its write() to
the file all happen afterwards in another process, and "-t 1" decouples the
child's lifetime from the client's exit.

So this interleaving looks possible:

	client socat exits
	shell runs grep -q on an empty/short $rxfile   -> return 1
	server child writes payload

Every other cross-process observation in this file goes through the retry
helper, and this same test already uses it for endpoint readiness:

	ovs_wait sctp_eps_has server 4443 || return 1

with ovs_wait() polling up to WAIT_TIMEOUT (raised to 10s when
KSFT_MACHINE_SLOW=yes). Would "ovs_wait grep -q "$payload" "$rxfile"" be
more robust here?

The same one-shot grep pattern was introduced one patch earlier in this
series, in the IPv6 SCTP test, so both places would need the same change.

> +
> +	info "done..."
> +	return 0
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902125133.1828572-1-houminxi%40gmail.com