Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] test_sctp_nat_connect_v4()
(tools/testing/selftests/net/openvswitch/openvswitch.sh, new lines…
--
selftests/net/openvswitch: add SCTP flow key test across conntrack NAT
This adds test_sctp_nat_connect_v4, which drives an SCTP association
through ct(commit,nat(dst=172.31.110.20:5555)),recirc and matches the
translated tuple after recirculation, so the SCTP branch of
ovs_nat_update_key() is actually exercised. The config fragment gains
CONFIG_NETFILTER_ADVANCED, CONFIG_NF_CT_PROTO_SCTP and CONFIG_NF_NAT.
One question below about listener teardown.
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index aa84fafc3201..21444657a040 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
[ ... ]
> @@ -1200,6 +1201,98 @@ test_nat_connect_v4 () {
> return 0
> }
>
> +# sctp_nat_connect_v4 test
> +# - SCTP association crosses a ct(commit,nat(dst=ip:port)) DNAT
> +# - post-recirc flows match the translated address and port, so the
> +# SCTP branch of the post-NAT flow key update is load-bearing
> +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"
[ ... ]
> + ovs_netns_spawn_daemon "test_sctp_nat_connect_v4" "server" \
> + socat -u -t 1 SCTP4-LISTEN:5555,fork \
> + OPEN:"$rxfile",creat,append
> + ovs_wait sctp_eps_has server 5555 || return 1
> +
> + info "verify SCTP association across NAT"
> + ovs_sbx "test_sctp_nat_connect_v4" ip netns exec client \
> + timeout 3 socat -u STDIN "SCTP4-CONNECT:192.168.0.20:4443" \
> + </dev/null || return 1
[Severity: Low]
This isn't a bug, but can a forked socat child outlive teardown here?
The listener in test_sctp_nat_connect_v4() is started with the fork
option, so socat forks one child per accepted association, while
ovs_netns_spawn_daemon() only records the pid of the backgrounded
ip netns exec:
tools/testing/selftests/net/openvswitch/openvswitch.sh:ovs_netns_spawn_daemon()
{
...
ip netns exec $netns $* >> $ovs_dir/stdout 2>>
$ovs_dir/stderr &
fi
pid=$!
ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
}
There is no process group kill and no wait, so a child still blocked
reading its SCTP socket is not signalled or reaped. run_test() then runs
ovs_exit_sig() followed immediately by rm -rf "$ovs_dir", and the
registered on_exit actions do ip netns del server, so such a child would
keep a reference on the server netns and an open descriptor on the
unlinked sctp-rx.txt until the association's retransmit/heartbeat timers
give up. The -t 1 option bounds the linger after EOF is seen, not the
wait for EOF itself.
This would show up when the client is killed by timeout 3, or when a
post-recirc flow is missing so the peer's SHUTDOWN/ABORT is dropped. On
the passing path the client closes stdin right away (</dev/null and the
here-string), so the child sees EOF and exits.
The same forking-listener plus single-pid SIGTERM pattern is already used
by test_sctp_connect_v4() and test_sctp_connect_v6() in this file, so
this may simply be the convention here. Would it be worth killing the
process group and waiting for the listener in ovs_netns_spawn_daemon()
instead, which would cover all three tests?
> +
> + 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
> + ovs_wait grep -q "$payload" "$rxfile" \
> + || { info "server did not receive SCTP DATA payload"
> + return 1; }
> +
> + info "done..."
> + return 0
> +}
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918144647.2024095-1-houminxi%40gmail.com
