Re: [PATCH net-next 5/6] 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 commit fixes a real ordering race but has no Fixes: tag.
- [Low] In TCP mode, ovpn_add_peer() creates a temporary file with
  `OVPN_TCP_KEYS_LOG=$(mktemp)` and redirects the background subshell…
- [Low] ovpn_wait_tcp_keys() decides whether the background subshell is
  still running with `while kill -0 "${OVPN_TCP_KEYS_PID}"`.

--

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

This patch saves the PID of the background TCP server subshell that
ovpn_add_peer() starts. That subshell runs listen and then new_key for
every peer, and its output now goes to a temporary log file. A new
ovpn_wait_tcp_keys() polls the subshell with a 30s bound, dumps the log
on failure, and is called from test.sh and test-close-socket.sh once all
peers are registered.

> The sleep 5 that follows does not cover it: it elapses while "listen" is
> still waiting for connections, and the peers only connect in the
> subsequent ovpn_add_peer() iterations, so the key loop starts well after
> that sleep has expired. Until now the test happened to work because of
> the unrelated delays that ran in between.

[ ... ]

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

[Severity: Low]
This fixes a race in the test, but there is no Fixes: tag. Should one be
added?

The background listen/new_key subshell followed by sleep 5 in
ovpn_add_peer() comes from:

959bc330a439 ("testing/selftests: add test tool and scripts for ovpn module")

Commit 7c29665a3a3c later rewrote the listen and new_key lines but kept
the same structure.

Documentation/process/maintainer-netdev.rst says "for fixes the Fixes:
tag is required, regardless of the tree". The exception there for tests
only covers tests that come with a code fix.

Also, the previous patch in this series ("selftests: ovpn: enable
TCP_NODELAY on TCP sockets") removed the two tcpdump captures and the
sleep 0.3 before the first TCP ping. That may be one of the "unrelated
delays" mentioned above.

> diff --git a/tools/testing/selftests/net/ovpn/common.sh b/tools/testing/selftests/net/ovpn/common.sh
> index 2f7851b823436..da732c64ffea6 100644
> --- a/tools/testing/selftests/net/ovpn/common.sh
> +++ b/tools/testing/selftests/net/ovpn/common.sh
> @@ -256,14 +256,19 @@ ovpn_add_peer() {
>  		fi
>  	else
>  		if [ ${1} -eq 0 ]; then
> +			OVPN_TCP_KEYS_LOG=$(mktemp)
>  			(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_LOG}" 2>&1 &
> +			OVPN_TCP_KEYS_PID=$!
>  			sleep 5
>  		else
>  			peer_ns="ovpn_peer${1}"

[ ... ]

> +ovpn_wait_tcp_keys() {
> +	local deadline rc
> +
> +	[ -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]
Can kill -0 and kill here end up hitting an unrelated process?

Both use the raw number saved by OVPN_TCP_KEYS_PID=$! in ovpn_add_peer().
Bash reaps background children itself, both from its SIGCHLD handling
and while it waits for the foreground sleep 0.2. It keeps the exit status
so the later wait still works.

Once the child is reaped, its PID is free in the kernel and can be given
to another task. If that happens before the next kill -0:

  - the loop treats the unrelated process as the key setup job and spins
    until the 30s deadline
  - kill then sends SIGTERM to that process
  - setup is reported as failed even though the keys were installed

alloc_pid() hands out PIDs cyclically, so this needs a PID wraparound
within one poll interval. That makes it very unlikely in practice, but
the check is not tied to the job's lifetime.

Could the liveness check and the kill be tied to the background job
itself rather than to the numeric PID?

> +			wait "${OVPN_TCP_KEYS_PID}" 2>/dev/null
> +			ovpn_dump_tcp_keys_log
> +			return 1

[Severity: Low]
Does this timeout path leak the OVPN_TCP_KEYS_LOG file that mktemp
creates in ovpn_add_peer()?

The only removal is the rm -f "${OVPN_TCP_KEYS_LOG}" further down. It
only runs when the subshell exits on its own, and this return 1 skips it.

The file is also left behind if a later ovpn_add_peer() call fails before
ovpn_wait_tcp_keys() is reached, for example when connect fails. Under
set -eE that path goes through the EXIT trap to ovpn_cleanup(), which
never touches OVPN_TCP_KEYS_LOG:

ovpn_cleanup() {
	local peer

	# some ovpn-cli processes sleep in background so they need manual poking
	killall "$(basename "${OVPN_CLI}")" 2>/dev/null || true

	for peer in "${!OVPN_LISTENER_PIDS[@]}"; do
		ovpn_stop_listener "${peer}" 2>/dev/null
	done
	...
}

The existing OVPN_TMP_JSONS temp files are already removed from
ovpn_cleanup() through ovpn_stop_listener(). Should OVPN_TCP_KEYS_LOG be
removed from ovpn_cleanup() too?

> +		fi
> +		sleep 0.2
> +	done

[ ... ]

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