Re: [PATCH net-next 5/7] selftests: ovpn: wait for the TCP server key setup before traffic

[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 new failure path for server-side key setup gives no hint of
  its cause.
- [Low] Claim: the patch fixes a race in the ovpn TCP selftest, in the
  backgrounded `listen && { new_key loop }` subshell plus `sleep 5` in…
- [Low] Claim: in ovpn_wait_tcp_keys() (common.sh lines 301-319),
  OVPN_TCP_KEYS_PID is a bare number that is never initialised or…

--

selftests: ovpn: wait for the TCP server key setup before traffic

In TCP mode, ovpn_add_peer() spawns a background subshell that accepts the
peers and installs their data keys. This commit records that subshell's PID.
A new helper, ovpn_wait_tcp_keys(), polls the PID with a bounded timeout. It
is called from test.sh and test-close-socket.sh once all peers are
registered.

> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>

[Severity: Low]
This might not matter, but should this patch have a Fixes: tag?

The backgrounded "listen && { new_key loop }" construct comes from commit
959bc330a439 ("testing/selftests: add test tool and scripts for ovpn
module"). It was later reworked in 7c29665a3a3c.

However, the commit message says the race was latent:

> Until now the test happened to work because of
> the unrelated delays that ran in between.

The delay that hid it seems to be the tcpdump setup plus the sleep 0.3 in
ovpn_run_basic_traffic(). The previous patch in this series ("selftests:
ovpn: enable TCP_NODELAY on TCP sockets") moved that delay under the
UDP-only branch.

If the race only shows up within this series, leaving out the tag is
probably fine.

> diff --git a/tools/testing/selftests/net/ovpn/common.sh b/tools/testing/selftests/net/ovpn/common.sh
> index 2f7851b823436..e33b1fe265221 100644
> --- a/tools/testing/selftests/net/ovpn/common.sh
> +++ b/tools/testing/selftests/net/ovpn/common.sh
> @@ -258,12 +258,16 @@ ovpn_add_peer() {
>  		if [ ${1} -eq 0 ]; then
>  			(ip netns exec "${server_ns}" ${OVPN_CLI} listen tun0 \
>  				1 ${M_ID} ${OVPN_TCP_PEERS_FILE} && {
> +				rc=0
>  				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
> +						1 0 ${OVPN_ALG} 0 data64.key \
> +						|| rc=1
>  				done
> +				exit ${rc}
>  			}) &
> +			OVPN_TCP_KEYS_PID=$!

[Severity: Low]
If this subshell fails, where does the output of listen and new_key go?

ovpn_add_peer 0 runs under ovpn_cmd_ok "register peer0 in overlay". At
that point ovpn_cmd_run() has already sent stdout/stderr to a temporary
file:

ovpn_cmd_run() {
    ...
	output_file=$(mktemp)
	if "$@" >"${output_file}" 2>&1; then
    ...
	rm -f "${output_file}"
	return "${ret}"
}

The backgrounded subshell inherits those descriptors. The file is deleted as
soon as ovpn_add_peer 0 returns, so anything ovpn-cli writes after that goes
to an unlinked inode.

With this patch, a listen or new_key failure is now a hard error reported
through ovpn_wait_tcp_keys(). The log would only show:

  wait for server-side key setup: command failed with rc=1: ovpn_wait_tcp_keys

It would contain no ovpn-cli error text and no failing peer id. Setting
OVPN_VERBOSE=1 doesn't help either, because the verbose dump in
ovpn_cmd_run() runs before the subshell has written anything.

Could the subshell output go to a dedicated log file, printed when
ovpn_wait_tcp_keys() fails?

>  			sleep 5
>  		else
>  			peer_ns="ovpn_peer${1}"

[ ... ]

> @@ -281,6 +285,39 @@ ovpn_add_peer() {

[ ... ]

> +OVPN_TCP_KEYS_TIMEOUT=${OVPN_TCP_KEYS_TIMEOUT:-30}
> +
> +ovpn_wait_tcp_keys() {
> +	local deadline
> +
> +	[ -n "${OVPN_TCP_KEYS_PID:-}" ] || return 0
> +
> +	deadline=$((SECONDS + OVPN_TCP_KEYS_TIMEOUT))
> +	while kill -0 "${OVPN_TCP_KEYS_PID}" 2>/dev/null; do
> +		if [ "${SECONDS}" -ge "${deadline}" ]; then
> +			printf '%s\n' \
> +				"server-side key setup still running after ${OVPN_TCP_KEYS_TIMEOUT}s"
> +			kill "${OVPN_TCP_KEYS_PID}" 2>/dev/null

[Severity: Low]
This is likely minor. OVPN_TCP_KEYS_PID is never initialized at file scope
and never cleared after the wait. It is only set in ovpn_add_peer(), for
peer 0 in TCP mode.

Could a value inherited from the environment make a UDP run poll an
unrelated process here and then send it SIGTERM on timeout? It could also
make the final wait fail with 127.

There is a related question about PID reuse. Once bash reaps the subshell
during the sleep 0.2, kill -0 and kill are working on a bare number.

In practice:

- each test runs as its own process
- the variable isn't exported
- ovpn_wait_tcp_keys() is called once per process
- cyclic PID allocation makes reuse within 0.2s unlikely at normal pid_max
  values

So if anything is needed, initializing OVPN_TCP_KEYS_PID= at the top of
common.sh and clearing it after the wait might be enough.

> +			wait "${OVPN_TCP_KEYS_PID}" 2>/dev/null
> +			return 1
> +		fi
> +		sleep 0.2
> +	done
> +
> +	wait "${OVPN_TCP_KEYS_PID}"
> +}

[ ... ]

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