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

[Antonio Quartulli <antonio@openvpn.net>]

Hi,

a few of Sashiko's finding are correct and worth investigating faster.
I'll respin this PR after having addressed the concerns.

Thanks,

pw-bot: cr

On 23/09/2026 08:49, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · 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] New helper ovpn_tcp_nodelay() in
>    tools/testing/selftests/net/ovpn/ovpn-cli.c references the TCP_NODELAY…
> - [Medium] In TCP mode the server-side data keys are installed by a
>    backgrounded subshell in ovpn_add_peer() ('(… ovpn-cli listen tun0 1…
> 
> --
> 
> selftests: ovpn: enable TCP_NODELAY on TCP sockets
> 
> This enables TCP_NODELAY on ovpn-cli's TCP sockets (both the accepted
> server side and the connected client side) and restricts the
> tcpdump-based peer ID capture check in the test script to UDP, since TCP
> does not preserve record boundaries.
> 
> Two questions below.
> 
> [ ... ]
> 
>> diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c
>> index f4effa7580c0f..6b458a654a337 100644
>> --- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
>> +++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
>> @@ -470,6 +470,18 @@ static int ovpn_parse_key_direction(const char *dir, struct ovpn_ctx *ctx)
>>   	return 0;
>>   }
>>   
>> +static int ovpn_tcp_nodelay(int socket)
>> +{
>> +	int opt = 1;
>> +	int ret;
>> +
>> +	ret = setsockopt(socket, IPPROTO_TCP, TCP_NODELAY, &opt, sizeof(opt));
> 
> [Severity: Medium]
> Does ovpn-cli still compile with this?  TCP_NODELAY is used here for the
> first time in this file, but the include block at the top of ovpn-cli.c is:
> 
> 	#include <stdint.h>
> 	#include <stdio.h>
> 	#include <inttypes.h>
> 	#include <stdbool.h>
> 	#include <string.h>
> 	#include <errno.h>
> 	#include <unistd.h>
> 	#include <arpa/inet.h>
> 	#include <net/if.h>
> 	#include <netinet/in.h>
> 	#include <time.h>
> 	...
> 	#include <sys/socket.h>
> 
> netinet/in.h provides IPPROTO_TCP but not TCP_NODELAY, so unless one of the
> libnl or mbedtls headers happens to pull in netinet/tcp.h on the build host,
> this fails with "'TCP_NODELAY' undeclared (first use in this function)".
> 
> Other net selftests using the same setsockopt() call include both headers
> explicitly, for example fin_ack_lat.c:
> 
> 	#include <netinet/in.h>
> 	#include <netinet/tcp.h>
> 
> and txtimestamp.c does the same.
> 
> Since ovpn-cli is listed in TEST_GEN_FILES in
> tools/testing/selftests/net/ovpn/Makefile and every ovpn TEST_PROGS script
> invokes it, would a failure here take out the whole ovpn suite?  Would
> adding #include <netinet/tcp.h> be the fix?  It is still missing at the end
> of the series (bc56ca6a91b2).
> 
>> +	if (ret < 0)
>> +		perror("setsockopt for TCP_NODELAY");
>> +
>> +	return ret;
>> +}
>> +
> 
> [ ... ]
> 
>> diff --git a/tools/testing/selftests/net/ovpn/test.sh b/tools/testing/selftests/net/ovpn/test.sh
>> index 9b5610837032f..d744c1a97d5fc 100755
>> --- a/tools/testing/selftests/net/ovpn/test.sh
>> +++ b/tools/testing/selftests/net/ovpn/test.sh
>> @@ -67,35 +67,33 @@ ovpn_run_basic_traffic() {
>>   	local tcpdump_timeout="1.5s"
>>   
>>   	for p in $(seq 1 ${OVPN_NUM_PEERS}); do
>> -		# The first part of the data packet header consists of:
>> -		# - TCP only: 2 bytes for the packet length
>> -		# - 5 bits for opcode ("9" for DATA_V2)
>> -		# - 3 bits for key-id ("0" at this point)
>> -		# - 12 bytes for peer-id:
>> -		#     - with asymmetric ID: "${p}" one way and "${p} + 9" the
>> -		#	other way
>> -		#     - with symmetric ID: "${p}" both ways
>> -		header1=$(printf "0x4800000%x" ${p})
>> -		header2=$(printf "0x4800000%x" $((p + OVPN_ID_OFFSET)))
>> -		raddr=""
>>   		if [ "${OVPN_PROTO}" == "UDP" ]; then
>> +			# The first part of the data packet header consists of:
>> +			# - 5 bits for opcode ("9" for DATA_V2)
>> +			# - 3 bits for key-id ("0" at this point)
>> +			# - 3 bytes for peer-id:
>> +			#     - with asymmetric ID: "${p}" one way and "${p} + 9" the
>> +			#	other way
>> +			#     - with symmetric ID: "${p}" both ways
>> +			header1=$(printf "0x4800000%x" ${p})
>> +			header2=$(printf "0x4800000%x" $((p + OVPN_ID_OFFSET)))
>>   			raddr=$(awk "NR == ${p} {print \$3}" \
>>   				"${OVPN_UDP_PEERS_FILE}")
>> +			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
>>   		fi
>> +		ovpn_cmd_ok "send baseline traffic to peer ${p}" \
>> +			ip netns exec ovpn_peer0 \
>> +			ping -qfc 100 -w 3 5.5.5.$((p + 1))
> 
> [Severity: Medium]
> Can moving "sleep 0.3" inside the UDP-only branch make the TCP variants
> flaky?  It was the only delay between peer setup and the first ping, and in
> TCP mode the server-side data keys are installed by a backgrounded subshell
> in ovpn_add_peer() whose PID is discarded:
> 
> tools/testing/selftests/net/ovpn/common.sh:ovpn_add_peer() {
> 	...
> 		(ip netns exec "${server_ns}" ${OVPN_CLI} listen tun0 \
> 			1 ${M_ID} ${OVPN_TCP_PEERS_FILE} && {
> 			for p in $(seq 1 ${OVPN_NUM_PEERS}); do
> 				ip netns exec "${server_ns}" \
> 					${OVPN_CLI} new_key tun0 ${p} \
> 					1 0 ${OVPN_ALG} 0 data64.key
> 			done
> 		}) &
> 	...
> }
> 
> The "listen" command returns as soon as all peers are accepted, because
> CMD_LISTEN ends in ovpn_waitbg() -> daemon(1, 1), so the new_key loop runs
> concurrently with the foreground script.  With the sleep now gone for TCP,
> ovpn_run_basic_traffic() starts "ping -qfc 100 -w 3 5.5.5.2" with no
> synchronization against that loop.
> 
> If "new_key tun0 1" has not landed yet, server to client packets for peer 1
> are dropped for lack of a key, and with -c 100 plus the -w 3 deadline ping
> exits non-zero, so ovpn_cmd_ok reports "run baseline data traffic" as
> failed.
> 
> Would recording the background setup PID and waiting for it (or polling
> get_key) before generating traffic be more robust than relying on the
> incidental sleep?
> 
>>   @@ -103,8 +101,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
>>   }
> 

-- 
Antonio Quartulli
OpenVPN Inc.