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

[Antonio Quartulli <antonio@openvpn.net>]

Hi,

On 13/10/2026 16:39, netdev-bot+sashiko@kernel.org wrote:
> 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?
> 

Testing tx-peer-id with TCP_NODELAY enabled is extremely challenging 
because we'd need to (almost) re-implement an OpenVPN TCP stream parser 
in the selftest.

Since the rest of the selftests already ensures that packets are well 
formed and that the ovpn TCP state machine works as expected, we decided 
to simply give up on this specific on-wire test.
We don't think to be losing coverage by doing so.

Hence this decision was deliberate.

Regards,



-- 
Antonio Quartulli
OpenVPN Inc.