[email protected] writes:
> 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?
A separate process group would be over kill. This is already
convention.
>> +
>> + 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
>> +}
>
> [ ... ]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
