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

[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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, no TCP selftest checks the TX peer-ID that
  goes out on the wire.

--

selftests: ovpn: enable TCP_NODELAY on TCP sockets

This commit makes ovpn-cli set TCP_NODELAY on its TCP sockets by default.
Setting OVPN_TCP_NODELAY=0 turns it off, and the new test-tcp-nagle.sh
uses that to keep running the TCP suite with Nagle enabled. It also limits
the tcpdump-based peer ID check in ovpn_run_basic_traffic() to UDP.

> 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

[ ... ]

> @@ -173,8 +171,10 @@ ovpn_run_basic_traffic() {
>  			ip netns exec ovpn_peer0 \
>  			ping -qfc 100 -s 3000 -w 3 5.5.5.$((p + 1))
>  
> -		wait "${tcpdump_pid1}" || return 1
> -		wait "${tcpdump_pid2}" || return 1
> +		if [ "${OVPN_PROTO}" == "UDP" ]; then
> +			wait "${tcpdump_pid1}" || return 1
> +			wait "${tcpdump_pid2}" || return 1
> +		fi
>  	done
>  }

[Severity: Medium]
This patch removes the TCP branch of ovpn_build_capture_filter() and makes
these waits UDP-only. After that, does any TCP selftest still check the
peer-id that goes out on the wire?

The capture was added by commit 367f4b163a8c ("selftests: ovpn: check
asymmetric peer-id"). Its purpose was to check the peer-id in the data
packet headers for every connection, TCP included.

On the kernel side, the TCP receive path never reads the peer-id from the
packet. It takes the peer that is bound to the socket:

drivers/net/ovpn/tcp.c:ovpn_tcp_rcv() {
    ...
	ovpn_recv(peer, skb);
    ...
}

So if a TCP peer had the wrong tx_id here, traffic would still flow:

drivers/net/ovpn/crypto_aead.c:ovpn_aead_encrypt() {
    ...
	op = ovpn_opcode_compose(OVPN_DATA_V2, ks->key_id, peer->tx_id);
    ...
}

The JSON expectations in the selftest directory don't compare tx-id
either. With this change, test-tcp.sh, test-symmetric-id-tcp.sh and the new
test-tcp-nagle.sh would all pass with an incorrect TCP tx_id.

The later patch in the series ("selftests: ovpn: wait for the TCP server
key setup before traffic") does not bring the TCP check back.

Could these TCP variants keep some form of peer-id check that does not
depend on TCP segment boundaries?

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