Re: [PATCH net-next 4/7] selftests: ovpn: enable TCP_NODELAY on TCP sockets

[netdev-bot+sashiko@kernel.org]

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] The patch reduces TCP selftest coverage.

--

selftests: ovpn: enable TCP_NODELAY on TCP sockets

This commit makes ovpn-cli set TCP_NODELAY on both the connecting and the
accepted TCP sockets, which matches the current userspace default. TCP does
not preserve record boundaries, so the tcpdump based DATA_V2 peer ID check
in ovpn_run_basic_traffic() now runs only for UDP. The TCP branch of
ovpn_build_capture_filter() is removed.

> diff --git a/tools/testing/selftests/net/ovpn/test.sh b/tools/testing/selftests/net/ovpn/test.sh
> index 392109d5e14e2..e2e6ffdd29bb0 100755
> --- a/tools/testing/selftests/net/ovpn/test.sh
> +++ b/tools/testing/selftests/net/ovpn/test.sh
> @@ -137,35 +137,33 @@ ovpn_run_basic_traffic() {
>  	local tcpdump_timeout="1.5s"
>  
>  	for p in $(seq 1 ${OVPN_NUM_PEERS}); do

[ ... ]

>  		if [ "${OVPN_PROTO}" == "UDP" ]; then

[ ... ]

> +			timeout ${tcpdump_timeout} ip netns exec "${peer_ns}" \
> +				tcpdump --immediate-mode -p -ni veth${p} -c 1 \
> +				"$(ovpn_build_capture_filter "${header1}" "${raddr}")" \
> +				>/dev/null 2>&1 &
> +			tcpdump_pid1=$!
> +			timeout ${tcpdump_timeout} ip netns exec "${peer_ns}" \
> +				tcpdump --immediate-mode -p -ni veth${p} -c 1 \
> +				"$(ovpn_build_capture_filter "${header2}" "${raddr}")" \
> +				>/dev/null 2>&1 &
> +			tcpdump_pid2=$!
> +
> +			sleep 0.3
>  		fi

[Severity: Low]
This isn't a bug, but now that the capture runs only for UDP,
test-tcp.sh and test-symmetric-id-tcp.sh no longer check the DATA_V2 peer
ID on the wire. Is there another way to keep some wire-level peer ID
coverage for the TCP variants?

On receive, ovpn_tcp_rcv() gets the peer from the stream, not from the
on-wire peer ID:

drivers/net/ovpn/tcp.c:ovpn_tcp_rcv() {
	struct ovpn_peer *peer = container_of(strp, struct ovpn_peer, tcp.strp);
    ...
}

So a successful ping over TCP does not by itself show that the correct
tx_id was sent.

tx_id is set in common code and written to the packet in
ovpn_aead_encrypt():

	op = ovpn_opcode_compose(OVPN_DATA_V2, ks->key_id, peer->tx_id);

The UDP variants, including test-symmetric-id.sh, still exercise that
path.

Separately, ovpn_connect() and ovpn_accept() in ovpn-cli.c now set
TCP_NODELAY unconditionally, so every TCP selftest runs with Nagle
disabled. Would it be worth adding an option that leaves Nagle enabled, so
that setup still gets tested?

> -		peer_ns="ovpn_peer${p}"
> -
> -		timeout ${tcpdump_timeout} ip netns exec "${peer_ns}" \
> -			tcpdump --immediate-mode -p -ni veth${p} -c 1 \
> -			"$(ovpn_build_capture_filter "${header1}" "${raddr}")" \
> -			>/dev/null 2>&1 &
> -		tcpdump_pid1=$!
> -		timeout ${tcpdump_timeout} ip netns exec "${peer_ns}" \
> -			tcpdump --immediate-mode -p -ni veth${p} -c 1 \
> -			"$(ovpn_build_capture_filter "${header2}" "${raddr}")" \
> -			>/dev/null 2>&1 &
> -		tcpdump_pid2=$!
> -
> -		sleep 0.3

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007133018.1451958-2-antonio%40openvpn.net